From: Paul Eggert <eggert@cs.ucla.edu>
To: 8545@debbugs.gnu.org
Subject: bug#8545: issues with recent doprnt-related changes
Date: Sun, 24 Apr 2011 22:46:33 -0700 [thread overview]
Message-ID: <4DB50AB9.6060100@cs.ucla.edu> (raw)
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);
next reply other threads:[~2011-04-25 5:46 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-25 5:46 Paul Eggert [this message]
2011-04-25 9:00 ` bug#8545: issues with recent doprnt-related changes Eli Zaretskii
2011-04-25 13:37 ` Stefan Monnier
2011-04-26 20:25 ` Paul Eggert
2011-04-27 1:14 ` Stefan Monnier
2011-04-26 6:02 ` Paul Eggert
2011-04-27 19:34 ` Eli Zaretskii
2011-04-27 23:51 ` Paul Eggert
2011-04-28 1:32 ` Juanma Barranquero
2011-04-28 3:11 ` Paul Eggert
2011-04-28 3:42 ` Juanma Barranquero
2011-04-28 5:06 ` Paul Eggert
2011-04-28 5:15 ` Eli Zaretskii
2011-04-28 5:29 ` Paul Eggert
2011-04-28 6:10 ` Eli Zaretskii
2011-04-28 6:42 ` Paul Eggert
2011-04-28 7:26 ` Eli Zaretskii
2011-04-28 7:54 ` Paul Eggert
2011-04-28 11:14 ` Eli Zaretskii
2011-04-29 12:28 ` Richard Stallman
2011-04-29 19:56 ` Eli Zaretskii
2011-04-29 23:49 ` Paul Eggert
2011-04-30 21:03 ` Richard Stallman
2011-05-01 5:41 ` Paul Eggert
2011-05-01 23:59 ` Richard Stallman
2011-05-02 0:23 ` Paul Eggert
[not found] ` <E1QH37h-0001yM-HR@fencepost.gnu.org>
2011-05-03 20:24 ` Paul Eggert
2011-05-01 4:25 ` Jason Rumney
2011-05-01 5:56 ` Paul Eggert
2011-05-01 8:12 ` Jason Rumney
2011-05-01 11:02 ` Andreas Schwab
2011-04-28 5:02 ` Eli Zaretskii
2011-04-28 5:50 ` Eli Zaretskii
[not found] ` <4DB9146D.2040702@cs.ucla.edu>
[not found] ` <E1QFQVO-0004Dq-6o@fencepost.gnu.org>
[not found] ` <4DB9E5FF.9020506@cs.ucla.edu>
2011-04-29 11:16 ` Eli Zaretskii
2011-04-29 14:41 ` Paul Eggert
2011-04-29 19:35 ` Eli Zaretskii
2011-04-29 20:32 ` Paul Eggert
2011-04-30 8:59 ` Eli Zaretskii
2011-05-04 7:28 ` Paul Eggert
2011-05-04 9:52 ` Eli Zaretskii
2011-05-04 14:56 ` Paul Eggert
2011-05-04 14:56 ` Paul Eggert
2011-05-05 20:36 ` Eli Zaretskii
2011-05-06 13:33 ` Stefan Monnier
2011-05-06 14:41 ` bug#8545: " Paul Eggert
2011-05-06 14:41 ` Paul Eggert
2011-05-06 15:03 ` bug#8545: " Eli Zaretskii
2011-05-06 15:03 ` Eli Zaretskii
2011-05-06 17:13 ` bug#8545: " Stefan Monnier
2011-05-06 17:13 ` Stefan Monnier
2011-05-06 19:57 ` bug#8545: " Eli Zaretskii
2011-05-06 19:57 ` Eli Zaretskii
2011-05-07 3:18 ` Stefan Monnier
2011-05-07 7:55 ` Eli Zaretskii
2011-05-07 7:55 ` bug#8545: " Eli Zaretskii
2011-05-07 3:18 ` Stefan Monnier
2011-05-06 13:33 ` Stefan Monnier
2011-05-05 20:36 ` Eli Zaretskii
-- strict thread matches above, loose matches on Subject: below --
2011-05-01 18:19 bug#8601: * 2 -> * 4 typo fix in detect_coding_charset Paul Eggert
2011-05-01 19:06 ` Andreas Schwab
2011-05-01 19:25 ` Paul Eggert
2011-05-06 7:29 ` bug#8601: Merged fixes for 8600, 8601, 8602, and (partially) for 8545 Paul Eggert
2020-09-14 12:37 ` bug#8545: " Lars Ingebrigtsen
2020-09-14 18:41 ` Eli Zaretskii
2020-09-16 2:01 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DB50AB9.6060100@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=8545@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.