bug#9196: integer and memory overflow issues (e.g., cut-and-paste crashes Emacs)

[Paul Eggert]

First, thanks for taking a look at the patch.  And in response to your
comments ...

On 07/29/11 03:01, Jan Djärv wrote:

> a crash would be fine by me.  Actually better than memory_full,
> because the core is much more useful.

We can easily arrange to crash, by replacing "memory_full (SIZE_MAX)"
with "abort ()".  I can do that for places where that's preferred.  It
sounds like xgselect.c is one of those places, so I'll do that; please
let me know of any other places.

> Since strlen is defined to return size_t and you store the result in
> a ptrdiff_t, does not that mean you have introduced a possible
> signed/unsigned conversion error?

It would, if Emacs allowed objects with size greater than PTRDIFF_MAX.
But Emacs doesn't, for two reasons.  First, such objects would cause
problems with C code, because pointer subtraction doesn't work within
these objects, and Emacs uses pointer subtraction heavily.  Second,
the Emacs internal coding style prefers to avoid unsigned integers
like size_t unless absolutely necessary (see
<http://lists.gnu.org/archive/html/emacs-devel/2011-04/msg00514.html>).
This is partly due to the confusion when comparing signed-to-unsigned.
Also, it's nice to have the readily-available option of using hardware
overflow-checking for signed integer overflow.

Perhaps this should be documented somewhere ...

> +      ptrdiff_t lim = min (TYPE_MAXIMUM (Window),
> +               min (PTRDIFF_MAX, SIZE_MAX) / sizeof (GtkWidget *));
> 
> Isn't this a compile time constant?  Should it not be a #define or something?

It could be a #define, but it's needed only at that one location,
and in that situation ordinary identifiers are clearer,
less error-prone, and better for debugging than macros are.

> IMHO, the check in gtkutil.c will only call memory_full when there
> is 2^31 (about 2 billion) scroll bars in Emacs.  Isn't it
> overengineering to check for that case?

I systematically looked for all places where memory is being
allocated, and where size calculations might overflow during this, and
fixed them all.  It's better to have a systematic policy where all
such size calculations are checked.  Trying to decide heuristically
which size overflows don't need checking because they can "never
happen" would lead to further sources of maintenance error, and anyway
the idea that some size overflows can "never happen" is uncomfortably
close to the apocryphal story that "640K of memory should be enough
for anybody".

> In xgselect.c:
> 
> +    int gfds_size_max =
> +      min (INT_MAX, min (PTRDIFF_MAX, SIZE_MAX) / sizeof *gfds);
> 
> Here a compile time constant is recalculated inside a loop.

Since it's a constant, the runtime cost of calculating it is zero, so
there's no efficiency gain by moving it.  Do you think it would be
clearer to hoist it out of the loop?  If so, we can easily do that;
but there is something to be said for having the definition of the
constant near its use.

> The xgselect.c is also overengineering IMHO.  The number checked
> represents the number of file descriptor sources Glib is checking.
> I can understand checking sizes for strings that come from external
> sources, but only code adds file descriptor sources.  If some bug
> causes the addition of 2 billion sources

2 billion sources is not always the limit.  On typical 32-bit hosts,
the current code stops working at 500 million sources, or 250 million
if one is worried about pointer subtraction.  And if future glib
versions change the associated struct, that 250-million limit could go
down further.  In cases like these, it's helpful to check the limit
even if the check can "never fail".