Subject: Fixes for several integer overflow and width issues
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Wed, 26 Oct 2011 00:51:05 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.