Fix a crash in compile_to_object function

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

Fix a crash in compile_to_object function

libor.bukata
On non-Linux platforms, gdb crashes when compile command is issued
because of the null pointer in struct osabi_names gdb_osab. The attached
patch adds a check to avoid this crash and adds osabi name for Solaris.
However, there is probably more work required to enable compile feature
on Solaris (e.g., solaris_infcall_munmap) and other platforms.

Thanks,
Libor

001-fix-null-ptr.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

Keith Seitz
On 9/6/19 6:37 AM, [hidden email] wrote:
> On non-Linux platforms, gdb crashes when compile command is issued
> because of the null pointer in struct osabi_names gdb_osab. The attached
> patch adds a check to avoid this crash and adds osabi name for Solaris.
> However, there is probably more work required to enable compile feature
> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
>

Thank you for the patch! The compile feature, as you have discovered, needs
much more testing on non-linux configurations.

> --- gdb-8.3/gdb/compile/compile.c 2019-08-19 13:07:57.669785758 +0000
> +++ gdb-8.3/gdb/compile/compile.c 2019-08-19 13:07:33.865626973 +0000
> @@ -697,7 +697,7 @@ compile_to_object (struct command_line *
>        const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>  
>        /* Allow triplets with or without vendor set.  */
> -      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
>        compiler->set_triplet_regexp (triplet_rx.c_str ());
>      }

I'm not sure about this. Should os_rx be NULL (which you've shown is possible
for a number of configurations), this would leave triplet_rx as $ARCH(-[^-]*)?-",
e.g., "x86_64(-[^-]*)?-". I would call that a malformed regexp for this purpose.

While the plugin may handle that gracefully, this just doesn't seem very
user-friendly to me, but I am not a maintainer, so you should definitely
wait for a real maintainer to chime in.

Otherwise, the only comments I have relate to

> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");

We prefer explicit comparisons with NULL/nullptr.

Omitting the true-case expression is unusual. Not invalid, but certainly
unusual (in gdb). I find no uses of this idiom in our sources. I would
prefer to see the more explicit

   os_rx != nullptr ? os_rx : ""

but I'll let official maintainers chime in on this usage. I'm just as curious
to see how others feel about it.

Keith
Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

libor.bukata
Hi Keith,

I have no objections to use 'os_rx != NULL ? os_rx : ""' instead; it is
more portable. Note that NULL should be used instead of nullptr as
nullptr is available since C++11.

Thanks,
Libor

feel free to use 'os_rx != nullptr ? os_rx : ""' instead

On 9/6/19 6:42 PM, Keith Seitz wrote:

> On 9/6/19 6:37 AM, [hidden email] wrote:
>> On non-Linux platforms, gdb crashes when compile command is issued
>> because of the null pointer in struct osabi_names gdb_osab. The attached
>> patch adds a check to avoid this crash and adds osabi name for Solaris.
>> However, there is probably more work required to enable compile feature
>> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
>>
> Thank you for the patch! The compile feature, as you have discovered, needs
> much more testing on non-linux configurations.
>
>> --- gdb-8.3/gdb/compile/compile.c 2019-08-19 13:07:57.669785758 +0000
>> +++ gdb-8.3/gdb/compile/compile.c 2019-08-19 13:07:33.865626973 +0000
>> @@ -697,7 +697,7 @@ compile_to_object (struct command_line *
>>         const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>>  
>>         /* Allow triplets with or without vendor set.  */
>> -      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
>> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
>>         compiler->set_triplet_regexp (triplet_rx.c_str ());
>>       }
> I'm not sure about this. Should os_rx be NULL (which you've shown is possible
> for a number of configurations), this would leave triplet_rx as $ARCH(-[^-]*)?-",
> e.g., "x86_64(-[^-]*)?-". I would call that a malformed regexp for this purpose.
>
> While the plugin may handle that gracefully, this just doesn't seem very
> user-friendly to me, but I am not a maintainer, so you should definitely
> wait for a real maintainer to chime in.
>
> Otherwise, the only comments I have relate to
>
>> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
> We prefer explicit comparisons with NULL/nullptr.
>
> Omitting the true-case expression is unusual. Not invalid, but certainly
> unusual (in gdb). I find no uses of this idiom in our sources. I would
> prefer to see the more explicit
>
>     os_rx != nullptr ? os_rx : ""
>
> but I'll let official maintainers chime in on this usage. I'm just as curious
> to see how others feel about it.
>
> Keith

Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

Rainer Orth-2
In reply to this post by libor.bukata
Hi Libor,

> On non-Linux platforms, gdb crashes when compile command is issued
> because of the null pointer in struct osabi_names gdb_osab. The attached
> patch adds a check to avoid this crash and adds osabi name for Solaris.
> However, there is probably more work required to enable compile feature
> on Solaris (e.g., solaris_infcall_munmap) and other platforms.

just out of curiosity: what prompted you to try this?

I gave it a very quick whirl myself, trying to run the
gdb.compile/compile.exp test on amd64-pc-solaris2.11 with gdb master and
libcc1.so from gcc mainline: all I got was a SIGTRAP from the very first
compile command.

That said, I'm quite unlikely to work on this any time soon: with
ca. 2500 failures in the gdb testsuite on Solaris and even basic
features unimplented (e.g. I'm currently looking into TLS support), I
believe there are way more pressing issues.  However, if you want to
give it a try yourself, I'm more than happy to help get a patch in.

        Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

libor.bukata
Hi Rainer,

I noticed it from the testsuite results (crash dump generated) for gdb
8.3. Although it is not implemented for Solaris, it should not crash,
which is the reason why I sent a patch that resolves a nullptr
dereference. The patched gdb prints an error message (instead of
crashing) and the user can continue with debugging. I agree with you
that there many more pressing issues that needs to be resolved first.

Thanks,
Libor

On 9/9/19 10:07 AM, Rainer Orth wrote:

> Hi Libor,
>
>> On non-Linux platforms, gdb crashes when compile command is issued
>> because of the null pointer in struct osabi_names gdb_osab. The attached
>> patch adds a check to avoid this crash and adds osabi name for Solaris.
>> However, there is probably more work required to enable compile feature
>> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
> just out of curiosity: what prompted you to try this?
>
> I gave it a very quick whirl myself, trying to run the
> gdb.compile/compile.exp test on amd64-pc-solaris2.11 with gdb master and
> libcc1.so from gcc mainline: all I got was a SIGTRAP from the very first
> compile command.
>
> That said, I'm quite unlikely to work on this any time soon: with
> ca. 2500 failures in the gdb testsuite on Solaris and even basic
> features unimplented (e.g. I'm currently looking into TLS support), I
> believe there are way more pressing issues.  However, if you want to
> give it a try yourself, I'm more than happy to help get a patch in.
>
> Rainer
>

Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

Tom Tromey-2
In reply to this post by libor.bukata
>>>>> ">" == libor bukata <[hidden email]> writes:

>> I have no objections to use 'os_rx != NULL ? os_rx : ""' instead; it
>> is more portable. Note that NULL should be used instead of nullptr as
>> nullptr is available since C++11.

Just FYI, gdb uses C++11 nowadays, so nullptr is fine.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: Fix a crash in compile_to_object function

Rainer Orth-2
In reply to this post by libor.bukata
Hi Libor,

> I noticed it from the testsuite results (crash dump generated) for gdb
> 8.3. Although it is not implemented for Solaris, it should not crash, which
> is the reason why I sent a patch that resolves a nullptr dereference. The
> patched gdb prints an error message (instead of crashing) and the user can
> continue with debugging. I agree with you that there many more pressing
> issues that needs to be resolved first.

that explains why I'm not seeing this in my gdb builds: I use a
32-bit-default gcc 9.1 to build a 64-bit gdb, so there's no matching
libcc1.so around and the affected tests are UNTESTED.  Fully agreed that
there should be no crashes even for unsupported features, of course.

I'll leave the actual review to the responsible maintainers.

        Rainer

--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University