divdi3: incomplete fix in db3d848e154b?

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

divdi3: incomplete fix in db3d848e154b?

Alexey Neyman
Hi,

I was compiling glibc (2.25 + d40dbe722f004) for sh4-unknown-linux-gnu
and the build failed due to rtld pulling abort.os again. This has been
"fixed" in master in db3d848e154b - which made divdi3 built only for
certain architectures. However, I think this just masks the real issue
that GCC7 inserts a trap where it knows something gets divided by zero.

Here is a fragment of generated code for i686-unknown-linux:

objdump -dlr csu/divdi3.os:
/home/avn/work/ctng/crosstool-ng/.build/src/glibc-2.25/csu/../sysdeps/wordsize-32/divdi3.c:91
   d0:   85 db                   test   %ebx,%ebx
   d2:   75 0c                   jne    e0 <__udivmoddi4+0xe0>
   d4:   0f 0b                   ud2

divdi3.c:
90          if (d0 == 0)
91            d0 = 1 / d0;        /* Divide intentionally by zero.  */

Isn't the purpose of this intentional division to generate a SIGFPE,
rather than SIGILL?

Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be
compiled with -fno-sanitize=integer-divide-by-zero?

Regards,
Alexey.

Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Zack Weinberg-2
On Sun, May 14, 2017 at 2:22 AM, Alexey Neyman <[hidden email]> wrote:

>
> divdi3.c:
> 90          if (d0 == 0)
> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>
> Isn't the purpose of this intentional division to generate a SIGFPE, rather
> than SIGILL?
>
> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled
> with -fno-sanitize=integer-divide-by-zero?

Yes, I think you're right.

We should try to get an actual hardware division by zero if we can;
raise(SIGFPE) will not fill in the siginfo structure exactly the same
way.  It might be the best we can do sometimes, though.

zw
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Jeff Law
On 05/14/2017 07:22 AM, Zack Weinberg wrote:

> On Sun, May 14, 2017 at 2:22 AM, Alexey Neyman <[hidden email]> wrote:
>>
>> divdi3.c:
>> 90          if (d0 == 0)
>> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>>
>> Isn't the purpose of this intentional division to generate a SIGFPE, rather
>> than SIGILL?
>>
>> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled
>> with -fno-sanitize=integer-divide-by-zero?
>
> Yes, I think you're right.
>
> We should try to get an actual hardware division by zero if we can;
> raise(SIGFPE) will not fill in the siginfo structure exactly the same
> way.  It might be the best we can do sometimes, though.
Note that GCC will detect this as a division by zero and try to turn it
into a trap on architectures that have trap instructions.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Joseph Myers
In reply to this post by Zack Weinberg-2
On Sun, 14 May 2017, Zack Weinberg wrote:

> We should try to get an actual hardware division by zero if we can;
> raise(SIGFPE) will not fill in the siginfo structure exactly the same
> way.  It might be the best we can do sometimes, though.

divdi3.c is only present for binary compatibility for architectures
exporting those functions from glibc 2.0.  The most we could need is a few
architecture-specific asms (in a macro defined in architecture-specific
versions of this file before including the generic one, say).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Zack Weinberg-2
In reply to this post by Jeff Law
On Sun, May 14, 2017 at 10:18 AM, Jeff Law <[hidden email]> wrote:

> On 05/14/2017 07:22 AM, Zack Weinberg wrote:
>> On Sun, May 14, 2017 at 2:22 AM, Alexey Neyman <[hidden email]> wrote:
>>>
>>>
>>> divdi3.c:
>>> 90          if (d0 == 0)
>>> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>>>
>>> Isn't the purpose of this intentional division to generate a SIGFPE,
>>> rather than SIGILL?
>>
>>
>> Yes, I think you're right.
>>
>> We should try to get an actual hardware division by zero if we can;
>> raise(SIGFPE) will not fill in the siginfo structure exactly the same
>> way.  It might be the best we can do sometimes, though.
>
> Note that GCC will detect this as a division by zero and try to turn it into
> a trap on architectures that have trap instructions.

Right, that's the problem. Callers reasonably expect to get SIGFPE
rather than SIGILL, and also apparently it turns into a call to
abort() which may not always be available.  I don't fully understand
Alexey's situation, though.

zw
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Adhemerval Zanella-2
In reply to this post by Alexey Neyman


On 14/05/2017 03:22, Alexey Neyman wrote:

> Hi,
>
> I was compiling glibc (2.25 + d40dbe722f004) for sh4-unknown-linux-gnu and the build failed due to rtld pulling abort.os again. This has been "fixed" in master in db3d848e154b - which made divdi3 built only for certain architectures. However, I think this just masks the real issue that GCC7 inserts a trap where it knows something gets divided by zero.
>
> Here is a fragment of generated code for i686-unknown-linux:
>
> objdump -dlr csu/divdi3.os:
> /home/avn/work/ctng/crosstool-ng/.build/src/glibc-2.25/csu/../sysdeps/wordsize-32/divdi3.c:91
>   d0:   85 db                   test   %ebx,%ebx
>   d2:   75 0c                   jne    e0 <__udivmoddi4+0xe0>
>   d4:   0f 0b                   ud2
>
> divdi3.c:
> 90          if (d0 == 0)
> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>
> Isn't the purpose of this intentional division to generate a SIGFPE, rather than SIGILL?
>
> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled with -fno-sanitize=integer-divide-by-zero?
>
> Regards,
> Alexey.
>

This is a known issue with GCC for SH [1], which still does not have support for
__builtin_trap.  From last comments it is still in discussion if the trap will
use either the '#trap' instruction or a undefined instruction.

Unless we get an unconditional trap fix this fix will be a better effort in the
sense each new GCC version will bring potentially new analysis that might
generate the trap instructions for current code.  So it is basically two efforts
here: fix/avoid any glibc code that might generate this kind of trap for SH
(d40dbe722f004 for instance) and check if the resulting gcc build actually
get it right (db3d848e154b fix for instance).

Another option could still requiring using GCC specific flags to build glibc
on SH (-fno-isolate-erroneous-paths-dereference and
-fno-isolate-erroneous-paths-attribute for instance).  I would prefer to avoid
it and have a SH support from default headers (assuming gcc was not configured
to use them).

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Jeff Law
On 05/15/2017 08:58 AM, Adhemerval Zanella wrote:

>
>
> On 14/05/2017 03:22, Alexey Neyman wrote:
>> Hi,
>>
>> I was compiling glibc (2.25 + d40dbe722f004) for sh4-unknown-linux-gnu and the build failed due to rtld pulling abort.os again. This has been "fixed" in master in db3d848e154b - which made divdi3 built only for certain architectures. However, I think this just masks the real issue that GCC7 inserts a trap where it knows something gets divided by zero.
>>
>> Here is a fragment of generated code for i686-unknown-linux:
>>
>> objdump -dlr csu/divdi3.os:
>> /home/avn/work/ctng/crosstool-ng/.build/src/glibc-2.25/csu/../sysdeps/wordsize-32/divdi3.c:91
>>    d0:   85 db                   test   %ebx,%ebx
>>    d2:   75 0c                   jne    e0 <__udivmoddi4+0xe0>
>>    d4:   0f 0b                   ud2
>>
>> divdi3.c:
>> 90          if (d0 == 0)
>> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>>
>> Isn't the purpose of this intentional division to generate a SIGFPE, rather than SIGILL?
>>
>> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled with -fno-sanitize=integer-divide-by-zero?
>>
>> Regards,
>> Alexey.
>>
>
> This is a known issue with GCC for SH [1], which still does not have support for
> __builtin_trap.  From last comments it is still in discussion if the trap will
> use either the '#trap' instruction or a undefined instruction.
>
> Unless we get an unconditional trap fix this fix will be a better effort in the
> sense each new GCC version will bring potentially new analysis that might
> generate the trap instructions for current code.  So it is basically two efforts
> here: fix/avoid any glibc code that might generate this kind of trap for SH
> (d40dbe722f004 for instance) and check if the resulting gcc build actually
> get it right (db3d848e154b fix for instance).
>
> Another option could still requiring using GCC specific flags to build glibc
> on SH (-fno-isolate-erroneous-paths-dereference and
> -fno-isolate-erroneous-paths-attribute for instance).  I would prefer to avoid
> it and have a SH support from default headers (assuming gcc was not configured
> to use them).
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216
Carlos and I discussed some stuff in this space a year or so ago in
person.  His assertion was that creating a trap instruction when
building glibc, even if GCC found undefined behaviour was fundamentally
wrong.

The basic idea was that if glibc happened to be working, even though it
was tickling undefined behaviour that converting the UB into a trap
could result in an un-bootable system and code that just halts
unexpectedly when it worked before.  The better behaviour was to just
let the UB occur and allow the system to continue to run (I'm going from
memory, so if I've missed something Carlos, don't hesitate to correct me).

I certainly understand Carlos's position, even if I don't 100% agree
with it (particularly from a security standpoint).  However, given that
position, building all glibc targets with -fno-isolate-erroneous-paths
would seem to be the right thing to do.

Right now GCC will only convert NULL pointer dereferences and division
by zero to a trap.  However, I expect that by the time GCC 8 is ready
we'll also be converting out-of-bounds array accesses (only those which
are proved out of bounds, of course).  There may be other cases of UB
that we ultimately want to convert -- ie, we shouldn't assume we're
necessarily done with conversion of erroneous code to traps.

Jeff

Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Andreas Schwab
On Mai 15 2017, Jeff Law <[hidden email]> wrote:

> I certainly understand Carlos's position, even if I don't 100% agree with
> it (particularly from a security standpoint).  However, given that
> position, building all glibc targets with -fno-isolate-erroneous-paths
> would seem to be the right thing to do.

libgcc2.c has the same problem.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Alexey Neyman
In reply to this post by Adhemerval Zanella-2
On 05/15/2017 07:58 AM, Adhemerval Zanella wrote:

>
> On 14/05/2017 03:22, Alexey Neyman wrote:
>> Hi,
>>
>> I was compiling glibc (2.25 + d40dbe722f004) for sh4-unknown-linux-gnu and the build failed due to rtld pulling abort.os again. This has been "fixed" in master in db3d848e154b - which made divdi3 built only for certain architectures. However, I think this just masks the real issue that GCC7 inserts a trap where it knows something gets divided by zero.
>>
>> Here is a fragment of generated code for i686-unknown-linux:
>>
>> objdump -dlr csu/divdi3.os:
>> /home/avn/work/ctng/crosstool-ng/.build/src/glibc-2.25/csu/../sysdeps/wordsize-32/divdi3.c:91
>>    d0:   85 db                   test   %ebx,%ebx
>>    d2:   75 0c                   jne    e0 <__udivmoddi4+0xe0>
>>    d4:   0f 0b                   ud2
>>
>> divdi3.c:
>> 90          if (d0 == 0)
>> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>>
>> Isn't the purpose of this intentional division to generate a SIGFPE, rather than SIGILL?
>>
>> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled with -fno-sanitize=integer-divide-by-zero?
>>
>> Regards,
>> Alexey.
>>
> This is a known issue with GCC for SH [1], which still does not have support for
> __builtin_trap.  From last comments it is still in discussion if the trap will
> use either the '#trap' instruction or a undefined instruction.
>
> Unless we get an unconditional trap fix this fix will be a better effort in the
> sense each new GCC version will bring potentially new analysis that might
> generate the trap instructions for current code.  So it is basically two efforts
> here: fix/avoid any glibc code that might generate this kind of trap for SH
> (d40dbe722f004 for instance) and check if the resulting gcc build actually
> get it right (db3d848e154b fix for instance).
But the issue I am pointing out here is not on SH - rather on
architectures that still build divdi3.c, such as i686. And the issue is
exactly that db3d848e154b fix does not get it right - it will result in
a different signal being delivered than the one intended by the code.

Regards,
Alexey.
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Adhemerval Zanella-2


On 15/05/2017 12:48, Alexey Neyman wrote:

> On 05/15/2017 07:58 AM, Adhemerval Zanella wrote:
>>
>> On 14/05/2017 03:22, Alexey Neyman wrote:
>>> Hi,
>>>
>>> I was compiling glibc (2.25 + d40dbe722f004) for sh4-unknown-linux-gnu and the build failed due to rtld pulling abort.os again. This has been "fixed" in master in db3d848e154b - which made divdi3 built only for certain architectures. However, I think this just masks the real issue that GCC7 inserts a trap where it knows something gets divided by zero.
>>>
>>> Here is a fragment of generated code for i686-unknown-linux:
>>>
>>> objdump -dlr csu/divdi3.os:
>>> /home/avn/work/ctng/crosstool-ng/.build/src/glibc-2.25/csu/../sysdeps/wordsize-32/divdi3.c:91
>>>    d0:   85 db                   test   %ebx,%ebx
>>>    d2:   75 0c                   jne    e0 <__udivmoddi4+0xe0>
>>>    d4:   0f 0b                   ud2
>>>
>>> divdi3.c:
>>> 90          if (d0 == 0)
>>> 91            d0 = 1 / d0;        /* Divide intentionally by zero.  */
>>>
>>> Isn't the purpose of this intentional division to generate a SIGFPE, rather than SIGILL?
>>>
>>> Shouldn't divdi3.c do something else here, e.g. raise(SIGFPE) or be compiled with -fno-sanitize=integer-divide-by-zero?
>>>
>>> Regards,
>>> Alexey.
>>>
>> This is a known issue with GCC for SH [1], which still does not have support for
>> __builtin_trap.  From last comments it is still in discussion if the trap will
>> use either the '#trap' instruction or a undefined instruction.
>>
>> Unless we get an unconditional trap fix this fix will be a better effort in the
>> sense each new GCC version will bring potentially new analysis that might
>> generate the trap instructions for current code.  So it is basically two efforts
>> here: fix/avoid any glibc code that might generate this kind of trap for SH
>> (d40dbe722f004 for instance) and check if the resulting gcc build actually
>> get it right (db3d848e154b fix for instance).
> But the issue I am pointing out here is not on SH - rather on architectures that still build divdi3.c, such as i686. And the issue is exactly that db3d848e154b fix does not get it right - it will result in a different signal being delivered than the one intended by the code.
>
> Regards,
> Alexey.

I am aware, my point is db3d848e154b patch was to follow what Joseph
suggested: divdi3.c should be only present for binary compatibility
for architectures exporting those functions from glibc 2.0, not as
default.  It is not to fix the expected exception (which is another
issue), but rather it was a side-effect of it.

Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Zack Weinberg-2
There are two problems, and maybe we should talk about them independently.

One problem is that the compiler may insert __builtin_trap operations
to cause prompt crashes for UB - which is a Good Thing - but on
architectures where the back-end doesn't know how to generate a
hardware trap instruction, __builtin_trap produces a call to abort,
which is not available in ld.so.  This will only keep happening, and
there may be architectures that don't _have_ a guaranteed-invalid
opcode, so it's not fair to call it the compiler's problem.  Why
shouldn't we just move the one true definition of abort into ld.so?
(In fact, I would like to ask this question about _all_ functions that
are defined in both ld.so and libc.so.)

The other problem is that divdi3.c in particular should be generating
SIGFPE instead of SIGILL.  I think there's a case to be made that the
optimization GCC is doing is _incorrect_ for division by constant
zero, precisely because arithmetic emulation code needs a way to
produce a "true" divide-by-zero SIGFPE.  But why do we even _have_
divdi3.c?  On architectures where libc.so needs to expose __divdi3,
why are we not getting the definition from libgcc.a?

zw
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Adhemerval Zanella-2


On 15/05/2017 15:30, Zack Weinberg wrote:

> There are two problems, and maybe we should talk about them independently.
>
> One problem is that the compiler may insert __builtin_trap operations
> to cause prompt crashes for UB - which is a Good Thing - but on
> architectures where the back-end doesn't know how to generate a
> hardware trap instruction, __builtin_trap produces a call to abort,
> which is not available in ld.so.  This will only keep happening, and
> there may be architectures that don't _have_ a guaranteed-invalid
> opcode, so it's not fair to call it the compiler's problem.  Why
> shouldn't we just move the one true definition of abort into ld.so?
> (In fact, I would like to ask this question about _all_ functions that
> are defined in both ld.so and libc.so.)

AFAIK glibc build system already does it, the problem is current abort
implementation pull more stuff that are meant only for libc.so.  I think
we will need to create a ld.so specific version of abort (IS_IN(ld))
with a slight different semantic.  Good thing is we already have
abort-instr.h which should be defined by all architectures (SH inclusive).

>
> The other problem is that divdi3.c in particular should be generating
> SIGFPE instead of SIGILL.  I think there's a case to be made that the
> optimization GCC is doing is _incorrect_ for division by constant
> zero, precisely because arithmetic emulation code needs a way to
> produce a "true" divide-by-zero SIGFPE.  But why do we even _have_
> divdi3.c?  On architectures where libc.so needs to expose __divdi3,
> why are we not getting the definition from libgcc.a?

Because we need to provide a compatibility symbols for some ABIs,
using libgcc.a won't help old binaries that calls divdi3 from libc.so.
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Zack Weinberg-2
On Mon, May 15, 2017 at 3:09 PM, Adhemerval Zanella
<[hidden email]> wrote:
> On 15/05/2017 15:30, Zack Weinberg wrote:
>> On architectures where libc.so needs to expose __divdi3,
>> why are we not getting the definition from libgcc.a?
>
> Because we need to provide a compatibility symbols for some ABIs,
> using libgcc.a won't help old binaries that calls divdi3 from libc.so.

Are you saying that on the affected architectures, a current libgcc.a
does not contain a definition of __divdi3?

zw
Reply | Threaded
Open this post in threaded view
|

Re: divdi3: incomplete fix in db3d848e154b?

Joseph Myers
In reply to this post by Zack Weinberg-2
On Mon, 15 May 2017, Zack Weinberg wrote:

> produce a "true" divide-by-zero SIGFPE.  But why do we even _have_
> divdi3.c?  On architectures where libc.so needs to expose __divdi3,
> why are we not getting the definition from libgcc.a?

libgcc.a symbols are hidden symbols (to prevent them from being reexported
from shared libraries by accident, as happened in glibc 2.0 before they
were hidden and before symbol versioning).  And as far as I know, making a
symbol into a compat symbol (at least the way glibc does it) has to be
done in the same .o file that defines it, which obviously can't be done
when the implementation is from libgcc.a.

--
Joseph S. Myers
[hidden email]