--- Begin Message ---
|
Subject: |
issues with recent doprnt-related changes |
|
Date: |
Sun, 24 Apr 2011 22:46:33 -0700 |
|
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.14) Gecko/20110223 Thunderbird/3.1.8 |
This is a followup to Bug#8435. Eli invited me to review the recent
doprnt-related changes, so here's a quick review:
* doprnt returns size_t. But Stefan wrote that he prefers sizes to be
signed values, and doprnt always returns a value that can fit in
EMACS_INT. So shouldn't doprnt return EMACS_INT, as it did before?
* doprnt supports only a small subset of the standard printf formats,
but this subset is not documented. It's unclear what the subset is.
Or it's a superset of the subset, with %S and %l? Anyway, this
should be documented clearly in the lead comment.
* I suggest that every feature in doprnt be a feature that is actually
needed and used; this will simplify maintainance.
* Format strings never include embedded null bytes, so there's
no need for doprnt to support that.
* If the format string is too long, the alloca inside doprnt will
crash Emacs on some hosts. I suggest removing the alloca,
instituting a fixed size limit on format specifiers, and documenting
that limit. Since user code cannot ever supply one of these
formats, that should be good enough.
* The width features of doprnt (e.g., %25s) are never used. That part
of the code is still buggy; please see some comments below. I
suggest removing it entirely; this will simplify things. But if not:
- doprnt mishandles format specifications such as %0.0.0d.
It passes them off to printf, and this results in undefined
behavior, near as I can tell.
- doprnt uses atoi (&fmtcpy[1]), but surely this isn't right if
there are flags such as '-'.
- Quite possibly there are other problems in this area, but I
didn't want to spend further time reviewing a never-used feature.
* In this code, in verror:
used = doprnt (buffer, size, m, m + mlen, ap);
/* Note: the -1 below is because `doprnt' returns the number of bytes
excluding the terminating null byte, and it always terminates with a
null byte, even when producing a truncated message. */
if (used < size - 1)
break;
I don't see the reason for the "- 1". If you replace this with:
used = doprnt (buffer, size, m, m + mlen, ap);
if (used < size)
break;
the code should still work, because, when used < size, the buffer
should be properly null-terminated. If it isn't then there's something
wrong with doprnt, no?
* In this code, in verror:
else if (size < size_max - 1)
size = size_max - 1;
there's no need for the "- 1"s. Just use this:
else if (size < size_max)
size = size_max;
* This code in verror:
if (buffer == buf)
buffer = (char *) xmalloc (size);
else
buffer = (char *) xrealloc (buffer, size);
uses xrealloc, which is unnecessarily expensive, as it may copy the
buffer's contents even though they are entirely garbage here. Use
this instead, to avoid the useless copy:
if (buffer != buf)
xfree (buffer);
buffer = (char *) xmalloc (size);
--- End Message ---
--- Begin Message ---
|
Subject: |
Re: issues with recent doprnt-related changes |
|
Date: |
Thu, 28 Apr 2011 01:50:53 -0400 |
> Date: Wed, 27 Apr 2011 16:51:06 -0700
> From: Paul Eggert <address@hidden>
> CC: address@hidden
>
> >> A quick second scan found a minor bug in size parsing: the
> >> expression "n >= SIZE_MAX / 10" should be "n > SIZE_MAX / 10".
> >
> > When they get to messages as long as SIZE_MAX, let them sue me for
> > taking away one byte.
>
> It's not a question of saving space at run-time. It's a question of
> helping the reader. The reader is left wondering: why is that ">="
> there?
The reader will be wondering with ">" as well. There's a comment
about checking for overflow which should be a good hint, especially
since SIZE_MAX is compared against.
> 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'? Is the
result still zero?
> /* Avoid int overflow, because many sprintfs seriously mess up
> with widths or precisions greater than INT_MAX. Avoid size_t
> overflow, since our counters use size_t. This test is slightly
> conservative, for speed and simplicity. */
> if (n >= min (INT_MAX, SIZE_MAX) / 10)
> error ("Format width or precision too large");
Sorry, I don't see how this is clearer. The current code after the
test is built out of the same building blocks as the test, and
therefore the intent and the details of the test are easier to
understand than with your variant, which perhaps is mathematically and
numerically equivalent, but makes the code reading _harder_ because it
severs the syntactical connection between the two.
> > "MOST_POSITIVE_FIXNUM + 1" is too much, since MOST_POSITIVE_FIXNUM
> > should be able to cover the terminating null character in Emacs.
>
> Why? Emacs size fields count the bytes in the string, and does not
> count the terminating null byte (which is not part of the string).
That's not what I know. String positions are zero-based and extend to
include the terminating null character. See the relevant parts of the
display engine code.
> * doprnt invokes strlen to find the length of the format. The
> vsnprintf code didn't need to do that: it traversed the format once.
> Surely it shouldn't be hard to change doprnt so that it traverses
> the format once rather than twice.
doprnt is invoked in the context of displaying an error message that
throws to top level, and so it doesn't need to be optimized (which
will surely make the code more complex and error-prone, and its
maintenance harder).
> * Sometimes verror will incorrectly truncate a string, even when there
> is plenty of memory. verror might call doprnt (buffer, SIZE, m, m +
> mlen, ap), and doprnt might discover that a multibyte character is
> chopped in half at the end of the output buffer, and might return
> (say) SIZE - 2. verror will incorrectly conclude that the output
> was just fine, but it wasn't complete.
Not an issue, what with the initial buffer size you enlarged to 4000.
I needed to artificially lower it to just 2 dozen bytes, just to see
the recovery code in action. If someone wants to display a 4001-byte
message that ends with a multibyte non-ASCII character, let them be
punished for not knowing how to write concisely.
> * 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? If not, let's forget about this issue until it hits us.
I'm closing this bug. We are already well past any real problems, and
invested too much energy and efforts of two busy people on this tiny
function, all because of your stubborn insistence on using a library
function where it doesn't fit the bill. I hope you now have more
respect for views and code of others in general, and mine in
particular, so we won't need to go through this painful experience
again in the future.
Let's move on; I still need to work on the bidirectional display of
overlay strings and display properties, a job that was already delayed
by several precious days due to these disputes and the gratuitous work
on the code that should have been left alone in the first place.
--- End Message ---