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

Previous Next

Full log

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 8545 <at> debbugs.gnu.org
Subject: Re: issues with recent doprnt-related changes
Date: Wed, 27 Apr 2011 22:34:45 +0300

> Date: Mon, 25 Apr 2011 23:02:25 -0700
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 8545 <at> debbugs.gnu.org
> 
> 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.

I added more checks, thanks for pointing this out.

> "%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.

You convinced me, so I removed %l.

> >> * 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

I see no reason for such complexity, just to avoid SAFE_ALLOCA.  But
feel free to make this change, if you think it's important enough.

> >>   - 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.

??? %-10s means to print a string LEFT-justified, and the code handles
that in this loop (which runs after the string was copied to its
destination):

	      if (minlen < 0)
		{
		  while (minlen < - width && bufsize > 0)
		    {
		      *bufptr++ = ' ';
		      bufsize--;
		      minlen++;
		    }
		  minlen = 0;
		}

I actually tried using %-30s, and it did work correctly (as did %30s).

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

The code doesn't call atoi for numeric arguments.  It delegates that
case to sprintf, which will handle the likes of %0-3d correctly.  And
for %s and %c the "0" flag is not supported anyway (as stated in the
comments) and GCC flags that with a warning.  So I see no problem
here.

> 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.  verror will reject SIZE_MAX-long messages
anyway, so I see no reason to squeeze one more byte here just to throw
it away there.

>   /* Limit the string to sizes that both Emacs and size_t can represent.  */
>   size_t size_max = min (MOST_POSITIVE_FIXNUM + 1, SIZE_MAX);

"MOST_POSITIVE_FIXNUM + 1" is too much, since MOST_POSITIVE_FIXNUM
should be able to cover the terminating null character in Emacs.  So I
used this:

   size_t size_max = min (MOST_POSITIVE_FIXNUM, SIZE_MAX);

> Thanks, can you make a similar change inside doprint?  It
> also uses xrealloc where xfree+xmalloc would be better.

Done.

> One other thing, the documentation says that lower-case l
> is a flag, but it's a length modifer and not a flag.

I fixed the doc on that account.

Previous Next

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