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

Previous Next

Full log

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: issues with recent doprnt-related changes
Date: Sun, 24 Apr 2011 22:46:33 -0700

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);

Previous Next

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