Re: [PATCH v7] Implement strlcpy, strlcat [BZ #178]

[Alexander Cherepanov <ch3root at openwall dot com>]

On 2016-01-04 23:55, Florian Weimer wrote:
> > > One suggestion is to make a non-terminated buffer undefined, but that
> > > breaks
> > > the snprintf analogy for size 0 inputs.

What happens in this case is more or less documented but "in practice 
this should not happen as it means that either dstsize is incorrect or 
that dst is not a proper string"[1] so it's not clear if glibc should 
support it officially. Making it UB seems to be a valid choice.

[1] 
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strlcat.3?query=strlcpy&sec=3#x4445534352495054494f4e

> > Sorry, what analogy is that? snprintf does not concatenate to a buffer
> > directly. What is the practical use case here? How does the use case
> > ignore the principle that strlcpy should null-terminate its output?
>
> We can probably ditch the size-0 documented special case for strlcat
> (where it is just extremely confusing and not very helpful),

Yeah, this is strange. The full analogy with snprintf is documented for 
strlcpy only but I don't think it's implied for strlcat.

Plus, the implementation[2] in openbsd (the most upstream IIUC) does 
pointer arithmetic on the destination pointer which is UB for NULL. I 
guess this means that dst=NULL is not supposed to be supported.

[2] 
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/string/strlcat.c?rev=1.16&content-type=text/x-cvsweb-markup

> but not for
> strlcpy, where it is part of the specification.
>
> Apart from that, my only objection to your strlcpy text is this:
>
> +The behavior of @code{strlcpy} is undefined if @var{size} is zero, or
> +if the source and destination strings overlap.
>
> I think it should say âif the source string and destination array
> overlapâ.   There is no destination string before the call.  I don't
> think it's necessary to make the special case defined where the source
> string is part of the destination array, but not located close to the
> beginning so that the actual copy will not overlap.  This is simply too
> subtle.   I think it is undefined for snprintf (âIf copying takes place
> between objects that overlap, the behavior is undefined.â),although
> this does not necessarily follow from the restrict qualify (based on my
> somewhat shaky understanding of restrict).

IIUC you are saying that this code:

  char s[10];
  snprintf(s, -1, "%s", "abc");

is invalid. IMHO this is wrong. The description of snprintf in C11 
doesn't mention size of the destination array or talk about any 
connection between n and s at all, it just says that characters beyond 
the (n-1)st are discarded.

I think both strlcpy and strlcat should work just fine with size=-1. 
This is not a subtle point.

> I would like to make a similar change to your strlcat text:
>
> +The behavior is undefined if @var{to} does not contain a null byte in
> +its first @var{size} bytes, or if the source and resulting destination
> +strings overlap.
>
> There, it does make sense to speak of a string, but supporting the
> special case where the source overlaps with the destination array, but
> not the part that is written, is again very subtle and difficult to tell
> apart from the regular overlap.

Yeah, something like this:

  char *s = "abc";
  strlcat(s, s, 2);

:-)

--
Alexander Cherepanov