[v1] x86: memcmp-avx2-movbe.S and memcmp-evex-movbe.S fix overflow bug.
Checks
Commit Message
Fix bugs introducted in commits:
author Noah Goldstein <goldstein.w.n@gmail.com>
Mon, 17 May 2021 17:57:24 +0000 (13:57 -0400)
commit 4ad473e97acdc5f6d811755b67c09f2128a644ce
And
author Noah Goldstein <goldstein.w.n@gmail.com>
Mon, 17 May 2021 17:56:52 +0000 (13:56 -0400)
commit 16d12015c57701b08d7bbed6ec536641bcafb428
Which added a bug which would cause pointer + length overflow to lead
to an early return as opposed to a Segmentation Fault.
Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
---
sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S | 13 ++++++++-----
sysdeps/x86_64/multiarch/memcmp-evex-movbe.S | 15 ++++++++-------
2 files changed, 16 insertions(+), 12 deletions(-)
Comments
On Mon, Jun 7, 2021 at 1:30 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> Fix bugs introducted in commits:
>
> author Noah Goldstein <goldstein.w.n@gmail.com>
> Mon, 17 May 2021 17:57:24 +0000 (13:57 -0400)
> commit 4ad473e97acdc5f6d811755b67c09f2128a644ce
>
> And
>
> author Noah Goldstein <goldstein.w.n@gmail.com>
> Mon, 17 May 2021 17:56:52 +0000 (13:56 -0400)
> commit 16d12015c57701b08d7bbed6ec536641bcafb428
>
> Which added a bug which would cause pointer + length overflow to lead
> to an early return as opposed to a Segmentation Fault.
>
> Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
> ---
> sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S | 13 ++++++++-----
> sysdeps/x86_64/multiarch/memcmp-evex-movbe.S | 15 ++++++++-------
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
Please open glibc bugs for both memcmp and memset overflow issue.
Thanks.
On 6/7/21 2:00 PM, Noah Goldstein via Libc-alpha wrote:
> Fix bugs introducted in commits:
>
> author Noah Goldstein <goldstein.w.n@gmail.com>
> Mon, 17 May 2021 17:57:24 +0000 (13:57 -0400)
> commit 4ad473e97acdc5f6d811755b67c09f2128a644ce
>
> And
>
> author Noah Goldstein <goldstein.w.n@gmail.com>
> Mon, 17 May 2021 17:56:52 +0000 (13:56 -0400)
> commit 16d12015c57701b08d7bbed6ec536641bcafb428
>
> Which added a bug which would cause pointer + length overflow to lead
> to an early return as opposed to a Segmentation Fault.
If we end up making this change, IMO it should come with an explicit
note that this behaviour is not guaranteed for invalid inputs in other
implementations of memcmp or for that matter, in future versions of this
memcmp.
An input that causes pointer + length overflow is undefined behaviour
and IMO we shouldn't try to define it for glibc. This change may not
have a noticeable performance impact but future requests to guarantee
this behaviour may not necessarily be this straightforward and IMO we
should not bind ourselves to it.
Siddhesh
On Mon, Jun 7, 2021 at 9:45 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jun 7, 2021 at 1:30 AM Noah Goldstein <goldstein.w.n@gmail.com>
> wrote:
> >
> > Fix bugs introducted in commits:
> >
> > author Noah Goldstein <goldstein.w.n@gmail.com>
> > Mon, 17 May 2021 17:57:24 +0000 (13:57 -0400)
> > commit 4ad473e97acdc5f6d811755b67c09f2128a644ce
> >
> > And
> >
> > author Noah Goldstein <goldstein.w.n@gmail.com>
> > Mon, 17 May 2021 17:56:52 +0000 (13:56 -0400)
> > commit 16d12015c57701b08d7bbed6ec536641bcafb428
> >
> > Which added a bug which would cause pointer + length overflow to lead
> > to an early return as opposed to a Segmentation Fault.
> >
> > Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
> > ---
> > sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S | 13 ++++++++-----
> > sysdeps/x86_64/multiarch/memcmp-evex-movbe.S | 15 ++++++++-------
> > 2 files changed, 16 insertions(+), 12 deletions(-)
> >
>
> Please open glibc bugs for both memcmp and memset overflow issue.
>
Done:
Memcmp: https://sourceware.org/bugzilla/show_bug.cgi?id=27961
Memset: https://sourceware.org/bugzilla/show_bug.cgi?id=27960
>
> Thanks.
>
> --
> H.J.
>
On Mon, Jun 7, 2021 at 10:21 AM Siddhesh Poyarekar <siddhesh@gotplt.org>
wrote:
> On 6/7/21 2:00 PM, Noah Goldstein via Libc-alpha wrote:
> > Fix bugs introducted in commits:
> >
> > author Noah Goldstein <goldstein.w.n@gmail.com>
> > Mon, 17 May 2021 17:57:24 +0000 (13:57 -0400)
> > commit 4ad473e97acdc5f6d811755b67c09f2128a644ce
> >
> > And
> >
> > author Noah Goldstein <goldstein.w.n@gmail.com>
> > Mon, 17 May 2021 17:56:52 +0000 (13:56 -0400)
> > commit 16d12015c57701b08d7bbed6ec536641bcafb428
> >
> > Which added a bug which would cause pointer + length overflow to lead
> > to an early return as opposed to a Segmentation Fault.
>
> If we end up making this change, IMO it should come with an explicit
> note that this behaviour is not guaranteed for invalid inputs in other
> implementations of memcmp or for that matter, in future versions of this
> memcmp.
>
> An input that causes pointer + length overflow is undefined behaviour
> and IMO we shouldn't try to define it for glibc.
Is it actually UB? The caller is not causing overflow. The implementation
method is. It is possible to implement without overflow.
> This change may not
> have a noticeable performance impact
Minor impact actually for memcmp.
> but future requests to guarantee
> this behaviour may not necessarily be this straightforward and IMO we
> should not bind ourselves to it.
Agreed.
>
> Siddhesh
>
On 6/7/21 10:28 AM, Noah Goldstein via Libc-alpha wrote:
>
> Is it actually UB? The caller is not causing overflow. The implementation
> method is. It is possible to implement without overflow.
The C Standard says that unless otherwise specified, when you pass an
array (pointer + size) to a standard function, all the addresses in the
array must be valid. It's valid if (say) memcmp is multithreaded and
compares the first halves of the two arrays in parallel with comparing
the second halves.
If I understand things correctly this patch isn't fixing a conformance
bug; it's merely a QoI issue, where by "quality" one means "I want this
particular undefined behavior to cause a core dump".
On Mon, Jun 7, 2021 at 1:45 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 6/7/21 10:28 AM, Noah Goldstein via Libc-alpha wrote:
> >
> > Is it actually UB? The caller is not causing overflow. The implementation
> > method is. It is possible to implement without overflow.
>
> The C Standard says that unless otherwise specified, when you pass an
> array (pointer + size) to a standard function, all the addresses in the
> array must be valid. It's valid if (say) memcmp is multithreaded and
> compares the first halves of the two arrays in parallel with comparing
> the second halves.
> If I understand things correctly this patch isn't fixing a conformance
> bug; it's merely a QoI issue, where by "quality" one means "I want this
> particular undefined behavior to cause a core dump".
>
I see. Good to hear there wasn't technically a bug :)
What do we want the behavior to be? Generally I would think
failing silently can be quite painful for users.
On 6/7/21 11:07 AM, Noah Goldstein wrote:
> What do we want the behavior to be? Generally I would think
> failing silently can be quite painful for users.
Traditionally we have typically said to go for the best typical-case
performance, and not to worry about how that affects undefined behavior.
So, for example, glibc's memcmp ((void *) 1, NULL, 0) does not dump core
even though its behavior is undefined, because it would slow glibc down
to force memcmp to dump core for that case.
A debugging library like valgrind's might choose to crash more reliably.
On Mon, Jun 7, 2021 at 3:51 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 6/7/21 11:07 AM, Noah Goldstein wrote:
> > What do we want the behavior to be? Generally I would think
> > failing silently can be quite painful for users.
>
> Traditionally we have typically said to go for the best typical-case
> performance, and not to worry about how that affects undefined behavior.
>
That works for me. The old versions are slightly faster / use less code.
Panicked a bit last night when I thought I might have pushed a bug :/
>
> So, for example, glibc's memcmp ((void *) 1, NULL, 0) does not dump core
> even though its behavior is undefined, because it would slow glibc down
> to force memcmp to dump core for that case.
>
> A debugging library like valgrind's might choose to crash more reliably.
>
On Mon, Jun 7, 2021 at 1:45 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 6/7/21 10:28 AM, Noah Goldstein via Libc-alpha wrote:
> >
> > Is it actually UB? The caller is not causing overflow. The implementation
> > method is. It is possible to implement without overflow.
>
> The C Standard says that unless otherwise specified, when you pass an
> array (pointer + size) to a standard function, all the addresses in the
> array must be valid. It's valid if (say) memcmp is multithreaded and
> compares the first halves of the two arrays in parallel with comparing
> the second halves.
>
Does this mean there is an issue with functions like wcsnlen?
They do say that they only need to be able to access memory up
to first null terminator but currently the x86_64 wcsnlen-avx2.S
implementation will multiple length by 4 which could cause overflow.
For example with a string length 1000 and maxlen passed as
2^62 + 1 the return will be 1.
Is that an issue? It's a pretty simple fix although I think this idiom
of multiply length by 4 to get byte count is in a lot of similarly specified
files.
>
> If I understand things correctly this patch isn't fixing a conformance
> bug; it's merely a QoI issue, where by "quality" one means "I want this
> particular undefined behavior to cause a core dump".
>
On 6/9/21 10:45 AM, Noah Goldstein wrote:
>
>
> On Mon, Jun 7, 2021 at 1:45 PM Paul Eggert <eggert@cs.ucla.edu
> <mailto:eggert@cs.ucla.edu>> wrote:
>
> On 6/7/21 10:28 AM, Noah Goldstein via Libc-alpha wrote:
> >
> > Is it actually UB? The caller is not causing overflow. The
> implementation
> > method is. It is possible to implement without overflow.
>
> The C Standard says that unless otherwise specified, when you pass an
> array (pointer + size) to a standard function, all the addresses in the
> array must be valid. It's valid if (say) memcmp is multithreaded and
> compares the first halves of the two arrays in parallel with comparing
> the second halves.
>
>
> Does this mean there is an issue with functions like wcsnlen?
>
> They do say that they only need to be able to access memory up
> to first null terminator but currently the x86_64 wcsnlen-avx2.S
> implementation will multiple length by 4 which could cause overflow.
>
> For example with a string length 1000 and maxlen passed as
> 2^62 + 1 the return will be 1.
The maxlen causes undefined behaviour; it is the responsibility of the
caller to ensure that it doesn't and the way to do that for wcsnlen is
to ensure that s+maxlen*sizeof(wchar_t) is within bounds of the object
and hence does not cause overflow.
Siddhesh
On Wed, Jun 9, 2021 at 1:25 AM Siddhesh Poyarekar <siddhesh@gotplt.org>
wrote:
> On 6/9/21 10:45 AM, Noah Goldstein wrote:
> >
> >
> > On Mon, Jun 7, 2021 at 1:45 PM Paul Eggert <eggert@cs.ucla.edu
> > <mailto:eggert@cs.ucla.edu>> wrote:
> >
> > On 6/7/21 10:28 AM, Noah Goldstein via Libc-alpha wrote:
> > >
> > > Is it actually UB? The caller is not causing overflow. The
> > implementation
> > > method is. It is possible to implement without overflow.
> >
> > The C Standard says that unless otherwise specified, when you pass an
> > array (pointer + size) to a standard function, all the addresses in
> the
> > array must be valid. It's valid if (say) memcmp is multithreaded and
> > compares the first halves of the two arrays in parallel with
> comparing
> > the second halves.
> >
> >
> > Does this mean there is an issue with functions like wcsnlen?
> >
> > They do say that they only need to be able to access memory up
> > to first null terminator but currently the x86_64 wcsnlen-avx2.S
> > implementation will multiple length by 4 which could cause overflow.
> >
> > For example with a string length 1000 and maxlen passed as
> > 2^62 + 1 the return will be 1.
>
> The maxlen causes undefined behaviour; it is the responsibility of the
> caller to ensure that it doesn't and the way to do that for wcsnlen is
> to ensure that s+maxlen*sizeof(wchar_t) is within bounds of the object
> and hence does not cause overflow.
> Siddhesh
>
Got it and thanks! Sorry for all the questions.
Was a bit thrown off because we have overflow tests for strncat / strnlen
so it appears we are testing UB.
On 6/9/21 11:13 AM, Noah Goldstein wrote:
> Got it and thanks! Sorry for all the questions.
>
> Was a bit thrown off because we have overflow tests for strncat / strnlen
> so it appears we are testing UB.
Hmm, I couldn't spot any overflow tests from a quick skim; there are
tests that set maxlen as SIZE_MAX (but should terminate before reaching
SIZE_MAX and hence not cause an overflow) and page boundary tests in
strnlen, but none of them should result in undefined behaviour. Could
you point out the ones that invoke undefined behaviour?
Thanks,
Siddhesh
On Wed, Jun 9, 2021 at 2:02 AM Siddhesh Poyarekar <siddhesh@gotplt.org>
wrote:
> On 6/9/21 11:13 AM, Noah Goldstein wrote:
> > Got it and thanks! Sorry for all the questions.
> >
> > Was a bit thrown off because we have overflow tests for strncat / strnlen
> > so it appears we are testing UB.
>
> Hmm, I couldn't spot any overflow tests from a quick skim; there are
> tests that set maxlen as SIZE_MAX (but should terminate before reaching
> SIZE_MAX and hence not cause an overflow) and page boundary tests in
> strnlen, but none of them should result in undefined behaviour. Could
> you point out the ones that invoke undefined behaviour?
>
Wait are you saying that the return value overflowing causes UB or
that the act of passing a maxlen where s + maxlen * sizeof(wchar_t) is
outside range object is UB?
If the former then why is the follow okay:
Previous example with a string whose length is 1000
but because wcslen is passed maxlen where maxlen * sizeof(wchar_t)
overflows and leads to a result less than 1000 the implementation of
wcslen in wcsnlen-avx2.S will return a length less than 1000.
If the latter then:
For test-wcsnlen which redirects to test-strnlen
If the UB is when is s+maxlen*sizeof(wchar_t) is outside object bound
then s + SIZE_MAX + sizeof(wchar_t) surely is.
Although even then test-strnlen s + SIZE_MAX will also overflow if s non
null.
>
> Thanks,
> Siddhesh
>
On 6/9/21 12:02 PM, Noah Goldstein wrote:
> Wait are you saying that the return value overflowing causes UB or
> that the act of passing a maxlen where s + maxlen * sizeof(wchar_t) is
> outside range object is UB?
Not specifically the return value, but more broadly, causing wcsnlen to
invoke undefined behaviour, which could include the former.
> If the former then why is the follow okay:
>
> Previous example with a string whose length is 1000
> but because wcslen is passed maxlen where maxlen * sizeof(wchar_t)
> overflows and leads to a result less than 1000 the implementation of
> wcslen in wcsnlen-avx2.S will return a length less than 1000.
>
>
> If the latter then:
>
> For test-wcsnlen which redirects to test-strnlen
> If the UB is when is s+maxlen*sizeof(wchar_t) is outside object bound
> then s + SIZE_MAX + sizeof(wchar_t) surely is.
>
> Although even then test-strnlen s + SIZE_MAX will also overflow if s non
> null.
That is a good catch; I did not notice that. That should be SIZE_MAX /
sizeof (CHAR).
Thanks,
Siddhesh
On Wed, Jun 9, 2021 at 2:48 AM Siddhesh Poyarekar <siddhesh@gotplt.org>
wrote:
> On 6/9/21 12:02 PM, Noah Goldstein wrote:
> > Wait are you saying that the return value overflowing causes UB or
> > that the act of passing a maxlen where s + maxlen * sizeof(wchar_t) is
> > outside range object is UB?
>
> Not specifically the return value, but more broadly, causing wcsnlen to
> invoke undefined behaviour, which could include the former.
>
> > If the former then why is the follow okay:
> >
> > Previous example with a string whose length is 1000
> > but because wcslen is passed maxlen where maxlen * sizeof(wchar_t)
> > overflows and leads to a result less than 1000 the implementation of
> > wcslen in wcsnlen-avx2.S will return a length less than 1000.
> >
> >
> > If the latter then:
> >
> > For test-wcsnlen which redirects to test-strnlen
> > If the UB is when is s+maxlen*sizeof(wchar_t) is outside object bound
> > then s + SIZE_MAX + sizeof(wchar_t) surely is.
> >
> > Although even then test-strnlen s + SIZE_MAX will also overflow if s non
> > null.
>
> That is a good catch; I did not notice that. That should be SIZE_MAX /
> sizeof (CHAR).
>
Do we want to support s + maxlen + sizeof(CHAR) overflowing? If not we can
speed up the AVX2/EVEX implementation of strnlen/wcsnlen/memchr/wmemchr.
(Assuming memchr is of same standard where s + n + sizeof(CHAR)
can't overflow. If not it has this bug).
> Thanks,
> Siddhesh
>
On 6/9/21 12:24 PM, Noah Goldstein wrote:
> Do we want to support s + maxlen + sizeof(CHAR) overflowing? If not we can
> speed up the AVX2/EVEX implementation of strnlen/wcsnlen/memchr/wmemchr.
We don't want to specify any behaviour that is undefined by the standard
so it's perfectly OK for the algorithm to assume that s + (maxlen - 1) *
sizeof(CHAR) (off by one in my previous comment, sorry :)) points to a
valid address if that makes the function go faster.
Siddhesh
On 6/9/21 12:31 PM, Siddhesh Poyarekar wrote:
> On 6/9/21 12:24 PM, Noah Goldstein wrote:
>> Do we want to support s + maxlen + sizeof(CHAR) overflowing? If not we
>> can
>> speed up the AVX2/EVEX implementation of strnlen/wcsnlen/memchr/wmemchr.
>
> We don't want to specify any behaviour that is undefined by the standard
> so it's perfectly OK for the algorithm to assume that s + (maxlen - 1) *
> sizeof(CHAR) (off by one in my previous comment, sorry :)) points to a
> valid address if that makes the function go faster.
s/valid address/valid reference in the object referred to by s/
In fact, like implementations for strlen, it is OK to assume that reads
beyond the object bounds are also OK as long as they're within the last
page that s is in. Likewise for reads before object bounds as long as
they're within the first page that s is in.
Siddhesh
On Jun 09 2021, Siddhesh Poyarekar wrote:
> On 6/9/21 12:02 PM, Noah Goldstein wrote:
>> Wait are you saying that the return value overflowing causes UB or
>> that the act of passing a maxlen where s + maxlen * sizeof(wchar_t) is
>> outside range object is UB?
>
> Not specifically the return value, but more broadly, causing wcsnlen to
> invoke undefined behaviour, which could include the former.
I don't think wcsnlen can assume that such an overflow doesn't happen.
Andreas.
On 6/9/21 1:09 PM, Andreas Schwab wrote:
> On Jun 09 2021, Siddhesh Poyarekar wrote:
>
>> On 6/9/21 12:02 PM, Noah Goldstein wrote:
>>> Wait are you saying that the return value overflowing causes UB or
>>> that the act of passing a maxlen where s + maxlen * sizeof(wchar_t) is
>>> outside range object is UB?
>>
>> Not specifically the return value, but more broadly, causing wcsnlen to
>> invoke undefined behaviour, which could include the former.
>
> I don't think wcsnlen can assume that such an overflow doesn't happen.
Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least the
one I have from 2011) and is defined in POSIX. The description doesn't
seem to specify any access semantics for wcsnlen + maxlen. Are you
referring to the fact that it's unspecified or are you aware of anywhere
else in the spec that requires the implementation to ensure valid access?
Siddhesh
On Jun 09 2021, Siddhesh Poyarekar wrote:
> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least the
> one I have from 2011) and is defined in POSIX. The description doesn't
> seem to specify any access semantics for wcsnlen + maxlen. Are you
> referring to the fact that it's unspecified or are you aware of anywhere
> else in the spec that requires the implementation to ensure valid access?
Does the sentence "The wcsnlen() function shall never examine more than
the first maxlen characters of the wide-character array pointed to by
ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
Andreas.
* Andreas Schwab:
> On Jun 09 2021, Siddhesh Poyarekar wrote:
>
>> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least the
>> one I have from 2011) and is defined in POSIX. The description doesn't
>> seem to specify any access semantics for wcsnlen + maxlen. Are you
>> referring to the fact that it's unspecified or are you aware of anywhere
>> else in the spec that requires the implementation to ensure valid access?
>
> Does the sentence "The wcsnlen() function shall never examine more than
> the first maxlen characters of the wide-character array pointed to by
> ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
Is this bug related?
wcsrtombs calls wcsnlen on input data which is not an array
<https://sourceware.org/bugzilla/show_bug.cgi?id=23711>
Thanks,
Florian
On 6/9/21 2:44 PM, Andreas Schwab wrote:
> On Jun 09 2021, Siddhesh Poyarekar wrote:
>
>> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least the
>> one I have from 2011) and is defined in POSIX. The description doesn't
>> seem to specify any access semantics for wcsnlen + maxlen. Are you
>> referring to the fact that it's unspecified or are you aware of anywhere
>> else in the spec that requires the implementation to ensure valid access?
>
> Does the sentence "The wcsnlen() function shall never examine more than
> the first maxlen characters of the wide-character array pointed to by
> ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
It does not specify any of that; that's what I said. My question was:
do you think we should interpret that as a constraint the implementation
has to take care of or are you aware of another text in the standard
that makes it clearer?
Siddhesh
On 6/9/21 2:50 PM, Florian Weimer wrote:
> Is this bug related?
>
> wcsrtombs calls wcsnlen on input data which is not an array
> <https://sourceware.org/bugzilla/show_bug.cgi?id=23711>
Your interpretation that the input has to be an array of size maxlen
appears to put constraints on maxlen.
Siddhesh
On Jun 09 2021, Siddhesh Poyarekar wrote:
> On 6/9/21 2:44 PM, Andreas Schwab wrote:
>> On Jun 09 2021, Siddhesh Poyarekar wrote:
>>
>>> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least the
>>> one I have from 2011) and is defined in POSIX. The description doesn't
>>> seem to specify any access semantics for wcsnlen + maxlen. Are you
>>> referring to the fact that it's unspecified or are you aware of anywhere
>>> else in the spec that requires the implementation to ensure valid access?
>> Does the sentence "The wcsnlen() function shall never examine more than
>> the first maxlen characters of the wide-character array pointed to by
>> ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
>
> It does not specify any of that; that's what I said.
Then you cannot assume that maxlen*sizeof(wchar_t) does not wrap around,
and using that as a limit would be a bug.
Andreas.
* Siddhesh Poyarekar:
> On 6/9/21 2:50 PM, Florian Weimer wrote:
>> Is this bug related?
>> wcsrtombs calls wcsnlen on input data which is not an array
>> <https://sourceware.org/bugzilla/show_bug.cgi?id=23711>
>
> Your interpretation that the input has to be an array of size maxlen
> appears to put constraints on maxlen.
But I think I later realized that GNU supports non-array use as an
extension, and that would mean that there is no such constraint.
Thanks,
Florian
On 6/9/21 3:07 PM, Florian Weimer wrote:
> * Siddhesh Poyarekar:
>
>> On 6/9/21 2:50 PM, Florian Weimer wrote:
>>> Is this bug related?
>>> wcsrtombs calls wcsnlen on input data which is not an array
>>> <https://sourceware.org/bugzilla/show_bug.cgi?id=23711>
>>
>> Your interpretation that the input has to be an array of size maxlen
>> appears to put constraints on maxlen.
>
> But I think I later realized that GNU supports non-array use as an
> extension, and that would mean that there is no such constraint.
OK got it.
Thanks,
Siddhesh
On 6/9/21 3:05 PM, Andreas Schwab wrote:
> Then you cannot assume that maxlen*sizeof(wchar_t) does not wrap around,
> and using that as a limit would be a bug.
Thanks for clarifying.
Siddhesh
So do we need a patch to fix this?
I see the issue of assuming maxlen * sizeof(wchar_t) does not wrap
in the following files:
wmemchr-sse4_1
wmemchr-avx2
wcsnlen-sse2
wcsnlen-avx2
I can get one ready pretty easily if we do need one.
On Wed, Jun 9, 2021 at 5:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jun 09 2021, Siddhesh Poyarekar wrote:
>
> > On 6/9/21 2:44 PM, Andreas Schwab wrote:
> >> On Jun 09 2021, Siddhesh Poyarekar wrote:
> >>
> >>> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least
> the
> >>> one I have from 2011) and is defined in POSIX. The description doesn't
> >>> seem to specify any access semantics for wcsnlen + maxlen. Are you
> >>> referring to the fact that it's unspecified or are you aware of
> anywhere
> >>> else in the spec that requires the implementation to ensure valid
> access?
> >> Does the sentence "The wcsnlen() function shall never examine more than
> >> the first maxlen characters of the wide-character array pointed to by
> >> ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
> >
> > It does not specify any of that; that's what I said.
>
> Then you cannot assume that maxlen*sizeof(wchar_t) does not wrap around,
> and using that as a limit would be a bug.
>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
>
On Wed, Jun 9, 2021 at 1:34 PM Noah Goldstein via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> So do we need a patch to fix this?
>
> I see the issue of assuming maxlen * sizeof(wchar_t) does not wrap
> in the following files:
>
> wmemchr-sse4_1
> wmemchr-avx2
>
> wcsnlen-sse2
> wcsnlen-avx2
>
> I can get one ready pretty easily if we do need one.
Can we create tests without undefined behavior to show the current
implementation is wrong? If yes, please add such testcases and fix
the implementations.
Thanks.
>
> On Wed, Jun 9, 2021 at 5:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> > On Jun 09 2021, Siddhesh Poyarekar wrote:
> >
> > > On 6/9/21 2:44 PM, Andreas Schwab wrote:
> > >> On Jun 09 2021, Siddhesh Poyarekar wrote:
> > >>
> > >>> Hmm, I just noticed that wcsnlen is not in the ISO C draft (at least
> > the
> > >>> one I have from 2011) and is defined in POSIX. The description doesn't
> > >>> seem to specify any access semantics for wcsnlen + maxlen. Are you
> > >>> referring to the fact that it's unspecified or are you aware of
> > anywhere
> > >>> else in the spec that requires the implementation to ensure valid
> > access?
> > >> Does the sentence "The wcsnlen() function shall never examine more than
> > >> the first maxlen characters of the wide-character array pointed to by
> > >> ws." constitute a limit on maxlen, or that ws+maxlen must be valid?
> > >
> > > It does not specify any of that; that's what I said.
> >
> > Then you cannot assume that maxlen*sizeof(wchar_t) does not wrap around,
> > and using that as a limit would be a bug.
> >
> > Andreas.
> >
> > --
> > Andreas Schwab, schwab@linux-m68k.org
> > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> > "And now for something completely different."
> >
On 6/10/21 2:03 AM, Noah Goldstein wrote:
> So do we need a patch to fix this?
>
We don't. As Andreas clarified, wcsnlen behaviour avoids overflow.
Siddhesh
On 6/10/21 2:03 AM, Noah Goldstein wrote:
> So do we need a patch to fix this?
>
> I see the issue of assuming maxlen * sizeof(wchar_t) does not wrap
> in the following files:
>
> wmemchr-sse4_1
> wmemchr-avx2
>
> wcsnlen-sse2
> wcsnlen-avx2
>
> I can get one ready pretty easily if we do need one.
Sorry I misunderstood your question. H.J. has the right advice for you.
Thanks,
Siddhesh
@@ -60,6 +60,7 @@
# endif
# define VEC_SIZE 32
+# define LOG_VEC_SIZE 7
# define PAGE_SIZE 4096
/* Warning!
@@ -200,8 +201,7 @@ L(return_vec_2):
# endif
VZEROUPPER_RETURN
- /* NB: p2align 5 here to ensure 4x loop is 32 byte aligned. */
- .p2align 5
+ .p2align 4
L(8x_return_vec_0_1_2_3):
/* Returning from L(more_8x_vec) requires restoring rsi. */
addq %rdi, %rsi
@@ -232,7 +232,7 @@ L(return_vec_3):
# endif
VZEROUPPER_RETURN
- .p2align 4
+ .p2align 5
L(more_8x_vec):
/* Set end of s1 in rdx. */
leaq -(VEC_SIZE * 4)(%rdi, %rdx), %rdx
@@ -241,8 +241,11 @@ L(more_8x_vec):
subq %rdi, %rsi
/* Align s1 pointer. */
andq $-VEC_SIZE, %rdi
+ leaq -1(%rdx), %rax
+ subq %rdi, %rax
/* Adjust because first 4x vec where check already. */
subq $-(VEC_SIZE * 4), %rdi
+ sarq $LOG_VEC_SIZE, %rax
.p2align 4
L(loop_4x_vec):
/* rsi has s2 - s1 so get correct address by adding s1 (in rdi).
@@ -267,8 +270,8 @@ L(loop_4x_vec):
jnz L(8x_return_vec_0_1_2_3)
subq $-(VEC_SIZE * 4), %rdi
/* Check if s1 pointer at end. */
- cmpq %rdx, %rdi
- jb L(loop_4x_vec)
+ decq %rax
+ jne L(loop_4x_vec)
subq %rdx, %rdi
/* rdi has 4 * VEC_SIZE - remaining length. */
@@ -53,6 +53,7 @@
# endif
# define VEC_SIZE 32
+# define LOG_VEC_SIZE 7
# define PAGE_SIZE 4096
# define CHAR_PER_VEC (VEC_SIZE / CHAR_SIZE)
@@ -163,10 +164,7 @@ ENTRY (MEMCMP)
/* NB: eax must be zero to reach here. */
ret
- /* NB: aligning 32 here allows for the rest of the jump targets
- to be tuned for 32 byte alignment. Most important this ensures
- the L(more_8x_vec) loop is 32 byte aligned. */
- .p2align 5
+ .p2align 4
L(less_vec):
/* Check if one or less CHAR. This is necessary for size = 0 but
is also faster for size = CHAR_SIZE. */
@@ -277,7 +275,7 @@ L(return_vec_3):
# endif
ret
- .p2align 4
+ .p2align 5
L(more_8x_vec):
/* Set end of s1 in rdx. */
leaq -(VEC_SIZE * 4)(%rdi, %rdx, CHAR_SIZE), %rdx
@@ -286,8 +284,11 @@ L(more_8x_vec):
subq %rdi, %rsi
/* Align s1 pointer. */
andq $-VEC_SIZE, %rdi
+ leaq -1(%rdx), %rax
+ subq %rdi, %rax
/* Adjust because first 4x vec where check already. */
subq $-(VEC_SIZE * 4), %rdi
+ sarq $LOG_VEC_SIZE, %rax
.p2align 4
L(loop_4x_vec):
VMOVU (%rsi, %rdi), %YMM1
@@ -307,8 +308,8 @@ L(loop_4x_vec):
testl %ecx, %ecx
jnz L(8x_return_vec_0_1_2_3)
subq $-(VEC_SIZE * 4), %rdi
- cmpq %rdx, %rdi
- jb L(loop_4x_vec)
+ decq %rax
+ jnz L(loop_4x_vec)
subq %rdx, %rdi
/* rdi has 4 * VEC_SIZE - remaining length. */