Re: [Groff] Re: groff 1.17: pre-grohtml's unsafe temporary file handling & other improvements

[solar]

On Tue, Jun 12, 2001 at 06:17:26PM +0100, Gaius Mulley wrote:
> 
> Many thanks to all who have suggested improvements to pre-grohtml.
> I've reworked several buffer issues within pre-grohtml in light of
> the overflow problem highlighted by several people. Here is the
> latest patch:

It really looks better, thanks!

> +    n = vsnprintf (p, size, fmt, ap);

I'm afraid, groff is meant to be portable to systems which either
don't have vsnprintf() or have versions written prior to when the
return value semantics were discussed, let alone standardized.

I think other parts of groff would benefit from the ability to use
*snprintf(), so it may be a good idea to include an implementation
with groff itself for use on systems which lack these functions.

> +    /* If that worked, return the string. */
> +    if (n > -1 && n < size)
> +      return p;
> +    /* Else try again with more space. */
> +    if (n > -1)    /* glibc 2.1 */
> +      size = n+1; /* precisely what is needed */

This is how *snprintf() should work.

> +    else           /* glibc 2.0 */

There's a third possibility: implementations which only return the
number of characters actually written.  SUSv2 is not very explicit
about the return value:

RETURN VALUE

    Upon successful completion, these functions return the number of
    bytes transmitted excluding the terminating null in the case of
    sprintf() or snprintf() or a negative value if an output error was
    encountered.

    If the value of n is zero on a call to snprintf(), an unspecified
    value less than 1 is returned.

It is non-obvious whether a too small value of n qualifies as an
output error.

Clearly, this specification was written prior to the public discussions
where Theo de Raadt and others were advocating the semantics now seen
with glibc 2.1+.

A simple way to cover this third possibility is to require that:

        n > -1 && n < size - 1

and increase the allocation size if all of the buffer was used.

> +      size *= 2;  /* twice the old size */
> +    if ((p = (char *)realloc (p, size)) == NULL)
> +      return NULL;

This is a non-issue for pre-grohtml, but this use of realloc() would
leak memory on failure.  Probably worth adding a comment, or use a
temporary variable to avoid the leak.

For the size *= 2 case, you could add another realloc() to decrease
the allocation size to what was actually needed.

-- 
/sd