GNU bug report logs - #8545
issues with recent doprnt-related changes

Previous Next

Full log

Message #70 received at 8545-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 8545-done <at> debbugs.gnu.org
Subject: Re: issues with recent doprnt-related changes
Date: Thu, 28 Apr 2011 15:11:11 -0700

> If that's what you mean, it's easy to fix.  Done.

If I understand things correctly, that fix (in bzr 104036) handles the
case where the format itself has a non-ASCII character that is
truncated, but it doesn't handle the case where the format is
something like "file name = %s", and %s expands to a long file name
that is truncated.  If so, surely that case still needs to be fixed.


>>>> * verror might invoke doprnt two or more times, which means that
>>>>   doprnt will traverse ap twice.  This does not work in general; the C
>>>>   standard is quite clear that the behavior is undefined in this case.
>>>
>>> Are there any platforms supported by Emacs where this actually
>>> happens?
>>
>> Yes, absolutely.  The bug happens on the platform I normally develop
>> Emacs on, namely RHEL 5.6 (x86-64).
>
> What is the implementation of va_start on that platform?

Sorry, I don't know.

> Is stashing away the ap argument all that's required here?

Yes, portable code is supposed to use va_copy.  Code that traverses
through an argument list N times can call va_start once, va_copy N - 1
times, and va_end N times (once on the original, once on each copy).

va_copy is a C99-ism, but we can use it as-is in Emacs source code,
and use the relevant gnulib module for obsolete platforms that lack it.
Do the DOS and NT ports have va_copy?  If not, it should be simple
to supply a substitute.


> You don't need me to reopen a bug, and you don't need an open bug
> report to work on some code, if you feel like it.

OK, thanks, I'll see if I can reopen the bug and install a fix or two.


>>>>  And why is there another test "n * 10 > SIZE_MAX - (fmt[1] -
>>>> '0')" that always returns 0, no matter what?
>>>
>>> ??? What happens if n*10 is SIZE_MAX-1 and fmt[1] is '2'?
>>
>> n * 10 cannot possibly be SIZE_MAX - 1, because at that
>> point n < SIZE_MAX / 10, which means n * 10 is at most
>> SIZE_MAX - SIZE_MAX % 10 - 10.  fmt[1] is an ASCII digit,
>> so n * 10 therefore cannot possibly be greater than
>> SIZE_MAX - (fmt[1] - '0').  Hence the expression
>> n * 10 > SIZE_MAX - (fmt[1] - '0') always returns zero.
>
> So now you suddenly realize that the ">=" in the first part of that if
> clause was a good idea after all, yes?

I already indicated that I understand how that first part works, in
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8545#26>.  But with the
">=" in place, the second part of that 'if' clause always returns 0:
that's unnecessarily confusing, since it misleads the reader into
thinking that the second part is needed, when it's not.

In other words, if "digit" is in the range 0 through 9, then this:

   if (n > SIZE_MAX / 10 || n * 10 > SIZE_MAX - digit) error (...);

is exact but complicated, and this:

   if (n >= SIZE_MAX / 10) error (...);

is simple but slightly conservative (it reports overflow for a few
very large but not overflowed numbers, but that's good enough here).
Finally, this:

   if (n >= SIZE_MAX / 10 || n * 10 > SIZE_MAX - digit) error (...);

is is neither exact nor simple, and is more confusing than either of
the above; there's nothing to recommend it over the other two.

Also, as I mentioned earlier, the code should also check for 'int'
overflow, as most sprintf implementations mess up with widths or
precisions greater than INT_MAX.  I'll check in a fix for that, with a
comment that tries to explain the situation and responds to your other
remarks about clarity here; please feel free to improve it.

Another possibility is to remove the 'if' test entirely, making it the
caller's responsibility to not specify outlandish widths in format
strings.  That would make the code simpler yet.  The other overflow
checks in doprnt would have to stay, since they depend on user data,
but this overflow check is superfluous.


> The issue is not whether it includes the null or not.  The issue is
> the range of values that an EMACS_INT data type must be able to
> represent, and the relation of that range to the maximum number of
> bytes in a string and to the largest possible string byte position
> that Emacs is able to handle.

OK, thanks.  I read the code, and if I understand it correctly, since
'point' is 1-origin, a buffer with MOST_POSITIVE_FIXNUM characters
will have values of 'point' ranging from 1 through
MOST_POSITIVE_FIXNUM + 1, but that "+ 1" would mean Fpoint wouldn't
work: so we should limit buffers to contain at most
MOST_POSITIVE_FIXNUM - 1 bytes.

Is it also the case that Emacs should limit strings to at most
MOST_POSITIVE_FIXNUM - 1 bytes?  Sorry, I couldn't tell this from the
functions you mentioned; there's a lot of code there, and this stuff
isn't immediately obvious.

If so, then should this doprnt code:

              tem = strlen (string);
              if (tem > MOST_POSITIVE_FIXNUM)
                error ("Format width or precision too large");
 
use ">=" rather than ">"?  There are two instances of this
sort of thing.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.