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

[Paul Eggert <eggert@cs.ucla.edu>]

On 07/29/11 09:49, Jan Djärv wrote:

>>> +    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.
> 
> I think you are assuming compiler optimizations here.  They might be
> true for gcc, but not for other compilers.  A define more clearly
> states that it is a constant than a computed variable.

Sorry, I don't follow this.  A 'define' wouldn't decrease the runtime
cost, as a non-optimizing compiler would evaluate the expansion of the
'define' at runtime.

Anyway, to address this issue I'll change it to an enum, which is
guaranteed to be calculated at compile-time and clearly marks it as
a constant.  And I'll hoist it out of the function entirely.
Something like this should do the trick:

  static GPollFD *gfds;
  static int gfds_size;
  enum { GFDS_SIZE_MAX =
	   min (INT_MAX, min (PTRDIFF_MAX, SIZE_MAX) / sizeof *gfds) };


> The problem with abort is that gcc may optimize them to look
> like they occurred elsewhere.

abort () is the standard way to trap within Emacs and other GNU
programs.  If there are problems with debugging calls to abort (),
then we should address those problems, but surely that's a separate
issue.

Anyway, it's much easier to debug an abort () than a memory corruption.


> It is also a maintenance burden to keep all checks....
> Please come up with a real scenario when this may actually fail
> before adding checks.

The scenario is when Emacs has more than about 250 million event
sources.  Admittedly this is quite unlikely now, but it may happen in
the future, and the fix is easy now.  Why not make sure Emacs is
robust?  We're talking about only two lines of code here:

  if (GFDS_SIZE_MAX / 2 < n_gfds)
    memory_full (SIZE_MAX);

which take only 2 instructions in the typical case.  Surely this is
not too high a price to pay for reliability.

If it's the code clutter that's the major objection here, we can use
an inline function to shorten it to just one line, like this:

  check_size (n_gfds <= GFDS_SIZE_MAX / 2);

This would have the same run-time cost (2 instructions), but would be
easier to read.  How about that idea?

> You aren't the one that has to take the time
> to update these checks when the code changes.

If the code changes in such a way that these checks need to change,
then the presence of the checks is a good thing.  The checks will help
remind programmers of the limits involved, and will help them avoid
memory corruption in the future.  Currently these issues are only in
programmers' heads and are too often forgotten, and this can easily
lead to bugs.

Also, it's not really true that I won't be the one that has to take
the time.  I have been taking the time to maintain and improve these
checks for months now.  I've found several serious bugs in the
process, some of which allow remote exploits.  I expect to find more
bugs, and I'll be happy to help in any future problems that crop up
in this area.  The goal is to have an Emacs implementation that is robust,
rather than one that crashes when given input that was thought
"couldn't happen".