public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* nonstrings in Glibc
@ 2017-11-20 16:54 Martin Sebor
  2017-11-20 17:59 ` Paul Eggert
  2017-11-20 18:20 ` Carlos O'Donell
  0 siblings, 2 replies; 17+ messages in thread
From: Martin Sebor @ 2017-11-20 16:54 UTC (permalink / raw)
  To: GNU C Library

I'm done testing my update to the -Wstringop-truncation GCC patch
to find misuses of non-string arrays.  With the very limited use
of attribute nonstring it only found one potential bug (22447).
I've been looking at other uses of strncpy in Glibc to see if there
are other arrays that would benefit from the attribute.  I'm not
sufficiently familiar with Glibc data structures so it's a very
slow going.  Could someone help suggests data structures with
array members that might be candidates?

Thanks
Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 16:54 nonstrings in Glibc Martin Sebor
@ 2017-11-20 17:59 ` Paul Eggert
  2017-11-20 18:44   ` Martin Sebor
  2017-11-20 18:20 ` Carlos O'Donell
  1 sibling, 1 reply; 17+ messages in thread
From: Paul Eggert @ 2017-11-20 17:59 UTC (permalink / raw)
  To: Martin Sebor, GNU C Library

On 11/20/2017 08:54 AM, Martin Sebor wrote:
> I've been looking at other uses of strncpy in Glibc to see if there
> are other arrays that would benefit from the attribute.  I'm not
> sufficiently familiar with Glibc data structures so it's a very
> slow going.  Could someone help suggests data structures with
> array members that might be candidates?

If GCC is not warning about uses of the array, what would be the benefit 
of marking it with __attribute__ ((nonstring))?

Is this because you're thinking of changing GCC so that it warns about 
strlen(x) where x is marked with __attribute__ ((nonstring))? If so, how 
would that benefit the typical case? Many char arrays start off being 
nonstrings, and are later turned into strings by storing '\0' somewhere. 
Although it's not OK to call strlen on these arrays at first, it's fine 
to do so later. How would a static attribute capture this typical situation?

The typical situation is distinct from the strncpy case where 
__attribute__ ((nonstring)) applies throught the array's lifetime. 
Although I can see that the strlen warning would be useful when code 
mistakenly applies strlen to strncpy-like arrays, these arrays are quite 
rare in glibc and you should be able to find them all by looking at uses 
of strncpy and strncat.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 16:54 nonstrings in Glibc Martin Sebor
  2017-11-20 17:59 ` Paul Eggert
@ 2017-11-20 18:20 ` Carlos O'Donell
  2017-11-20 18:41   ` Florian Weimer
  2017-11-21 23:41   ` Martin Sebor
  1 sibling, 2 replies; 17+ messages in thread
From: Carlos O'Donell @ 2017-11-20 18:20 UTC (permalink / raw)
  To: Martin Sebor, GNU C Library

On 11/20/2017 08:54 AM, Martin Sebor wrote:
> I'm done testing my update to the -Wstringop-truncation GCC patch
> to find misuses of non-string arrays.  With the very limited use
> of attribute nonstring it only found one potential bug (22447).
> I've been looking at other uses of strncpy in Glibc to see if there
> are other arrays that would benefit from the attribute.  I'm not
> sufficiently familiar with Glibc data structures so it's a very
> slow going.  Could someone help suggests data structures with
> array members that might be candidates?

struct sockaddr's sun_path?

http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735

Is that what you need help finding?

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 18:20 ` Carlos O'Donell
@ 2017-11-20 18:41   ` Florian Weimer
  2017-11-21 21:21     ` Martin Sebor
  2017-11-21 23:41   ` Martin Sebor
  1 sibling, 1 reply; 17+ messages in thread
From: Florian Weimer @ 2017-11-20 18:41 UTC (permalink / raw)
  To: Carlos O'Donell, Martin Sebor, GNU C Library

On 11/20/2017 07:20 PM, Carlos O'Donell wrote:
> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>> I'm done testing my update to the -Wstringop-truncation GCC patch
>> to find misuses of non-string arrays.  With the very limited use
>> of attribute nonstring it only found one potential bug (22447).
>> I've been looking at other uses of strncpy in Glibc to see if there
>> are other arrays that would benefit from the attribute.  I'm not
>> sufficiently familiar with Glibc data structures so it's a very
>> slow going.  Could someone help suggests data structures with
>> array members that might be candidates?
> 
> struct sockaddr's sun_path?
>  > http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735

Please include some context in case the link target goes away.

This discussion was about the flexible array member nature of sun_path. 
Another complication is the overloading of the member with the abstract 
namespace (leading NUL byte).

For the variable length member, d_name in struct dirent is another 
example.  For struct dirent and struct dirent_64, all uses which rely on 
a fixed size of the d_name member should probably result in warnings.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 17:59 ` Paul Eggert
@ 2017-11-20 18:44   ` Martin Sebor
  2017-11-21  0:45     ` Paul Eggert
  0 siblings, 1 reply; 17+ messages in thread
From: Martin Sebor @ 2017-11-20 18:44 UTC (permalink / raw)
  To: Paul Eggert, GNU C Library

On 11/20/2017 10:59 AM, Paul Eggert wrote:
> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>> I've been looking at other uses of strncpy in Glibc to see if there
>> are other arrays that would benefit from the attribute.  I'm not
>> sufficiently familiar with Glibc data structures so it's a very
>> slow going.  Could someone help suggests data structures with
>> array members that might be candidates?
>
> If GCC is not warning about uses of the array, what would be the benefit
> of marking it with __attribute__ ((nonstring))?
>
> Is this because you're thinking of changing GCC so that it warns about
> strlen(x) where x is marked with __attribute__ ((nonstring))? If so, how
> would that benefit the typical case? Many char arrays start off being
> nonstrings, and are later turned into strings by storing '\0' somewhere.
> Although it's not OK to call strlen on these arrays at first, it's fine
> to do so later. How would a static attribute capture this typical
> situation?

The expectation is that users will use strnlen and other "bounded"
functions with arrays declared non-string rather than strlen et al.

The checker runs late in GCC, after all optimizing transformations,
so provably safe uses of such arrays with unbounded functions are
not diagnosed.

For example:

char a[4] __attribute__ ((nonstring));

int f (void)
{
   __builtin_strcpy (a, "123");

   return __builtin_strlen (a);   // safe, no warning
}

int g (void)
{
   __builtin_strncpy (a, "1234", sizeof a);

   return __builtin_strlen (a);   // not safe, warning
}

a.c: In function ‘g’:
a.c:14:10: warning: ‘__builtin_strlen’ argument 1 declared attribute 
‘nonstring’ [-Wstringop-overflow=]
    return __builtin_strlen (a);   // not safe, warning
           ^~~~~~~~~~~~~~~~~~~~
a.c:1:6: note: argument ‘a’ declared here
  char a[4] __attribute__ ((nonstring));
       ^

> The typical situation is distinct from the strncpy case where
> __attribute__ ((nonstring)) applies throught the array's lifetime.
> Although I can see that the strlen warning would be useful when code
> mistakenly applies strlen to strncpy-like arrays, these arrays are quite
> rare in glibc and you should be able to find them all by looking at uses
> of strncpy and strncat.

Right, that's goal.  I have been looking at strncpy uses but not
really finding any opportunities to annotate character arrays (or
pointers) nonstring beyond those that have already been marked.
I'll keep looking.

Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 18:44   ` Martin Sebor
@ 2017-11-21  0:45     ` Paul Eggert
  0 siblings, 0 replies; 17+ messages in thread
From: Paul Eggert @ 2017-11-21  0:45 UTC (permalink / raw)
  To: Martin Sebor, GNU C Library

On 11/20/2017 10:44 AM, Martin Sebor wrote:
> I have been looking at strncpy uses but not
> really finding any opportunities to annotate character arrays (or
> pointers) nonstring beyond those that have already been marked.
> I'll keep looking.

Perhaps you should look at calls to strnlen too. If code uses strnlen, 
that's a hint that possibly strnlen's argument should be marked with 
nostring.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 18:41   ` Florian Weimer
@ 2017-11-21 21:21     ` Martin Sebor
  2017-11-21 21:34       ` Florian Weimer
  2017-11-21 21:37       ` Zack Weinberg
  0 siblings, 2 replies; 17+ messages in thread
From: Martin Sebor @ 2017-11-21 21:21 UTC (permalink / raw)
  To: Florian Weimer, Carlos O'Donell, GNU C Library

On 11/20/2017 11:41 AM, Florian Weimer wrote:
> On 11/20/2017 07:20 PM, Carlos O'Donell wrote:
>> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>>> I'm done testing my update to the -Wstringop-truncation GCC patch
>>> to find misuses of non-string arrays.  With the very limited use
>>> of attribute nonstring it only found one potential bug (22447).
>>> I've been looking at other uses of strncpy in Glibc to see if there
>>> are other arrays that would benefit from the attribute.  I'm not
>>> sufficiently familiar with Glibc data structures so it's a very
>>> slow going.  Could someone help suggests data structures with
>>> array members that might be candidates?
>>
>> struct sockaddr's sun_path?
>>  > http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>
> Please include some context in case the link target goes away.
>
> This discussion was about the flexible array member nature of sun_path.
> Another complication is the overloading of the member with the abstract
> namespace (leading NUL byte).
>
> For the variable length member, d_name in struct dirent is another
> example.  For struct dirent and struct dirent_64, all uses which rely on
> a fixed size of the d_name member should probably result in warnings.

I see two struct dirent in my Glibc build on x86_64: one in
bits/dirent.h and another in sysdeps/unix/sysv/linux/bits/dirent.h.
AFAICT, the first one is the one that's installed and the second
one is used internally within Glibc.  Did I get that right?  Should
one of these be annotated with attribute nonstring? (POSIX requires
d_name to be nul-terminated and there are assumptions in Glibc that
rely on it being so, such as the _D_EXACT_NAMLEN() macro, that
trigger the non-string warning if I annotate the latter.)

Also, what exactly do you mean by "uses which rely on a fixed size
of the d_name member?"  Something attribute nonstring could help
with or something else/some other warning?

Thanks
Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 21:21     ` Martin Sebor
@ 2017-11-21 21:34       ` Florian Weimer
  2017-11-21 21:37       ` Zack Weinberg
  1 sibling, 0 replies; 17+ messages in thread
From: Florian Weimer @ 2017-11-21 21:34 UTC (permalink / raw)
  To: Martin Sebor, Carlos O'Donell, GNU C Library

On 11/21/2017 10:20 PM, Martin Sebor wrote:

>> For the variable length member, d_name in struct dirent is another
>> example.  For struct dirent and struct dirent_64, all uses which rely on
>> a fixed size of the d_name member should probably result in warnings.
> 
> I see two struct dirent in my Glibc build on x86_64: one in
> bits/dirent.h and another in sysdeps/unix/sysv/linux/bits/dirent.h.
> AFAICT, the first one is the one that's installed and the second
> one is used internally within Glibc.

No, both are installed, but the one in sysdeps overrides the top-level 
one (on Linux).

> Did I get that right?  Should
> one of these be annotated with attribute nonstring? (POSIX requires
> d_name to be nul-terminated and there are assumptions in Glibc that
> rely on it being so, such as the _D_EXACT_NAMLEN() macro, that
> trigger the non-string warning if I annotate the latter.)
> 
> Also, what exactly do you mean by "uses which rely on a fixed size
> of the d_name member?"  Something attribute nonstring could help
> with or something else/some other warning?

It's not really related to the nonstring attribute, except that it's 
valid for the array not to contain a null terminator in both cases.

These structs can be allocated as part of a larger object where the name 
field runs off the end of the struct sockaddr_un/struct dirent object. 
As a result, if you use

   strcpy (d2->d_name, d1->d_name)

to make a copy of the name, that can introduce a buffer overflow.  (We 
had this bug in glob.)

The C language does not have a good way to address this, but things have 
been this way for ages on Linux with glibc.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 21:21     ` Martin Sebor
  2017-11-21 21:34       ` Florian Weimer
@ 2017-11-21 21:37       ` Zack Weinberg
  2017-11-21 22:31         ` Andreas Schwab
  2017-11-21 22:47         ` Joseph Myers
  1 sibling, 2 replies; 17+ messages in thread
From: Zack Weinberg @ 2017-11-21 21:37 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Florian Weimer, Carlos O'Donell, GNU C Library

On Tue, Nov 21, 2017 at 4:20 PM, Martin Sebor <msebor@gmail.com> wrote:
>
> I see two struct dirent in my Glibc build on x86_64: one in
> bits/dirent.h and another in sysdeps/unix/sysv/linux/bits/dirent.h.
> AFAICT, the first one is the one that's installed and the second
> one is used internally within Glibc.  Did I get that right?

No, that's not right. The top-level bits directory contains _fallback_
bits headers, to be used only if no sysdeps directory overrides them;
I don't know why it's in the top level instead of in sysdeps/generic
(which is where you'd expect it to be) but it's probably something
horrible to do with the include search path.

On Linux-based configurations, sysdeps/unix/sysv/linux/bits/dirent.h
should be both installed and used internally.

>  Should
> one of these be annotated with attribute nonstring? (POSIX requires
> d_name to be nul-terminated and there are assumptions in Glibc that
> rely on it being so, such as the _D_EXACT_NAMLEN() macro, that
> trigger the non-string warning if I annotate the latter.)

If POSIX requires d_name to be nul-terminated, then I think it's safe
to say that attribute nonstring is not appropriate for it (assuming I
understand the point of attribute nonstring, anyway, which I only know
about courtesy this thread).

zw

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 21:37       ` Zack Weinberg
@ 2017-11-21 22:31         ` Andreas Schwab
  2017-11-21 22:39           ` Florian Weimer
  2017-11-21 22:47         ` Joseph Myers
  1 sibling, 1 reply; 17+ messages in thread
From: Andreas Schwab @ 2017-11-21 22:31 UTC (permalink / raw)
  To: Zack Weinberg
  Cc: Martin Sebor, Florian Weimer, Carlos O'Donell, GNU C Library

On Nov 21 2017, Zack Weinberg <zackw@panix.com> wrote:

> If POSIX requires d_name to be nul-terminated,

"The array d_name is of unspecified size, but shall contain a filename
of at most {NAME_MAX} bytes followed by a terminating null byte."

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 22:31         ` Andreas Schwab
@ 2017-11-21 22:39           ` Florian Weimer
  0 siblings, 0 replies; 17+ messages in thread
From: Florian Weimer @ 2017-11-21 22:39 UTC (permalink / raw)
  To: Andreas Schwab, Zack Weinberg
  Cc: Martin Sebor, Carlos O'Donell, GNU C Library

On 11/21/2017 11:31 PM, Andreas Schwab wrote:
> On Nov 21 2017, Zack Weinberg <zackw@panix.com> wrote:
> 
>> If POSIX requires d_name to be nul-terminated,
> 
> "The array d_name is of unspecified size, but shall contain a filename
> of at most {NAME_MAX} bytes followed by a terminating null byte."

Our definition of NAME_MAX is not large enough on Linux.  I suspect the 
actual kernel limit is around a page size or something like that.

We could start to enforce that, but then you won't be able to access 
some files on NTFS or SMB file systems anymore.  They have a 255 
character limit, too, but as it's measured in UTF-16 wide characters, 
and we use UTF-8 for file name encoding, 255 bytes aren't sufficient to 
store all names.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 21:37       ` Zack Weinberg
  2017-11-21 22:31         ` Andreas Schwab
@ 2017-11-21 22:47         ` Joseph Myers
  1 sibling, 0 replies; 17+ messages in thread
From: Joseph Myers @ 2017-11-21 22:47 UTC (permalink / raw)
  To: Zack Weinberg
  Cc: Martin Sebor, Florian Weimer, Carlos O'Donell, GNU C Library

On Tue, 21 Nov 2017, Zack Weinberg wrote:

> No, that's not right. The top-level bits directory contains _fallback_
> bits headers, to be used only if no sysdeps directory overrides them;
> I don't know why it's in the top level instead of in sysdeps/generic
> (which is where you'd expect it to be) but it's probably something
> horrible to do with the include search path.

Moved by Roland in:

commit c72565e5f1124c2dc72573e83406fe999e56091f
Author: Roland McGrath <roland@gnu.org>
Date:   Thu Dec 22 03:12:10 2005 +0000

    2005-12-21  Roland McGrath  <roland@redhat.com>
    
        * sysdeps/generic/bits: Subdirectory and all files moved to ...
        * bits: ... here, new subdirectory.
        * Makeconfig (+includes): Reordered includes to put build and sysdeps
        dirs first after $(..)include, $(sysincludes) last.
        * sysdeps/generic/bits: Subdirectory and all files moved to ...
        * bits: ... here, new subdirectory.
        * Makeconfig (+includes): Reordered includes to put build and sysdeps
        dirs first after $(..)include, $(sysincludes) last.

(I don't know why, however.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-20 18:20 ` Carlos O'Donell
  2017-11-20 18:41   ` Florian Weimer
@ 2017-11-21 23:41   ` Martin Sebor
  2017-11-22  0:14     ` Dmitry V. Levin
  1 sibling, 1 reply; 17+ messages in thread
From: Martin Sebor @ 2017-11-21 23:41 UTC (permalink / raw)
  To: Carlos O'Donell, GNU C Library

On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>> I'm done testing my update to the -Wstringop-truncation GCC patch
>> to find misuses of non-string arrays.  With the very limited use
>> of attribute nonstring it only found one potential bug (22447).
>> I've been looking at other uses of strncpy in Glibc to see if there
>> are other arrays that would benefit from the attribute.  I'm not
>> sufficiently familiar with Glibc data structures so it's a very
>> slow going.  Could someone help suggests data structures with
>> array members that might be candidates?
>
> struct sockaddr's sun_path?
>
> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>
> Is that what you need help finding?

Yes, that's what I'm looking for, thanks!

 From the referenced thread it sounds like POSIX doesn't require
sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
But I'm not sure what happens on Linux.  According to Michael
Kerrisk's response it sounds like it is nul-terminated, but
then according to the longer discussion on linux.kernel.api
it sounds like it isn't.  Which is it?

If it's not guaranteed to be nul-terminated then the following
suggests the code in clntunix_create might be unsafe:

clnt_unix.c: In function ‘clntunix_create’:
clnt_unix.c:137:13: warning: ‘strlen’ argument 1 declared attribute 
‘nonstring’ [-Wstringop-overflow=]
        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;
              ^~~~~~~~~~~~~~~~~~~~~~~~

Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-21 23:41   ` Martin Sebor
@ 2017-11-22  0:14     ` Dmitry V. Levin
  2017-11-27 16:25       ` Martin Sebor
  2017-11-27 16:25       ` Martin Sebor
  0 siblings, 2 replies; 17+ messages in thread
From: Dmitry V. Levin @ 2017-11-22  0:14 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Carlos O'Donell, GNU C Library

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On Tue, Nov 21, 2017 at 04:41:27PM -0700, Martin Sebor wrote:
> On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
> > On 11/20/2017 08:54 AM, Martin Sebor wrote:
> >> I'm done testing my update to the -Wstringop-truncation GCC patch
> >> to find misuses of non-string arrays.  With the very limited use
> >> of attribute nonstring it only found one potential bug (22447).
> >> I've been looking at other uses of strncpy in Glibc to see if there
> >> are other arrays that would benefit from the attribute.  I'm not
> >> sufficiently familiar with Glibc data structures so it's a very
> >> slow going.  Could someone help suggests data structures with
> >> array members that might be candidates?
> >
> > struct sockaddr's sun_path?
> >
> > http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
> >
> > Is that what you need help finding?
> 
> Yes, that's what I'm looking for, thanks!
> 
>  From the referenced thread it sounds like POSIX doesn't require
> sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
> But I'm not sure what happens on Linux.  According to Michael
> Kerrisk's response it sounds like it is nul-terminated, but
> then according to the longer discussion on linux.kernel.api
> it sounds like it isn't.  Which is it?

When struct sockaddr_un is passed to linux kernel, the kernel doesn't
treat sun_path as nul-terminated, see net/unix/af_unix.c:unix_mkname
for implementation details.

However, when linux kernel returns struct sockaddr_un, sun_path is
nul-terminated if there is enough room provided by userspace, e.g.
it may need sizeof(struct sockaddr_un) + 1 bytes to write that NUL
beyond struct sockaddr_un.sun_path.

strace test suite contains a test (sun_path.test) for this linux kernel
behavior.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-22  0:14     ` Dmitry V. Levin
  2017-11-27 16:25       ` Martin Sebor
@ 2017-11-27 16:25       ` Martin Sebor
  1 sibling, 0 replies; 17+ messages in thread
From: Martin Sebor @ 2017-11-27 16:25 UTC (permalink / raw)
  To: Carlos O'Donell, GNU C Library

On 11/21/2017 05:14 PM, Dmitry V. Levin wrote:
> On Tue, Nov 21, 2017 at 04:41:27PM -0700, Martin Sebor wrote:
>> On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
>>> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>>>> I'm done testing my update to the -Wstringop-truncation GCC patch
>>>> to find misuses of non-string arrays.  With the very limited use
>>>> of attribute nonstring it only found one potential bug (22447).
>>>> I've been looking at other uses of strncpy in Glibc to see if there
>>>> are other arrays that would benefit from the attribute.  I'm not
>>>> sufficiently familiar with Glibc data structures so it's a very
>>>> slow going.  Could someone help suggests data structures with
>>>> array members that might be candidates?
>>>
>>> struct sockaddr's sun_path?
>>>
>>> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>>>
>>> Is that what you need help finding?
>>
>> Yes, that's what I'm looking for, thanks!
>>
>>  From the referenced thread it sounds like POSIX doesn't require
>> sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
>> But I'm not sure what happens on Linux.  According to Michael
>> Kerrisk's response it sounds like it is nul-terminated, but
>> then according to the longer discussion on linux.kernel.api
>> it sounds like it isn't.  Which is it?
>
> When struct sockaddr_un is passed to linux kernel, the kernel doesn't
> treat sun_path as nul-terminated, see net/unix/af_unix.c:unix_mkname
> for implementation details.

Thank you.  This is good to know (and expected).  The Glibc
attribute is meant to help with the user side of things so
it's your note below that's especially relevant.  (There
are a few warnings in the Linux kernel suggesting it too
might benefit from the new attribute, if only to suppress
them.)

> However, when linux kernel returns struct sockaddr_un, sun_path is
> nul-terminated if there is enough room provided by userspace, e.g.
> it may need sizeof(struct sockaddr_un) + 1 bytes to write that NUL
> beyond struct sockaddr_un.sun_path.

So if I understand correctly, sun_path will contain a nul-
terminated string if there is enough room for the terminating
nul but not otherwise.  If that's so, that would suggest that
adding attribute nonstring to sum_path might be appropriate.
At the same time, based on existing usage it seems that it
would be prone to false positives.  Annotating sun_path with
it turns up only one warning in a Glibc build:

clnt_unix.c:137:13: warning: ‘strlen’ argument 1 declared attribute 
‘nonstring’ [-Wstringop-overflow=]
        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;

AFAICS, the function is only called from clnt_create() where
it's passed a nul-terminated string, so unless it can be called
from user code with a non-nul-terminated array this would be
an example of such a false positive.

Carlos, do you think the attribute would be helpful despite
the false positives?

Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-22  0:14     ` Dmitry V. Levin
@ 2017-11-27 16:25       ` Martin Sebor
  2017-11-27 16:40         ` Carlos O'Donell
  2017-11-27 16:25       ` Martin Sebor
  1 sibling, 1 reply; 17+ messages in thread
From: Martin Sebor @ 2017-11-27 16:25 UTC (permalink / raw)
  To: Carlos O'Donell, GNU C Library

On 11/21/2017 05:14 PM, Dmitry V. Levin wrote:
> On Tue, Nov 21, 2017 at 04:41:27PM -0700, Martin Sebor wrote:
>> On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
>>> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>>>> I'm done testing my update to the -Wstringop-truncation GCC patch
>>>> to find misuses of non-string arrays.  With the very limited use
>>>> of attribute nonstring it only found one potential bug (22447).
>>>> I've been looking at other uses of strncpy in Glibc to see if there
>>>> are other arrays that would benefit from the attribute.  I'm not
>>>> sufficiently familiar with Glibc data structures so it's a very
>>>> slow going.  Could someone help suggests data structures with
>>>> array members that might be candidates?
>>>
>>> struct sockaddr's sun_path?
>>>
>>> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>>>
>>> Is that what you need help finding?
>>
>> Yes, that's what I'm looking for, thanks!
>>
>>  From the referenced thread it sounds like POSIX doesn't require
>> sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
>> But I'm not sure what happens on Linux.  According to Michael
>> Kerrisk's response it sounds like it is nul-terminated, but
>> then according to the longer discussion on linux.kernel.api
>> it sounds like it isn't.  Which is it?
>
> When struct sockaddr_un is passed to linux kernel, the kernel doesn't
> treat sun_path as nul-terminated, see net/unix/af_unix.c:unix_mkname
> for implementation details.

Thank you.  This is good to know (and expected).  The Glibc
attribute is meant to help with the user side of things so
it's your note below that's especially relevant.  (There
are a few warnings in the Linux kernel suggesting it too
might benefit from the new attribute, if only to suppress
them.)

> However, when linux kernel returns struct sockaddr_un, sun_path is
> nul-terminated if there is enough room provided by userspace, e.g.
> it may need sizeof(struct sockaddr_un) + 1 bytes to write that NUL
> beyond struct sockaddr_un.sun_path.

So if I understand correctly, sun_path will contain a nul-
terminated string if there is enough room for the terminating
nul but not otherwise.  If that's so, that would suggest that
adding attribute nonstring to sum_path might be appropriate.
At the same time, based on existing usage it seems that it
would be prone to false positives.  Annotating sun_path with
it turns up only one warning in a Glibc build:

clnt_unix.c:137:13: warning: ‘strlen’ argument 1 declared attribute 
‘nonstring’ [-Wstringop-overflow=]
        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;

AFAICS, the function is only called from clnt_create() where
it's passed a nul-terminated string, so unless it can be called
from user code with a non-nul-terminated array this would be
an example of such a false positive.

Carlos, do you think the attribute would be helpful despite
the false positives?

Martin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: nonstrings in Glibc
  2017-11-27 16:25       ` Martin Sebor
@ 2017-11-27 16:40         ` Carlos O'Donell
  0 siblings, 0 replies; 17+ messages in thread
From: Carlos O'Donell @ 2017-11-27 16:40 UTC (permalink / raw)
  To: Martin Sebor, GNU C Library

On 11/27/2017 08:24 AM, Martin Sebor wrote:
> On 11/21/2017 05:14 PM, Dmitry V. Levin wrote:
>> On Tue, Nov 21, 2017 at 04:41:27PM -0700, Martin Sebor wrote:
>>> On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
>>>> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>>>>> I'm done testing my update to the -Wstringop-truncation GCC patch
>>>>> to find misuses of non-string arrays.  With the very limited use
>>>>> of attribute nonstring it only found one potential bug (22447).
>>>>> I've been looking at other uses of strncpy in Glibc to see if there
>>>>> are other arrays that would benefit from the attribute.  I'm not
>>>>> sufficiently familiar with Glibc data structures so it's a very
>>>>> slow going.  Could someone help suggests data structures with
>>>>> array members that might be candidates?
>>>>
>>>> struct sockaddr's sun_path?
>>>>
>>>> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>>>>
>>>> Is that what you need help finding?
>>>
>>> Yes, that's what I'm looking for, thanks!
>>>
>>>  From the referenced thread it sounds like POSIX doesn't require
>>> sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
>>> But I'm not sure what happens on Linux.  According to Michael
>>> Kerrisk's response it sounds like it is nul-terminated, but
>>> then according to the longer discussion on linux.kernel.api
>>> it sounds like it isn't.  Which is it?
>>
>> When struct sockaddr_un is passed to linux kernel, the kernel doesn't
>> treat sun_path as nul-terminated, see net/unix/af_unix.c:unix_mkname
>> for implementation details.
> 
> Thank you.  This is good to know (and expected).  The Glibc
> attribute is meant to help with the user side of things so
> it's your note below that's especially relevant.  (There
> are a few warnings in the Linux kernel suggesting it too
> might benefit from the new attribute, if only to suppress
> them.)
> 
>> However, when linux kernel returns struct sockaddr_un, sun_path is
>> nul-terminated if there is enough room provided by userspace, e.g.
>> it may need sizeof(struct sockaddr_un) + 1 bytes to write that NUL
>> beyond struct sockaddr_un.sun_path.
> 
> So if I understand correctly, sun_path will contain a nul-
> terminated string if there is enough room for the terminating
> nul but not otherwise.  If that's so, that would suggest that
> adding attribute nonstring to sum_path might be appropriate.
> At the same time, based on existing usage it seems that it
> would be prone to false positives.  Annotating sun_path with
> it turns up only one warning in a Glibc build:
> 
> clnt_unix.c:137:13: warning: ‘strlen’ argument 1 declared attribute ‘nonstring’ [-Wstringop-overflow=]
>        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;
> 
> AFAICS, the function is only called from clnt_create() where
> it's passed a nul-terminated string, so unless it can be called
> from user code with a non-nul-terminated array this would be
> an example of such a false positive.
> 
> Carlos, do you think the attribute would be helpful despite
> the false positives?
I think it would be helpful.

I consider it bad form to manipulate sun_path with strlen at *any*
time since the code might change along with expectations of where
the data came from, and that's how we end up with hard to find
exploits.

To be clear, I advocate for:

* Add the attribute + fix the glibc usage.

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-11-27 16:40 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-20 16:54 nonstrings in Glibc Martin Sebor
2017-11-20 17:59 ` Paul Eggert
2017-11-20 18:44   ` Martin Sebor
2017-11-21  0:45     ` Paul Eggert
2017-11-20 18:20 ` Carlos O'Donell
2017-11-20 18:41   ` Florian Weimer
2017-11-21 21:21     ` Martin Sebor
2017-11-21 21:34       ` Florian Weimer
2017-11-21 21:37       ` Zack Weinberg
2017-11-21 22:31         ` Andreas Schwab
2017-11-21 22:39           ` Florian Weimer
2017-11-21 22:47         ` Joseph Myers
2017-11-21 23:41   ` Martin Sebor
2017-11-22  0:14     ` Dmitry V. Levin
2017-11-27 16:25       ` Martin Sebor
2017-11-27 16:40         ` Carlos O'Donell
2017-11-27 16:25       ` Martin Sebor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).