string: Add tests for zero length string inputs
Commit Message
For the string functions that take string lengths as an argument we
should ensure that no data is read or written if a length of zero is
specified. Pointers to PROT_NONE memory are used to ensure that any
reads or writes will cause a fault.
ChangeLog:
2014-09-16 Will Newton <will.newton@linaro.org>
* string/test-memccpy.c (do_test_zero_length): New function.
(test_main): Call do_test_zero_length.
* string/test-memchr.c: Likewise.
* string/test-memcmp.c: Likewise.
* string/test-memcpy.c: Likewise.
* string/test-memmem.c: Likewise.
* string/test-memmove.c: Likewise.
* string/test-memrchr.c: Likewise.
* string/test-memset.c: Likewise.
* string/test-strncmp.c: Likewise.
* string/test-strncpy.c: Likewise.
* string/test-strnlen.c: Likewise.
* string/test-strncasecmp.c (do_test_zero_length): New function.
(test_locale): Call do_test_zero_length.
* string/test-strncat.c (do_test_zero_length): New function.
(main): Call do_test_zero_length.
---
string/test-memccpy.c | 10 ++++++++++
string/test-memchr.c | 9 +++++++++
string/test-memcmp.c | 11 +++++++++++
string/test-memcpy.c | 11 +++++++++++
string/test-memmem.c | 10 ++++++++++
string/test-memmove.c | 12 ++++++++++++
string/test-memrchr.c | 10 ++++++++++
string/test-memset.c | 11 +++++++++++
string/test-strncasecmp.c | 15 +++++++++++++++
string/test-strncat.c | 24 ++++++++++++++++++++++++
string/test-strncmp.c | 11 +++++++++++
string/test-strncpy.c | 11 +++++++++++
string/test-strnlen.c | 10 ++++++++++
13 files changed, 155 insertions(+)
Comments
On Tue, Sep 16, 2014 at 04:40:30PM -0700, Will Newton wrote:
> For the string functions that take string lengths as an argument we
> should ensure that no data is read or written if a length of zero is
> specified. Pointers to PROT_NONE memory are used to ensure that any
> reads or writes will cause a fault.
>
You do not need these. C standard requires arguments to be valid
pointers for most string functions, and they are already marked nonnull
in header.
Just adding size 0 to inputs would suffice.
On 19 September 2014 04:23, Ondřej Bílka <neleai@seznam.cz> wrote:
> On Tue, Sep 16, 2014 at 04:40:30PM -0700, Will Newton wrote:
>> For the string functions that take string lengths as an argument we
>> should ensure that no data is read or written if a length of zero is
>> specified. Pointers to PROT_NONE memory are used to ensure that any
>> reads or writes will cause a fault.
>>
> You do not need these. C standard requires arguments to be valid
> pointers for most string functions, and they are already marked nonnull
> in header.
>
> Just adding size 0 to inputs would suffice.
These tests are not testing null pointers, they are testing that when
given a zero length the functions actually read/write zero bytes.
Whether the specification demands that behaviour is arguable but I
believe that it is the most sane behaviour.
On 19/09/14 18:09, Will Newton wrote:
> On 19 September 2014 04:23, Ondřej Bílka <neleai@seznam.cz> wrote:
>> On Tue, Sep 16, 2014 at 04:40:30PM -0700, Will Newton wrote:
>>> For the string functions that take string lengths as an argument we
>>> should ensure that no data is read or written if a length of zero is
>>> specified. Pointers to PROT_NONE memory are used to ensure that any
>>> reads or writes will cause a fault.
>>>
>> You do not need these. C standard requires arguments to be valid
>> pointers for most string functions, and they are already marked nonnull
>> in header.
>>
>> Just adding size 0 to inputs would suffice.
>
> These tests are not testing null pointers, they are testing that when
> given a zero length the functions actually read/write zero bytes.
> Whether the specification demands that behaviour is arguable but I
> believe that it is the most sane behaviour.
>
Valid pointers is more than just non-NULL. In particular, it implies
that is safe to dereference the addressed byte in a source operand even
when the length parameter is zero. Thus testing that no bytes are read
would be incorrect.
On 22 September 2014 09:09, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 19/09/14 18:09, Will Newton wrote:
>> On 19 September 2014 04:23, Ondřej Bílka <neleai@seznam.cz> wrote:
>>> On Tue, Sep 16, 2014 at 04:40:30PM -0700, Will Newton wrote:
>>>> For the string functions that take string lengths as an argument we
>>>> should ensure that no data is read or written if a length of zero is
>>>> specified. Pointers to PROT_NONE memory are used to ensure that any
>>>> reads or writes will cause a fault.
>>>>
>>> You do not need these. C standard requires arguments to be valid
>>> pointers for most string functions, and they are already marked nonnull
>>> in header.
>>>
>>> Just adding size 0 to inputs would suffice.
>>
>> These tests are not testing null pointers, they are testing that when
>> given a zero length the functions actually read/write zero bytes.
>> Whether the specification demands that behaviour is arguable but I
>> believe that it is the most sane behaviour.
>>
>
> Valid pointers is more than just non-NULL. In particular, it implies
> that is safe to dereference the addressed byte in a source operand even
> when the length parameter is zero. Thus testing that no bytes are read
> would be incorrect.
If that is the case then I withdraw the patch. Is that requirement
documented anywhere?
On 22/09/14 17:15, Will Newton wrote:
> On 22 September 2014 09:09, Richard Earnshaw <rearnsha@arm.com> wrote:
>> On 19/09/14 18:09, Will Newton wrote:
>>> On 19 September 2014 04:23, Ondřej Bílka <neleai@seznam.cz> wrote:
>>>> On Tue, Sep 16, 2014 at 04:40:30PM -0700, Will Newton wrote:
>>>>> For the string functions that take string lengths as an argument we
>>>>> should ensure that no data is read or written if a length of zero is
>>>>> specified. Pointers to PROT_NONE memory are used to ensure that any
>>>>> reads or writes will cause a fault.
>>>>>
>>>> You do not need these. C standard requires arguments to be valid
>>>> pointers for most string functions, and they are already marked nonnull
>>>> in header.
>>>>
>>>> Just adding size 0 to inputs would suffice.
>>>
>>> These tests are not testing null pointers, they are testing that when
>>> given a zero length the functions actually read/write zero bytes.
>>> Whether the specification demands that behaviour is arguable but I
>>> believe that it is the most sane behaviour.
>>>
>>
>> Valid pointers is more than just non-NULL. In particular, it implies
>> that is safe to dereference the addressed byte in a source operand even
>> when the length parameter is zero. Thus testing that no bytes are read
>> would be incorrect.
>
> If that is the case then I withdraw the patch. Is that requirement
> documented anywhere?
>
C99 $7.21.1 clause 2.
R.
On 09/22/2014 09:09 AM, Richard Earnshaw wrote:
> Valid pointers is more than just non-NULL. In particular, it implies
> that is safe to dereference the addressed byte in a source operand even
> when the length parameter is zero.
I just reread C99 7.1.4 clause 1 and 7.21.2 clause 2, and I don't see
that implication. For example, the following program appears to be
strictly conforming:
#include <string.h>
char src[1];
char dst[1];
int
main (void)
{
memcpy (dst, src + 1, 0);
return 0;
}
Here, src + 1 is a valid pointer even though one cannot safely
dereference it. So it appears to be reasonable to check that memcpy
doesn't dereference the source when the size is zero.
On 09/22/2014 06:09 PM, Richard Earnshaw wrote:
>> These tests are not testing null pointers, they are testing that when
>> given a zero length the functions actually read/write zero bytes.
>> Whether the specification demands that behaviour is arguable but I
>> believe that it is the most sane behaviour.
> Valid pointers is more than just non-NULL. In particular, it implies
> that is safe to dereference the addressed byte in a source operand even
> when the length parameter is zero.
Valid pointers can also point one element past the end of an array of
objects. Such pointers can occur naturally during the final iteration
of buffer-processing loops. I don't think it is reasonable to expect
that programmers write special code (or at least early loop exits) to
deal with this corner case. This has to work, and if the C standard
does not guarantee it does, it needs fixing.
> Thus testing that no bytes are read would be incorrect.
I disagree, per the above.
On 22/09/14 18:48, Paul Eggert wrote:
> On 09/22/2014 09:09 AM, Richard Earnshaw wrote:
>> Valid pointers is more than just non-NULL. In particular, it implies
>> that is safe to dereference the addressed byte in a source operand even
>> when the length parameter is zero.
>
> I just reread C99 7.1.4 clause 1 and 7.21.2 clause 2, and I don't see
> that implication. For example, the following program appears to be
> strictly conforming:
>
> #include <string.h>
>
> char src[1];
> char dst[1];
>
> int
> main (void)
> {
> memcpy (dst, src + 1, 0);
> return 0;
> }
>
> Here, src + 1 is a valid pointer even though one cannot safely
> dereference it. So it appears to be reasonable to check that memcpy
> doesn't dereference the source when the size is zero.
>
Read clause 1 of 7.1.4 again. "If an argument to a function has an
invalid value ... or a pointer outside of the address space of the
program... the behaviour is undefined."
Ergo, if src+1 can point outside of the address space of the program,
it's undefined behaviour. And by corollary, it must be safe to
dereference the precise location passed as the argument (but obviously,
not necessarily bytes either side of it).
My reading of those sections also leads me to believe that memcpy could
legitimately expect to perform "*(char*)dst = *(char*)dst", even if the
length is zero.
R.
On 23/09/14 13:41, Florian Weimer wrote:
> On 09/22/2014 06:09 PM, Richard Earnshaw wrote:
>>> These tests are not testing null pointers, they are testing that when
>>> given a zero length the functions actually read/write zero bytes.
>>> Whether the specification demands that behaviour is arguable but I
>>> believe that it is the most sane behaviour.
>
>> Valid pointers is more than just non-NULL. In particular, it implies
>> that is safe to dereference the addressed byte in a source operand even
>> when the length parameter is zero.
>
> Valid pointers can also point one element past the end of an array of
> objects.
I don't think such a pointer forms a valid argument for a library
function though. See my previous reply to Paul.
> Such pointers can occur naturally during the final iteration
> of buffer-processing loops. I don't think it is reasonable to expect
> that programmers write special code (or at least early loop exits) to
> deal with this corner case. This has to work, and if the C standard
> does not guarantee it does, it needs fixing.
>
>> Thus testing that no bytes are read would be incorrect.
>
> I disagree, per the above.
>
R.
On 23/09/14 13:57, Richard Earnshaw wrote:
> On 23/09/14 13:41, Florian Weimer wrote:
>> On 09/22/2014 06:09 PM, Richard Earnshaw wrote:
>>>> These tests are not testing null pointers, they are testing that when
>>>> given a zero length the functions actually read/write zero bytes.
>>>> Whether the specification demands that behaviour is arguable but I
>>>> believe that it is the most sane behaviour.
>>
>>> Valid pointers is more than just non-NULL. In particular, it implies
>>> that is safe to dereference the addressed byte in a source operand even
>>> when the length parameter is zero.
>>
>> Valid pointers can also point one element past the end of an array of
>> objects.
>
> I don't think such a pointer forms a valid argument for a library
> function though. See my previous reply to Paul.
>
>> Such pointers can occur naturally during the final iteration
>> of buffer-processing loops. I don't think it is reasonable to expect
>> that programmers write special code (or at least early loop exits) to
>> deal with this corner case. This has to work, and if the C standard
>> does not guarantee it does, it needs fixing.
>>
>>> Thus testing that no bytes are read would be incorrect.
>>
>> I disagree, per the above.
>>
>
> R.
>
Furthermore, I can think of no reason why 7.21.1/2 would explicitly
require valid pointers when the length parameter was 0 unless it was
intended that dereferencing could occur.
R.
Richard Earnshaw <rearnsha@arm.com> writes:
> On 22/09/14 18:48, Paul Eggert wrote:
>> On 09/22/2014 09:09 AM, Richard Earnshaw wrote:
>>> Valid pointers is more than just non-NULL. In particular, it implies
>>> that is safe to dereference the addressed byte in a source operand even
>>> when the length parameter is zero.
>>
>> I just reread C99 7.1.4 clause 1 and 7.21.2 clause 2, and I don't see
>> that implication. For example, the following program appears to be
>> strictly conforming:
>>
>> #include <string.h>
>>
>> char src[1];
>> char dst[1];
>>
>> int
>> main (void)
>> {
>> memcpy (dst, src + 1, 0);
>> return 0;
>> }
>>
>> Here, src + 1 is a valid pointer even though one cannot safely
>> dereference it. So it appears to be reasonable to check that memcpy
>> doesn't dereference the source when the size is zero.
>>
>
> Read clause 1 of 7.1.4 again. "If an argument to a function has an
> invalid value ... or a pointer outside of the address space of the
> program... the behaviour is undefined."
>
> Ergo, if src+1 can point outside of the address space of the program,
> it's undefined behaviour.
src+1 is _not_ outside of the address space. It is a valid pointer
(which you must not dereference).
Andreas.
Richard Earnshaw wrote:
> if src+1 can point outside of the address space of the program
As Andreas points out, src+1 does not point outside the address space of
the program. It is a valid pointer.
> My reading of those sections also leads me to believe that memcpy could
> legitimately expect to perform "*(char*)dst = *(char*)dst", even if the
> length is zero.
I'm sorry, but this reading is incorrect. If the size is zero, memcpy
cannot store any bytes into the destination. Any memcpy that does
otherwise would break a lot of programs.
> I can think of no reason why 7.21.1/2 would explicitly
> require valid pointers when the length parameter was 0 unless it was
> intended that dereferencing could occur.
It caters to (unusual) architectures that require valid pointers in
address registers even when the pointers are not dereferenced, e.g.,
loading a pointer into an address register will trap if the pointer is
invalid.
On 23/09/14 14:57, Paul Eggert wrote:
> Richard Earnshaw wrote:
>
>> if src+1 can point outside of the address space of the program
>
> As Andreas points out, src+1 does not point outside the address space of
> the program. It is a valid pointer.
>
OK, so do we agree that for a valid pointer P, if P is *not*
dereferencable, then P-1 must be? Put another way, if P and P-1 are in
the same 'page' then it is safe to dereference them.
R.
On Tue, Sep 23, 2014 at 06:57:53AM -0700, Paul Eggert wrote:
> Richard Earnshaw wrote:
>
> >if src+1 can point outside of the address space of the program
>
> As Andreas points out, src+1 does not point outside the address
> space of the program. It is a valid pointer.
Agree.
> >My reading of those sections also leads me to believe that memcpy could
> >legitimately expect to perform "*(char*)dst = *(char*)dst", even if the
> >length is zero.
>
> I'm sorry, but this reading is incorrect. If the size is zero,
> memcpy cannot store any bytes into the destination. Any memcpy that
> does otherwise would break a lot of programs.
Note that even if the dest pointer can legally be dereferenced, it's
absolutely illegal in C11 for memcpy to store anything there (even
rewriting the value that's already there) since it would introduce a
data race and violate the memory model requirements. The importance of
this point cannot be overstated: as of C11, all extraneous writes that
may have been careless, inconsequential implementation details in the
past are now serious implementation bugs!
> >I can think of no reason why 7.21.1/2 would explicitly
> >require valid pointers when the length parameter was 0 unless it was
> >intended that dereferencing could occur.
>
> It caters to (unusual) architectures that require valid pointers in
> address registers even when the pointers are not dereferenced, e.g.,
> loading a pointer into an address register will trap if the pointer
> is invalid.
Yes. The C standard is written under the assumption that it's
impossible to work with invalid pointers in any way whatsoever. This
is why the _value_ of _any_ pointer object, wherever it may be stored,
becomes an indeterminate value when the pointed-to object's lifetime
ends, and part of why you can't do things like new=realloc(old,n);
if(new==old)...
Rich
On Tue, Sep 23, 2014 at 03:58:27PM +0100, Richard Earnshaw wrote:
> On 23/09/14 14:57, Paul Eggert wrote:
> > Richard Earnshaw wrote:
> >
> >> if src+1 can point outside of the address space of the program
> >
> > As Andreas points out, src+1 does not point outside the address space of
> > the program. It is a valid pointer.
> >
>
> OK, so do we agree that for a valid pointer P, if P is *not*
> dereferencable, then P-1 must be? Put another way, if P and P-1 are in
> the same 'page' then it is safe to dereference them.
Nope.
struct foo {
char a;
int b[];
}
struct foo *bar = malloc(sizeof *bar);
int *p = (int *)((unsigned char *)bar + offsetof(struct foo, b));
Now neither p nor p-1 is dereferencable, but p is a valid pointer (to
a byte within the representation array of *bar, cast to int*).
Note that the reason I used a flexible array member was to get an
offset that's valid for an object of type int (so that the cast to
int* isn't an alignment violation) but where no object actually
exists.
Rich
On 17 September 2014 00:40, Will Newton <will.newton@linaro.org> wrote:
> For the string functions that take string lengths as an argument we
> should ensure that no data is read or written if a length of zero is
> specified. Pointers to PROT_NONE memory are used to ensure that any
> reads or writes will cause a fault.
>
> ChangeLog:
>
> 2014-09-16 Will Newton <will.newton@linaro.org>
>
> * string/test-memccpy.c (do_test_zero_length): New function.
> (test_main): Call do_test_zero_length.
> * string/test-memchr.c: Likewise.
> * string/test-memcmp.c: Likewise.
> * string/test-memcpy.c: Likewise.
> * string/test-memmem.c: Likewise.
> * string/test-memmove.c: Likewise.
> * string/test-memrchr.c: Likewise.
> * string/test-memset.c: Likewise.
> * string/test-strncmp.c: Likewise.
> * string/test-strncpy.c: Likewise.
> * string/test-strnlen.c: Likewise.
> * string/test-strncasecmp.c (do_test_zero_length): New function.
> (test_locale): Call do_test_zero_length.
> * string/test-strncat.c (do_test_zero_length): New function.
> (main): Call do_test_zero_length.
> ---
> string/test-memccpy.c | 10 ++++++++++
> string/test-memchr.c | 9 +++++++++
> string/test-memcmp.c | 11 +++++++++++
> string/test-memcpy.c | 11 +++++++++++
> string/test-memmem.c | 10 ++++++++++
> string/test-memmove.c | 12 ++++++++++++
> string/test-memrchr.c | 10 ++++++++++
> string/test-memset.c | 11 +++++++++++
> string/test-strncasecmp.c | 15 +++++++++++++++
> string/test-strncat.c | 24 ++++++++++++++++++++++++
> string/test-strncmp.c | 11 +++++++++++
> string/test-strncpy.c | 11 +++++++++++
> string/test-strnlen.c | 10 ++++++++++
> 13 files changed, 155 insertions(+)
Is there any consensus that this patch is useful or should I drop it?
Thanks,
> diff --git a/string/test-memccpy.c b/string/test-memccpy.c
> index 725d640..6192e5e 100644
> --- a/string/test-memccpy.c
> +++ b/string/test-memccpy.c
> @@ -230,6 +230,15 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, buf2 + page_size, buf1 + BUF1PAGES * page_size, 0, 1, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -263,6 +272,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memchr.c b/string/test-memchr.c
> index 0ba79b8..29904d1 100644
> --- a/string/test-memchr.c
> +++ b/string/test-memchr.c
> @@ -141,6 +141,14 @@ do_random_tests (void)
> }
> }
>
> +static void do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size), 'a', 0, NULL);
> +}
> +
> int
> test_main (void)
> {
> @@ -167,6 +175,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memcmp.c b/string/test-memcmp.c
> index 14090ed..80436c6 100644
> --- a/string/test-memcmp.c
> +++ b/string/test-memcmp.c
> @@ -471,6 +471,16 @@ check2 (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (CHAR *) (buf2 + page_size),
> + (CHAR *) (buf1 + BUF1PAGES * page_size), 0, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -519,6 +529,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
> #include "../test-skeleton.c"
> diff --git a/string/test-memcpy.c b/string/test-memcpy.c
> index 136c985..4d2f65b 100644
> --- a/string/test-memcpy.c
> +++ b/string/test-memcpy.c
> @@ -206,6 +206,16 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size),
> + (char *) (buf1 + BUF1PAGES * page_size), 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -247,6 +257,7 @@ test_main (void)
> do_test (0, 0, getpagesize ());
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memmem.c b/string/test-memmem.c
> index caaa191..6199c22 100644
> --- a/string/test-memmem.c
> +++ b/string/test-memmem.c
> @@ -151,6 +151,15 @@ static const char *const strs[] =
> "abc0", "aaaa0", "abcabc0"
> };
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size), 0,
> + strs[0], strlen (strs[0]), NULL);
> +}
>
> int
> test_main (void)
> @@ -178,6 +187,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memmove.c b/string/test-memmove.c
> index 7e1c41c..25abb57 100644
> --- a/string/test-memmove.c
> +++ b/string/test-memmove.c
> @@ -244,6 +244,17 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size),
> + (char *) (buf1 + BUF1PAGES * page_size),
> + (char *) (buf1 + BUF1PAGES * page_size), 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -283,6 +294,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memrchr.c b/string/test-memrchr.c
> index efe4e9f..23c4a8b 100644
> --- a/string/test-memrchr.c
> +++ b/string/test-memrchr.c
> @@ -137,6 +137,15 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size), 'a', 0, NULL);
> +}
> +
> int
> test_main (void)
> {
> @@ -163,6 +172,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-memset.c b/string/test-memset.c
> index 2171b0d..b5432ce 100644
> --- a/string/test-memset.c
> +++ b/string/test-memset.c
> @@ -192,6 +192,15 @@ do_random_tests (void)
> }
> #endif
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size), 0, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -227,6 +236,8 @@ test_main (void)
> do_random_tests ();
> #endif
>
> + do_test_zero_length ();
> +
> return ret;
> }
>
> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c
> index 6ad54e0..78d2dec 100644
> --- a/string/test-strncasecmp.c
> +++ b/string/test-strncasecmp.c
> @@ -53,9 +53,13 @@ simple_strncasecmp (const char *s1, const char *s2, size_t n)
> static int
> stupid_strncasecmp (const char *s1, const char *s2, size_t max)
> {
> + if (max == 0)
> + return 0;
> +
> size_t ns1 = strlen (s1) + 1;
> size_t ns2 = strlen (s2) + 1;
> size_t n = ns1 < ns2 ? ns1 : ns2;
> +
> if (n > max)
> n = max;
> int ret = 0;
> @@ -258,6 +262,16 @@ bz14195 (void)
> }
>
> static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size),
> + (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
> +}
> +
> +static void
> test_locale (const char *locale)
> {
> size_t i;
> @@ -270,6 +284,7 @@ test_locale (const char *locale)
>
> bz12205 ();
> bz14195 ();
> + do_test_zero_length ();
>
> printf ("%23s", locale);
> FOR_EACH_IMPL (impl, 0)
> diff --git a/string/test-strncat.c b/string/test-strncat.c
> index 4915c59..4eef967 100644
> --- a/string/test-strncat.c
> +++ b/string/test-strncat.c
> @@ -31,6 +31,10 @@ char *
> stupid_strncat (char *dst, const char *src, size_t n)
> {
> char *ret = dst;
> +
> + if (n == 0)
> + return ret;
> +
> while (*dst++ != '\0');
> --dst;
> while (n--)
> @@ -232,6 +236,25 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + char dst[1];
> + char *src = (char *) (buf2 + page_size);
> + dst[0] = '\0';
> +
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + if (CALL (impl, dst, src, 0) != dst)
> + {
> + error (0, 0, "Wrong result in function %s %p != %p", impl->name,
> + CALL (impl, dst, src, 0), dst);
> + ret = 1;
> + return;
> + }
> +}
> +
> int
> main (void)
> {
> @@ -269,5 +292,6 @@ main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
> diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> index f3b2c68..225bab5 100644
> --- a/string/test-strncmp.c
> +++ b/string/test-strncmp.c
> @@ -317,6 +317,16 @@ check2 (void)
> free (s2);
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size),
> + (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -387,6 +397,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
> index 2326acc..455da43 100644
> --- a/string/test-strncpy.c
> +++ b/string/test-strncpy.c
> @@ -245,6 +245,16 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointers to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size),
> + (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -278,6 +288,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> diff --git a/string/test-strnlen.c b/string/test-strnlen.c
> index be9edd2..c8fd05a 100644
> --- a/string/test-strnlen.c
> +++ b/string/test-strnlen.c
> @@ -122,6 +122,15 @@ do_random_tests (void)
> }
> }
>
> +static void
> +do_test_zero_length (void)
> +{
> + /* Test for behaviour with zero length and pointer to PROT_NONE
> + memory. */
> + FOR_EACH_IMPL (impl, 0)
> + do_one_test (impl, (char *) (buf2 + page_size), 0, 0);
> +}
> +
> int
> test_main (void)
> {
> @@ -167,6 +176,7 @@ test_main (void)
> }
>
> do_random_tests ();
> + do_test_zero_length ();
> return ret;
> }
>
> --
> 1.9.3
>
On 09/23/2014 02:57 PM, Richard Earnshaw wrote:
> On 23/09/14 13:41, Florian Weimer wrote:
>> On 09/22/2014 06:09 PM, Richard Earnshaw wrote:
>>>> These tests are not testing null pointers, they are testing that when
>>>> given a zero length the functions actually read/write zero bytes.
>>>> Whether the specification demands that behaviour is arguable but I
>>>> believe that it is the most sane behaviour.
>>
>>> Valid pointers is more than just non-NULL. In particular, it implies
>>> that is safe to dereference the addressed byte in a source operand even
>>> when the length parameter is zero.
>>
>> Valid pointers can also point one element past the end of an array of
>> objects.
>
> I don't think such a pointer forms a valid argument for a library
> function though. See my previous reply to Paul.
They are a fairly common occurrence with the [first, last) iterator
ranges in C++. It's common to compute a pointer/length pair {first,
last - first} and pass that to C functions, including C library functions.
This pattern is already incorrect in important corner cases (e.g.,
iterators derived from empty vectors), but do we really have to make the
situation even worse?
On 10/15/2014 11:41 AM, Will Newton wrote:
> On 17 September 2014 00:40, Will Newton <will.newton@linaro.org> wrote:
>> For the string functions that take string lengths as an argument we
>> should ensure that no data is read or written if a length of zero is
>> specified. Pointers to PROT_NONE memory are used to ensure that any
>> reads or writes will cause a fault.
> Is there any consensus that this patch is useful or should I drop it?
I, for one, think the patch is useful.
@@ -230,6 +230,15 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, buf2 + page_size, buf1 + BUF1PAGES * page_size, 0, 1, 0);
+}
+
int
test_main (void)
{
@@ -263,6 +272,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -141,6 +141,14 @@ do_random_tests (void)
}
}
+static void do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size), 'a', 0, NULL);
+}
+
int
test_main (void)
{
@@ -167,6 +175,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -471,6 +471,16 @@ check2 (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (CHAR *) (buf2 + page_size),
+ (CHAR *) (buf1 + BUF1PAGES * page_size), 0, 0);
+}
+
int
test_main (void)
{
@@ -519,6 +529,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
#include "../test-skeleton.c"
@@ -206,6 +206,16 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size),
+ (char *) (buf1 + BUF1PAGES * page_size), 0);
+}
+
int
test_main (void)
{
@@ -247,6 +257,7 @@ test_main (void)
do_test (0, 0, getpagesize ());
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -151,6 +151,15 @@ static const char *const strs[] =
"abc0", "aaaa0", "abcabc0"
};
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size), 0,
+ strs[0], strlen (strs[0]), NULL);
+}
int
test_main (void)
@@ -178,6 +187,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -244,6 +244,17 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size),
+ (char *) (buf1 + BUF1PAGES * page_size),
+ (char *) (buf1 + BUF1PAGES * page_size), 0);
+}
+
int
test_main (void)
{
@@ -283,6 +294,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -137,6 +137,15 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size), 'a', 0, NULL);
+}
+
int
test_main (void)
{
@@ -163,6 +172,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -192,6 +192,15 @@ do_random_tests (void)
}
#endif
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size), 0, 0);
+}
+
int
test_main (void)
{
@@ -227,6 +236,8 @@ test_main (void)
do_random_tests ();
#endif
+ do_test_zero_length ();
+
return ret;
}
@@ -53,9 +53,13 @@ simple_strncasecmp (const char *s1, const char *s2, size_t n)
static int
stupid_strncasecmp (const char *s1, const char *s2, size_t max)
{
+ if (max == 0)
+ return 0;
+
size_t ns1 = strlen (s1) + 1;
size_t ns2 = strlen (s2) + 1;
size_t n = ns1 < ns2 ? ns1 : ns2;
+
if (n > max)
n = max;
int ret = 0;
@@ -258,6 +262,16 @@ bz14195 (void)
}
static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size),
+ (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
+}
+
+static void
test_locale (const char *locale)
{
size_t i;
@@ -270,6 +284,7 @@ test_locale (const char *locale)
bz12205 ();
bz14195 ();
+ do_test_zero_length ();
printf ("%23s", locale);
FOR_EACH_IMPL (impl, 0)
@@ -31,6 +31,10 @@ char *
stupid_strncat (char *dst, const char *src, size_t n)
{
char *ret = dst;
+
+ if (n == 0)
+ return ret;
+
while (*dst++ != '\0');
--dst;
while (n--)
@@ -232,6 +236,25 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ char dst[1];
+ char *src = (char *) (buf2 + page_size);
+ dst[0] = '\0';
+
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ if (CALL (impl, dst, src, 0) != dst)
+ {
+ error (0, 0, "Wrong result in function %s %p != %p", impl->name,
+ CALL (impl, dst, src, 0), dst);
+ ret = 1;
+ return;
+ }
+}
+
int
main (void)
{
@@ -269,5 +292,6 @@ main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -317,6 +317,16 @@ check2 (void)
free (s2);
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size),
+ (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
+}
+
int
test_main (void)
{
@@ -387,6 +397,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -245,6 +245,16 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointers to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size),
+ (char *) (buf1 + BUF1PAGES * page_size), 0, 0);
+}
+
int
test_main (void)
{
@@ -278,6 +288,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}
@@ -122,6 +122,15 @@ do_random_tests (void)
}
}
+static void
+do_test_zero_length (void)
+{
+ /* Test for behaviour with zero length and pointer to PROT_NONE
+ memory. */
+ FOR_EACH_IMPL (impl, 0)
+ do_one_test (impl, (char *) (buf2 + page_size), 0, 0);
+}
+
int
test_main (void)
{
@@ -167,6 +176,7 @@ test_main (void)
}
do_random_tests ();
+ do_test_zero_length ();
return ret;
}