[v7] Implement strlcpy, strlcat [BZ #178]

  On 12/30/2015 09:31 AM, Paul Eggert wrote:

>> One suggestion is to make a non-terminated buffer undefined, but that
>> breaks
>> the snprintf analogy for size 0 inputs.
> 
> 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), 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).

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.

I'm attaching a new version with the proposed manual edits, on top of
your version.

Has anyone reviewed the current implementation and has comments about that?

Thanks,
Florian
  

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

:-)
  

On 01/04/2016 04:02 PM, Alexander Cherepanov wrote:
> 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,

True, but the POSIX description for snprintf (s, n, ...) has the 
additional restriction that n must be the size of the buffer referred to 
by s, so POSIX implementations of snprintf can assume that s's size is n 
and that the destination array does not overlap any of the sources, 
regardless of which parts of the destination are written to.
  

On 2016-01-05 03:52, Paul Eggert wrote:
> On 01/04/2016 04:02 PM, Alexander Cherepanov wrote:
>> 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,
>
> True, but the POSIX description for snprintf (s, n, ...) has the
> additional restriction that n must be the size of the buffer referred to
> by s, so POSIX implementations of snprintf can assume that s's size is n
> and that the destination array does not overlap any of the sources,
> regardless of which parts of the destination are written to.

Are you seriously arguing for glibc not to comply with C11? I probably 
miss something about glibc...

OTOH [1] states (Excluding dprintf()): "The functionality described on 
this reference page is aligned with the ISO C standard. Any conflict 
between the requirements described here and the ISO C standard is 
unintentional. This volume of POSIX.1-2008 defers to the ISO C standard."

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html

So, you just found a bug in POSIX. Cool!

Then, descriptions (e.g. [2], [3]) of strlcpy and strlcat refer to C9X 
or BSD, not to POSIX. So it's not relevant to the current discussion.

[2] https://www.sudo.ws/todd/papers/strlcpy.html
[3] 
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strlcat.3?query=strlcpy&sec=3

Finally, when the standard talks about objects that copying takes place 
between, it doesn't necessarily mean whole original objects. In most 
cases their sizes are not known at all. Well, do you think the fact that 
@var{to} and @var{from} overlap makes the code:

   char s[10] = "abc";
   strncat(s, s, 2);

is invalid even though there is no overlapping copying?
  

Alexander Cherepanov wrote:
> Are you seriously arguing for glibc not to comply with C11?

Naaah. And on second thought, my comment was based on a backwards interpretation 
of the POSIX spec: that spec places extra constraints on the implementation not 
on the application. So please forget my mistaken comment about POSIX snprintf.

> descriptions (e.g. [2], [3]) of strlcpy and strlcat refer to C9X or BSD,
> not to POSIX. So it's not relevant to the current discussion.

Yes and no. I think Florian is arguing that strlcpy should act like snprintf, 
and so in that sense it's relevant. However, I agree with you that snprintf is 
not an exact analog and is not a good precedent for strlcpy.

> do you think the fact that @var{to} and @var{from}
> overlap makes the code:
>
>    char s[10] = "abc";
>    strncat(s, s, 2);
>
> is invalid even though there is no overlapping copying?

I don't think strncat is an exact analog for strlcat either. That is, although I 
agree that the strncat example has well-defined behavior in C11, this is not 
precedent for strlcat, any more than snprintf would be. They're just different 
functions, that's all.

For example, it'd be reasonable for strlcpy to verify that every byte in the 
destination array is writeable. That could help catch bugs in programs. The spec 
should not disallow such an implementation.
  

On 01/05/2016 01:02 AM, Alexander Cherepanov wrote:

>> +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:

No, my point was that this code:

  char s[10];
  strcpy (s + 5, "abc");
  snprintf (s, sizeof (s), "%s", s + 5);

is invalid.  This depends on what “objects” means in “If copying takes
place between objects that overlap, the behavior is undefined.”

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

POSIX, on the other hand, speaks of the “n argument which states the
size of the buffer referred to by s”.  POSIX also requires that the -1
argument results in -1/EOVERFLOW result, but this does not apply to
strlcpy because it has a size_t (not int) result.

I'm not sure what the intent behind these specifications is.  I would
certainly prefer that size arguments must denote object sizes.
Everything else is just extremely confusing.

For example, I don't expect

  read (fd, ptr, SSIZE_MAX)

to succeed as long as the actual number of bytes read is sufficiently
small (and it fails with EFAULT on Linux).

Florian
  

On 01/05/2016 03:09 AM, Florian Weimer wrote:
> For example, I don't expect
>
>    read (fd, ptr, SSIZE_MAX)
>
> to succeed as long as the actual number of bytes read is sufficiently
> small (and it fails with EFAULT on Linux).

I agree with your expectation. Any program playing that sort of game is 
a security problem waiting to happen, and GNU/Linux is doing the right 
thing here.
  

On 2016-01-05 10:06, Paul Eggert wrote:
>> Are you seriously arguing for glibc not to comply with C11?
>
> Naaah.And on second thought, my comment was based on a backwards
> interpretation of the POSIX spec: that spec places extra constraints on
> the implementation not on the application. So please forget my mistaken
> comment about POSIX snprintf.

Sorry, I don't understand what you mean. Your first reading of POSIX 
seems quite reasonable to me. And that reading conflicts with C11 so I 
reported it in http://austingroupbugs.net/view.php?id=1020 .

>> descriptions (e.g. [2], [3]) of strlcpy and strlcat refer to C9X or BSD,
>> not to POSIX. So it's not relevant to the current discussion.
>
> Yes and no. I think Florian is arguing that strlcpy should act like
> snprintf, and so in that sense it's relevant. However, I agree with you
> that snprintf is not an exact analog and is not a good precedent for
> strlcpy.

It's not just Florian arguing about snprintf, it's the original article 
about strlcpy/strlcat referring to snprintf and OpenBSD man page 
unambiguously stating:

| Besides quibbles over the return type (size_t versus int) and signal
| handler safety (snprintf(3) is not entirely safe on some systems),
| the following two are equivalent:
|
|   n = strlcpy(dst, src, len);
|   n = snprintf(dst, len, "%s", src);

What I meant in my previous email is that these descriptions referred to 
C9X and BSD flavors of snprintf, not to the POSIX one. But now that we 
are agree to ignore POSIX for a moment, this detail is not important.

>> do you think the fact that @var{to} and @var{from}
>> overlap makes the code:
>>
>>    char s[10] = "abc";
>>    strncat(s, s, 2);
>>
>> is invalid even though there is no overlapping copying?
>
> I don't think strncat is an exact analog for strlcat either. That is,
> although I agree that the strncat example has well-defined behavior in
> C11, this is not precedent for strlcat, any more than snprintf would be.
> They're just different functions, that's all.
>
> For example, it'd be reasonable for strlcpy to verify that every byte in
> the destination array is writeable. That could help catch bugs in
> programs. The spec should not disallow such an implementation.

Ok, I guess it boils down to the question which sources are considers 
authoritative for describing strlcpy/strlcat? Is OpenBSD man page included?
  

On 01/05/2016 11:14 AM, Alexander Cherepanov wrote:
> Your first reading of POSIX seems quite reasonable to me. And that 
> reading conflicts with C11 so I reported it in 
> http://austingroupbugs.net/view.php?id=1020 .
>

I was referring to the C11 semantics, which POSIX defers to. In looking 
at that bug-report trail, it appears that the semantics of snprintf are 
controversial in this area. And rightly so, since C11 requires 
implementations to not diagnose troublesome application usage such as 
snprintf (buf, SIZE_MAX, ...).

> But now that we are agree to ignore POSIX for a moment, this detail is 
> not important.
>

True.

>
> it boils down to the question which sources are considers 
> authoritative for describing strlcpy/strlcat?

There aren't any. The BSD man pages do not agree, and their 
implementations do not agree. If we add strlcpy/strlcat, we will just 
need to use our best judgment to distinguish valid from buggy uses.
  

On 2016-01-05 14:09, Florian Weimer wrote:
> On 01/05/2016 01:02 AM, Alexander Cherepanov wrote:
>
>>> +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:
>
> No, my point was that this code:
>
>    char s[10];
>    strcpy (s + 5, "abc");
>    snprintf (s, sizeof (s), "%s", s + 5);
>
> is invalid.

Then my example is even more invalid (according to this logic).

> This depends on what “objects” means in “If copying takes
> place between objects that overlap, the behavior is undefined.”

Ok

>>    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.
>
> POSIX, on the other hand, speaks of the “n argument which states the
> size of the buffer referred to by s”.

I'm not sure what you mean. That POSIX differs from ISO C and should be 
preferred here? Or that POSIX just describes the same in more details 
and that ISO C actually means the same? Or that authors of 
strlcpy/strlcat talked about the POSIX flavor of snprintf in their doc? 
Or something else?

Also: http://austingroupbugs.net/view.php?id=1020 .

> POSIX also requires that the -1
> argument results in -1/EOVERFLOW result,

Yeah, clear contradiction with ISO C.

> but this does not apply to
> strlcpy because it has a size_t (not int) result.
>
> I'm not sure what the intent behind these specifications is.  I would
> certainly prefer that size arguments must denote object sizes.
> Everything else is just extremely confusing.

Ok, I'm confused. I can see at least 3 different parts in drafting the 
specification of strlcpy/strlcat for glibc:
1) what was required/guaranteed by original strlcpy/strlcat,
2) what glibc is willing to break from it,
3) what glibc wants to add on top of it.

When you talk about snprintf analogy is it 1 or 3?

Which sources do you consider authoritative for 1? OpenBSD man page counts?

My impression was that snprintf analogy is in 1. And that this means 
that @var{size} is not required to be the size of the array.

> For example, I don't expect
>
>    read (fd, ptr, SSIZE_MAX)
>
> to succeed as long as the actual number of bytes read is sufficiently
> small (and it fails with EFAULT on Linux).

You mean that it violates POSIX and you like it? Hm...

I guess opinions will differ (as we have already seen with 
malloc(SSIZE_MAX+1)). But I would expect that all places where glibc 
intentionally deviates from the standards to be marked (at least). Are 
there such places already?
  

On 01/05/2016 10:20 PM, Alexander Cherepanov wrote:

>> POSIX, on the other hand, speaks of the “n argument which states the
>> size of the buffer referred to by s”.
> 
> I'm not sure what you mean. That POSIX differs from ISO C and should be
> preferred here? Or that POSIX just describes the same in more details
> and that ISO C actually means the same?

My point is that we are reading too much into those texts.  I can only
assume that the POSIX authors assumed their formulation was equivalent
when it clearly is not.  This means that some clarification is needed
(both in ISO C and POSIX) to decide what we really want (which, at this
stage should be what existing implementations do, if they are aligned,
but people working standards have sometimes different ideas about how to
deal with such ambiguities), or if things should be left unspecified or
implementation-defined.

>> but this does not apply to
>> strlcpy because it has a size_t (not int) result.
>>
>> I'm not sure what the intent behind these specifications is.  I would
>> certainly prefer that size arguments must denote object sizes.
>> Everything else is just extremely confusing.
> 
> Ok, I'm confused. I can see at least 3 different parts in drafting the
> specification of strlcpy/strlcat for glibc:
> 1) what was required/guaranteed by original strlcpy/strlcat,
> 2) what glibc is willing to break from it,
> 3) what glibc wants to add on top of it.
> 
> When you talk about snprintf analogy is it 1 or 3?

1) for strlcpy.  For strlcat, I don't think anymore I can make this case.

> Which sources do you consider authoritative for 1? OpenBSD man page counts?

The current (amended) OpenBSD manual page.

> My impression was that snprintf analogy is in 1. And that this means
> that @var{size} is not required to be the size of the array.

Doesn't OpenBSD snprintf has the bounded pointer check?

>> For example, I don't expect
>>
>>    read (fd, ptr, SSIZE_MAX)
>>
>> to succeed as long as the actual number of bytes read is sufficiently
>> small (and it fails with EFAULT on Linux).
> 
> You mean that it violates POSIX and you like it? Hm...

I don't think it violates POSIX.  I think the text was not written with
this level of exegesis in mind.

Florian
  

On 2016-01-06 00:05, Paul Eggert wrote:
> On 01/05/2016 11:14 AM, Alexander Cherepanov wrote:
>> Your first reading of POSIX seems quite reasonable to me. And that
>> reading conflicts with C11 so I reported it in
>> http://austingroupbugs.net/view.php?id=1020 .
>
> I was referring to the C11 semantics,

Ok. Then perhaps you are willing to remove the following piece from the 
snprintf description: "you should allocate at least @var{size} 
characters for the string @var{s}"[1]? (And maybe fix the use of 
xmalloc/xrealloc vs. results checks in the example there.)

More generally, the glibc manual describes many functions quite 
differently from C11 (e.g., I've filed bugs 19406 and 19407) and it's 
not clear what are the plans in this area.

[1] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=manual/stdio.texi;h=0326f29eaecd6efa20dd397db095f755bffac495;hb=HEAD#l2474

> which POSIX defers to. In looking
> at that bug-report trail, it appears that the semantics of snprintf are
> controversial in this area. And rightly so, since C11 requires
> implementations to not diagnose troublesome application usage such as
> snprintf (buf, SIZE_MAX, ...).

Some call it troublesome, others consider SIZE_MAX a useful 
no-truncation-required value permitting to unify various code paths.

>> it boils down to the question which sources are considers
>> authoritative for describing strlcpy/strlcat?
>
> There aren't any. The BSD man pages do not agree, and their
> implementations do not agree. If we add strlcpy/strlcat, we will just
> need to use our best judgment to distinguish valid from buggy uses.

Ok, I see.
  

On 2016-01-06 02:07, Florian Weimer wrote:
> On 01/05/2016 10:20 PM, Alexander Cherepanov wrote:
>
>>> POSIX, on the other hand, speaks of the “n argument which states the
>>> size of the buffer referred to by s”.
>>
>> I'm not sure what you mean. That POSIX differs from ISO C and should be
>> preferred here? Or that POSIX just describes the same in more details
>> and that ISO C actually means the same?
>
> My point is that we are reading too much into those texts.  I can only
> assume that the POSIX authors assumed their formulation was equivalent
> when it clearly is not.  This means that some clarification is needed
> (both in ISO C and POSIX) to decide what we really want (which, at this
> stage should be what existing implementations do, if they are aligned,
> but people working standards have sometimes different ideas about how to
> deal with such ambiguities), or if things should be left unspecified or
> implementation-defined.

I don't think this is applicable to ISO C in this particular case. In 
questions "size of object vs. number of chars to copy" and "string vs. 
non-null-terminated array" it seems quite accurate and consistent. I 
doubt there is a chance to change something here. There even was a 
movement in the direction opposite to what you want -- it was clarified 
in C11 for memchr that an implementation cannot go further after a 
matching character is found (s.a. glibc bug 19406).

OTOH POSIX is quite sloppy in this area. Apart from the snprintf issue, 
I recently reported a strndup issue -- 
http://austingroupbugs.net/view.php?id=1019 . I mainly aimed at 
consistency with other strn* functions, maybe you want to chime in with 
other considerations.

Sure, C11 has its own problems, bugs and crazy defect reports but IMHO 
they are more advanced.

>>> but this does not apply to
>>> strlcpy because it has a size_t (not int) result.
>>>
>>> I'm not sure what the intent behind these specifications is.  I would
>>> certainly prefer that size arguments must denote object sizes.
>>> Everything else is just extremely confusing.
>>
>> Ok, I'm confused. I can see at least 3 different parts in drafting the
>> specification of strlcpy/strlcat for glibc:
>> 1) what was required/guaranteed by original strlcpy/strlcat,
>> 2) what glibc is willing to break from it,
>> 3) what glibc wants to add on top of it.
>>
>> When you talk about snprintf analogy is it 1 or 3?
>
> 1) for strlcpy.  For strlcat, I don't think anymore I can make this case.
>
>> Which sources do you consider authoritative for 1? OpenBSD man page counts?
>
> The current (amended) OpenBSD manual page.

Ok, thanks.

>> My impression was that snprintf analogy is in 1. And that this means
>> that @var{size} is not required to be the size of the array.
>
> Doesn't OpenBSD snprintf has the bounded pointer check?

Whether they calculate s+n? AFACT no but it's possible that I missed it.

>>> For example, I don't expect
>>>
>>>     read (fd, ptr, SSIZE_MAX)
>>>
>>> to succeed as long as the actual number of bytes read is sufficiently
>>> small (and it fails with EFAULT on Linux).
>>
>> You mean that it violates POSIX and you like it? Hm...
>
> I don't think it violates POSIX.  I think the text was not written with
> this level of exegesis in mind.

Maybe. I don't have enough experience with POSIX to judge it. But the 
problem with such POV is that a standard, once published, starts to live 
its own life. No matter what its authors thought or intended other folks 
work with the published text.

OTOH, is this intentional behavior of read on Linux? Is there a 
consensus around it? If yes, why not submit it to POSIX?