bug#9874: Fixes for several integer overflow and width issues

bug#9874: Fixes for several integer overflow and width issues

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 10446 bytes --]

Package: Emacs
Version: 24.0.90
Tags: patch

Attached is a patch to the Emacs trunk (bzr 106194) to fix several
issues, mostly due to integer overflow.  This patch was derived by
looking for and fixing common integer-width issues.  The patch does
not attempt to fix all these issues, just some issues that are clear
and for which the fixes are straightforward.

The patch only fixes problems; it does not add features.  As patches
go, it is relatively large (647 kB) so I have compressed it; if you
want an uncompressed copy please let me know.  I would like to install
it after some more testing.  If you'd like to wait until after the
next Emacs pretest, please let me know.

Here is a list of issues fixed.  This list is not complete, but it
covers the major issues and several of the minor ones.  Documenting
the rest of the minor issues would be a big undertaking with a small
payoff.

  * Make Emacs safer by using wider ints when narrower ones might overflow.
    This fixes many bugs.  Here is a partial list.  Unless otherwise
    specified the bug examples are on typical 64-bit hosts.

     - On my Fedora 14 x86-64 host, (signal-process 4294967295 1)
       crashes my entire login session, Emacs included, and leaves my
       workstation in a corrupted state in which the screen
       continually flashes a nonsense pattern and I cannot log in.
       This bug occurs because Emacs incorrectly assumes that fixnums
       fit into pid_t values, which is not true on typical 64-bit hosts.

     - Conversely, Emacs assumes that uid_t, gid_t, pid_t, and file
       descriptor values fit into fixnums, but this is not true for
       many 32-bit hosts.  For example, HP-UX 11i v3 process-IDs can
       go as high as 2**30 - 1.  This problem affects many primitives,
       including call-process and emacs-pid.

     - The following code makes Emacs dump core:

         (progn
	   (setq code-conversion-map-vector 0)
	   (register-code-conversion-map 'x (make-vector 1 1)))

     - (font-get-glyphs FONT-OBJECT FROM TO) goes beserk if TO - FROM
       exceeds 2**31, and if you're lucky it dumps core.

     - A selective-display value of 4294967297 is silently treated as
       if it were 1.  There is a similar problem with x-max-tooltip-size.

     - (char-category-set 4294967296) returns a nonzero value.  It
       should report an error, since the argument is not a character.

     - (char-resolve-modifiers most-negative-fixnum) returns 0; it
       should return its argument.

     - (unibyte-string 4294967296) does not report an out-of-range
       error, as it should.

     - On typical 32-bit hosts, define-charset-internal mishandles some cases
       where the maximum code is greater than 2**31 (which is true for
       some Asian codes).  It calculates the wrong value for maximum
       codes, reports the wrong ranges when signalling an error, in
       some cases does not report values that are out of range, and
       (when --with-wide-int is specified and certain compiler
       optimization flags are used) it rejects some valid codes.

     - (progn (put 'foo 'char-table-extra-slots 4294967296)
       (make-char-table 'foo)) does not signal an out-of-range error,
       as it should.

     - (set-char-table-range CHARSET 4294967296 VALUE) does not report
       an out-of-range error, as it should.

     - (backward-char most-negative-fixnum) incorrectly behaves as if it were
       (backward-char most-positive-fixnum).

     - (unencodable-char-position START END CODING-SYSTEM COUNT "abc")
       incorrectly treats a START or END or COUNT value like
       4294967297 as if it were 1.  There are similar problems with
       the START and END values of (check-coding-systems-region START
       END CODING-SYSTEM-LIST).

     - (find-operation-coding-system 'write-region 1 2) has an
       off-by-one error that causes it to access the garbage that
       is one past its argument array.

     - (define-coding-system-internal ...) mishandles many values
       outside of C 'int' range: typically it silently converts them
       to 'int', resulting in undefined behavior.

     - (string-to-number "10" 4294967298) returns the mathematically
       incorrect value 2 rather than reporting a range error.

     - dbus-call-method omits integer overflow checks in several cases.

     - dbus-call-method-asynchronously mishandles large timeouts.

     - Long time durations (either positive or negative) are
       mishandled.  For example, (sleep-for -2147483649.0) waits for
       2**31 seconds, whereas it should either fail or return right
       away.  There are similar bugs in sit-for, read-char,
       read-event, read-char-exclusive, accept-process-output, and
       hourglass-delay.

     - (signal-process (emacs-pid) 4294967296) should fail, because
       there is no signal 4294967296, but it returns 0, indicating
       success.

     - On typical 64-bit hosts with 32-bit uid_t, (user-login-name
       4294967296) returns "root" whereas it should fail.  There is a
       similar problem with (user-full-name 4294967296).

     - (float-time '(0 0 4294967296)) returns 0.0, which is incorrect.
       It should either report an error for an out-of-range
       microseconds component, or return the mathematically-correct
       result.

     - (encode-time 0 0 0 1 1 1970 4294967296) acts like (encode-time
       0 0 0 1 1 1970 0), ignoring the high order bits of the time
       zone argument.  These high order bits should be passed to the
       underlying C implementation (which may well mishandle them, but
       at least Emacs isn't introducing a bug).

     - insert-file-contents overly trusts the inserted-char counts
       returned by the hooks; they should be sanity checked, to avoid
       the potential for calculating incorrect buffer offsets.

     - concat mishandles some long strings.  It checks for byte count
       overflow in places where it should check for char count overflow,
       and it misses some byte count overflows.

     - (set-mouse-position FRAME 4294967296 4294967296) is silently
       treated as if it were (set-mouse-position FRAME 0 0).  There
       are similar problems with set-mouse-pixel-position,
       set-frame-height, set-frame-width, set-frame-size,
       set-frame-position, modify-frame-parameters.

     - (modify-frame-parameters FRAME ALIST) can overrun the C stack
       if ALIST is long.

     - (open-font FONT-ENTITY 4294967296) is silently treated as if it
       were (open-font FONT-ENTITY 0).

     - (destroy-fringe-bitmap BITMAP) mishandles a bitmap whose fringe
       is 4294967296, by silently treating it as a fringe of 0.  There
       are similar issues with other fringe bitmap operations.

     - (define-fringe-bitmap BITMAP BITS HEIGHT WIDTH) incorrectly
       treats a HEIGHT of -4294967296 as if it were zero, and
       similarly for WIDTH.

     - (fringe-bitmaps-at-pos 4294967296) is silently treated as if it
       were (fringe-bitmaps-at-pos 0).

     - (gnutls-error-fatalp -4294967297) is silently treated as if it
       were (gnutls-error-fatalp -1).

     - Setting tool-bar-button-margin values to 4294967297 is silently
       treated as if they were set to 1.  There is a similar problem with
       tool-bar-border and hscroll-step.

     - (lookup-image ... :relief 4294967297) is silently treated as if
       it was :relief 1.

     - If auto-save-timeout is large, it wraps around with large files
       and behaves as if it were negative, or zero, or small.

     - (define-key KEYMAP KEY DEF) badly mishandles KEY or DEF values
       whose lengths exceed 2**31.  There are similar issues with
       lookup-key.

     - (text-char-description 4294967296) is silently treated as if it
       were (text-char-description 0).

     - The Lisp reader mishandles syntax errors like '(#^^[]), causing
       it to read storage that is out of bounds of an array.  It also
       mishandles '(#^^[4294967297 ...]), causing it to treat the
       large integer as if it were 1.

     - The Lisp printer mishandles hash tables containing more than
       2**31 elements, if print-length exceeds 2**31.

     - (set-process-window-size 4294967296 4294967296) is silently
       treated as (set-process-window-size 0 0).

     - (set-process-datagram-address PROCESS '(4294967296 ...)) silently
       treats the 4294967296 as if it were 0.

     - (set-network-process-option PROCESS OPTION 4294967296) does not
       diagnose the out-of-range integer, for integer options.

     - (internal-describe-syntax-value '(0 . 4294967296)) silently treats
       the 4294967296 as if it were 0.

     - (parse-partial-sexp FROM TO 4294967296) silently treats the
       4294967296 as if it were 0.

     - (set-window-hscroll 4294967296) silently treats the 4294967296
       as if it were 0.

     - (window-line-height 4294967296) silently treats the 4294967296
       as if it were 0.

     - (scroll-left 4294967296) silently treats the 4294967296 as if
       it were 0.  There is a similar problem with scroll-right and
       with hourglass-delay.

     - (dump-glyph-row 4294967297 4294967297) silently treats both
       instances of 4294967297 as if they were 1.  There is a similar
       problem with dump-tool-bar-row.

     - (bitmap-spec-p '(34359738368 34359738368 "x")) incorrectly returns t.
       This can cause further problems in the code that loads bitmaps.

     - (x-change-window-property PROP VALUE FRAME TYPE 4294967304) silently
       treats the 4294967304 as if it were 8.

  * Callers to larger_vector often blindly multiply sizes by 2, which
    can lead to integer overflow with large sizes.  Change
    larger_vector's API to make it easier check for size overflow when
    growing a vector.

  * Make Emacs more efficient when configured on a 32-bit host
    --with-wide-int, by using ptrdiff_t instead of EMACS_INT when
    ptrdiff_t suffices.  This reduces the size of Emacs data
    structures; for example, on one Fedora 14 test, this patch shrank
    the number of pure bytes used by 11%, and shrank the executable
    size (text+data) by 5.3%.

  * Similarly, fix a few other places where a wide int is used even
    though a narrower int suffices.  For example, since the internal
    buffer in Fcall_process can never exceed CALLPROC_BUFFER_SIZE_MAX
    (64 KiB), its size can be stored in an int rather than in an EMACS_INT.

[-- Attachment #2: intwidth.txt.gz --]
[-- Type: application/x-gzip, Size: 161920 bytes --]

bug#9874: Fixes for several integer overflow and width issues

[Andreas Schwab]

Paul Eggert <eggert@cs.ucla.edu> writes:

>      - Conversely, Emacs assumes that uid_t, gid_t, pid_t, and file
>        descriptor values fit into fixnums,

File descriptors are always small numbers.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9874: Fixes for several integer overflow and width issues

[Stefan Monnier]

> Attached is a patch to the Emacs trunk (bzr 106194) to fix several
> issues, mostly due to integer overflow.  This patch was derived by
> looking for and fixing common integer-width issues.  The patch does
> not attempt to fix all these issues, just some issues that are clear
> and for which the fixes are straightforward.

Thank you very much, but we're too far in the release process for such
large patches, so it will have to wait for 24.2.


        Stefan

bug#9874: Fixes for several integer overflow and width issues

[Juanma Barranquero]

> Thank you very much, but we're too far in the release process for such
> large patches, so it will have to wait for 24.2.

These bugs seem serious enough:

    - On my Fedora 14 x86-64 host, (signal-process 4294967295 1)
      crashes my entire login session, Emacs included, and leaves my
      workstation in a corrupted state in which the screen
      continually flashes a nonsense pattern and I cannot log in.
      This bug occurs because Emacs incorrectly assumes that fixnums
      fit into pid_t values, which is not true on typical 64-bit hosts.

    - The following code makes Emacs dump core:

        (progn
          (setq code-conversion-map-vector 0)
          (register-code-conversion-map 'x (make-vector 1 1)))

    - (font-get-glyphs FONT-OBJECT FROM TO) goes beserk if TO - FROM
      exceeds 2**31, and if you're lucky it dumps core.

    - (modify-frame-parameters FRAME ALIST) can overrun the C stack
      if ALIST is long.

    - The Lisp reader mishandles syntax errors like '(#^^[]), causing
      it to read storage that is out of bounds of an array.  It also
      mishandles '(#^^[4294967297 ...]), causing it to treat the
      large integer as if it were 1.

    - insert-file-contents overly trusts the inserted-char counts
      returned by the hooks; they should be sanity checked, to avoid
      the potential for calculating incorrect buffer offsets.

    - concat mishandles some long strings.  It checks for byte count
      overflow in places where it should check for char count overflow,
      and it misses some byte count overflows.

    - (find-operation-coding-system 'write-region 1 2) has an
      off-by-one error that causes it to access the garbage that
      is one past its argument array.

 * Callers to larger_vector often blindly multiply sizes by 2, which
   can lead to integer overflow with large sizes.  Change
   larger_vector's API to make it easier check for size overflow when
   growing a vector.

bug#9874: Fixes for several integer overflow and width issues

[David De La Harpe Golden]

On 26/10/11 15:26, Juanma Barranquero wrote:
>> Thank you very much, but we're too far in the release process for such
>> large patches, so it will have to wait for 24.2.
>
> These bugs seem serious enough:
>
>      - On my Fedora 14 x86-64 host, (signal-process 4294967295 1)
>        crashes my entire login session, Emacs included, and leaves my
>        workstation in a corrupted state in which the screen
>        continually flashes a nonsense pattern and I cannot log in.
>        This bug occurs because Emacs incorrectly assumes that fixnums
>        fit into pid_t values, which is not true on typical 64-bit hosts.
>

Hmm. The really serious/horrible part there (screen manglement and being 
unable to login again) sounds like some X and/or [gkx]dm bug well 
outside emacs' responsibility.  Given the current state of the emacs 
code, it should "just" kill (hup) all the processes you can kill*.  It 
shouldn't be any worse than if you just opened up an xterm and typed 
"kill -HUP -1".

Yes, that probably includes suddenly ending your entire desktop session, 
but unless you're running as a privileged user that should "just" leave 
you staring in annoyance at the [gkx]dm login dialog, there shouldn't be 
any lingering corruption, just like there shouldn't be any lingering 
corruption with a "kill -HUP -1".

Emacs bug that 4294967295 silently/surprisingly becomes -1 in 
signal-process? Well, probably. Emacs bug that your computer's state 
somehow gets corrupted to the point you can't log in again?  No.


*  pid_t 32-bit, 4294967295 => 32-bit -1, and kill(-1,...) signals 
everything you're allowed signal.

bug#9874: Fixes for several integer overflow and width issues

[Stefan Monnier]

>> Thank you very much, but we're too far in the release process for such
>> large patches, so it will have to wait for 24.2.
> These bugs seem serious enough:

I don't claim they're not serious: just too hypothetical to justify
such a large patch at such a late stage,


        Stefan

bug#9874: Fixes for several integer overflow and width issues

[Andreas Schwab]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Thank you very much, but we're too far in the release process for such
>>> large patches, so it will have to wait for 24.2.
>> These bugs seem serious enough:
>
> I don't claim they're not serious: just too hypothetical to justify
> such a large patch at such a late stage,

Especially the EMACS_INT -> ptrdiff_t change does not fix any bug.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9874: Fixes for several integer overflow and width issues

[Paul Eggert]

On 10/26/11 07:09, Stefan Monnier wrote:
> Thank you very much

You're welcome.

> we're too far in the release process for such large patches

OK, I'll hold off until after the release.

To follow up on two of the other comments:

* The problem with (signal-process 4294967295 1) also occurred
  with (signal-process -1 1), so it's true that that spectacular
  failure wasn't all Emacs's fault.  (I have since upgraded to
  Fedora 15 and have been too chicken to try out the test case
  in my new environment....)

* File descriptors are small ints on platforms I regularly use,
  but POSIX doesn't guarantee this assumption.  Removing the assumption
  added two lines of code.  It's not a big deal either way.

bug#9874: Fixes for several integer overflow and width issues

[Andreas Schwab]

Paul Eggert <eggert@cs.ucla.edu> writes:

> * File descriptors are small ints on platforms I regularly use,
>   but POSIX doesn't guarantee this assumption. 

Every function returning a new file descriptor returns the smallest
available one, so if you want to get a very big one you have to have
odles of open files already (unless you force a particular descriptor
with dup2, which Emacs doesn't).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#9874: Fixes for several integer overflow and width issues

[Paul Eggert]

On 10/27/11 01:28, Andreas Schwab wrote:
> if you want to get a very big one you have to have
> odles of open files already

Yes, that's the scenario.  The invoker of Emacs could
open oodles of file descriptors, causing Emacs to
obtain a high file-descriptor number whenever
it opens a file, creates a pipe, etc.

Admittedly unlikely, but easy to defend against.

bug#9874: Fixes for several integer overflow and width issues

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

Since we've now created an emacs-24 branch it seems like a good time
to install this pending patch now.  I'm attaching an updated copy.
It's the same as before, but updated in the light of the Emacs
changes since the patch was created.  It seems large enough
(about 20,000 lines, 653 kB) that I thought it wise to compress it.
The patch is relative to bzr 107822 in the trunk.

[-- Attachment #2: intsmall-patch.txt.gz --]
[-- Type: application/x-gzip, Size: 162641 bytes --]

bug#9874: Fixes for several integer overflow and width issues

[Paul Eggert]

Stefan OKed this patch so I installed this into the trunk
as bzr 108370 and am marking the bug as done.  The idea
is to give Dmitry a chance to rework his proposal for
immediate strings
<http://lists.gnu.org/archive/html/emacs-devel/2012-05/msg00435.html>
in the light of the usually-smaller data structures when
configured --with-wide-int on 32-bit hosts.

This patch affects w32fns.c and w32menu.c in what I
hope are trivial and safe ways, so I'm CC'ing this to
Eli to give him a heads-up too.