[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#8545: issues with recent doprnt-related changes
From: |
Paul Eggert |
Subject: |
bug#8545: issues with recent doprnt-related changes |
Date: |
Mon, 25 Apr 2011 23:02:25 -0700 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 |
On 04/25/11 02:00, Eli Zaretskii wrote:
>> * Format strings never include embedded null bytes, so there's
>> no need for doprnt to support that.
>
> Potentially, someone could call `error' with its first argument taken
> from a Lisp string, which could include null characters. But again,
> this feature was there to begin with, and I see no particular need to
> remove it.
The feature is buggy, because the code does not check
fmt versus fmt_end every time it increases fmt; it checks
only sometimes. Hence one can construct examples where
doprnt will overrun the format, e.g., by having '%l' at the
end of the format.
If doprnt were written to expect a null-terminated string, which
is what all its callers pass anyway, it would be simpler and
easier to maintain and would not have this problem.
"%l" is a strange case anyway, since one cannot reliably use
"%l" as an alias for "%d". For example, the format "%dx" prints
an integer followed by an 'x', but if you try to use "%lx" instead,
it doesn't work. At least, we should remove "%l" as a format
specifier, as it's a rightly-unused feature and it's just asking
for trouble to try to support it. This should also fix the
format-overrun bug mentioned earlier.
>> * If the format string is too long, the alloca inside doprnt will
>> crash Emacs on some hosts.
>
> You are right. I modified doprnt to use SAFE_ALLOCA instead.
There's no need for alloca or SAFE_ALLOCA or xmalloc or any
dynamic allocator. Instead, convert any width and precision
values to integers, and use "*". For example, if the caller
specifies this:
"%012345.6789g", 3.14
pass this to sprintf:
"%0*.*g", 12345, 6789, 3.14
That way, the format string itself has easily-bounded size and
the code never needs to use alloca or xmalloc or whatever.
> Since both error and verror are now marked as ATTRIBUTE_FORMAT_PRINTF,
> the compiler will detect such invalid formats and flag them.
Sure, but in that case why maintain code to implement the two
formats (%S and %l) that are flagged invalid and are never used and
should never be used?
>> - doprnt uses atoi (&fmtcpy[1]), but surely this isn't right if
>> there are flags such as '-'.
>
> Why not? In that case, atoi will produce a negative value for
> `width', which is already handled by the code. If I'm missing
> something, please point out the specific problems with that.
I don't see how the negative value is handled correctly.
%-10s means to print a string right-justified, but the code
surely treats it as if it were %0s. And other flags
are possible, e.g., atoi will parse "%0-3d" as if the
width were zero, but the width is 3 (the "0" is a flag).
A quick second scan found a minor bug in size parsing: the
expression "n >= SIZE_MAX / 10" should be "n > SIZE_MAX / 10".
> The reason I originally limited to `size_max - 1' is that the games
> you play with the maximum size, viz.:
>
> size_t size_max =
> min (MOST_POSITIVE_FIXNUM, min (INT_MAX, SIZE_MAX - 1)) + 1;
>
> are neither clear nor commented. E.g., why the second `min'? could
> INT_MAX be ever larger than SIZE_MAX-1? if so, what does that mean in
> terms of relation between `int' and `size_t' on such a platform?
The C Standard allows INT_MAX to be larger than SIZE_MAX - 1, yes.
I don't know of any current targets with that property, but it didn't
hurt to be safe. The second 'min' was needed because vsnprintf
can't create a string longer than INT_MAX bytes. Since doprnt doesn't
have that silly limit, the above line should be changed to something
like the following (this time with a comment :-):
/* Limit the string to sizes that both Emacs and size_t can represent. */
size_t size_max = min (MOST_POSITIVE_FIXNUM + 1, SIZE_MAX);
> You are right, I made that change.
Thanks, can you make a similar change inside doprint? It
also uses xrealloc where xfree+xmalloc would be better.
One other thing, the documentation says that lower-case l
is a flag, but it's a length modifer and not a flag. It
must be specified after the precision (if given) and
immediately before the conversion specifier character,
and it cannot be intermixed with flags like 0 and - and +.
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/25
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/25
- bug#8545: issues with recent doprnt-related changes,
Paul Eggert <=
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Juanma Barranquero, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/27
- bug#8545: issues with recent doprnt-related changes, Juanma Barranquero, 2011/04/28
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/28
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/28
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/28
- bug#8545: issues with recent doprnt-related changes, Eli Zaretskii, 2011/04/28
- bug#8545: issues with recent doprnt-related changes, Paul Eggert, 2011/04/28