* [PATCH] elf: Check objname before calling fatal_error
@ 2024-04-08 16:06 H.J. Lu
2024-04-08 16:39 ` Sunil Pandey
2024-04-08 16:47 ` Adhemerval Zanella Netto
0 siblings, 2 replies; 7+ messages in thread
From: H.J. Lu @ 2024-04-08 16:06 UTC (permalink / raw)
To: libc-alpha
_dl_signal_error may be called with objname == NULL. _dl_exception_create
checks objname == NULL. But fatal_error doesn't. Check objname before
calling fatal_error. This fixes BZ #31596.
---
elf/dl-catch.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/elf/dl-catch.c b/elf/dl-catch.c
index 2109516dba..8ef7a4c706 100644
--- a/elf/dl-catch.c
+++ b/elf/dl-catch.c
@@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
__longjmp (lcatch->env[0].__jmpbuf, 1);
}
else
- fatal_error (errcode, objname, occasion, errstring);
+ {
+ if (objname == NULL)
+ objname = "";
+ fatal_error (errcode, objname, occasion, errstring);
+ }
}
rtld_hidden_def (_dl_signal_error)
--
2.44.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 16:06 [PATCH] elf: Check objname before calling fatal_error H.J. Lu
@ 2024-04-08 16:39 ` Sunil Pandey
2024-04-08 16:47 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 7+ messages in thread
From: Sunil Pandey @ 2024-04-08 16:39 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
On Mon, Apr 8, 2024 at 9:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> _dl_signal_error may be called with objname == NULL. _dl_exception_create
> checks objname == NULL. But fatal_error doesn't. Check objname before
> calling fatal_error. This fixes BZ #31596.
> ---
> elf/dl-catch.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/elf/dl-catch.c b/elf/dl-catch.c
> index 2109516dba..8ef7a4c706 100644
> --- a/elf/dl-catch.c
> +++ b/elf/dl-catch.c
> @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname,
> const char *occasion,
> __longjmp (lcatch->env[0].__jmpbuf, 1);
> }
> else
> - fatal_error (errcode, objname, occasion, errstring);
> + {
> + if (objname == NULL)
> + objname = "";
> + fatal_error (errcode, objname, occasion, errstring);
> + }
> }
> rtld_hidden_def (_dl_signal_error)
>
> --
> 2.44.0
>
>
LGTM
Reviewed-by: Sunil K Pandey <skpgkp2@gmail.com>
-Sunil
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 16:06 [PATCH] elf: Check objname before calling fatal_error H.J. Lu
2024-04-08 16:39 ` Sunil Pandey
@ 2024-04-08 16:47 ` Adhemerval Zanella Netto
2024-04-08 16:58 ` H.J. Lu
1 sibling, 1 reply; 7+ messages in thread
From: Adhemerval Zanella Netto @ 2024-04-08 16:47 UTC (permalink / raw)
To: libc-alpha, H.J. Lu
On 08/04/24 13:06, H.J. Lu wrote:
> _dl_signal_error may be called with objname == NULL. _dl_exception_create
> checks objname == NULL. But fatal_error doesn't. Check objname before
> calling fatal_error. This fixes BZ #31596.
Do we have a reproducer for this? The one from BZ#31596 does seems to trigger
it.
> ---
> elf/dl-catch.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/elf/dl-catch.c b/elf/dl-catch.c
> index 2109516dba..8ef7a4c706 100644
> --- a/elf/dl-catch.c
> +++ b/elf/dl-catch.c
> @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
> __longjmp (lcatch->env[0].__jmpbuf, 1);
> }
> else
> - fatal_error (errcode, objname, occasion, errstring);
> + {
> + if (objname == NULL)
> + objname = "";
> + fatal_error (errcode, objname, occasion, errstring);
> + }
> }
> rtld_hidden_def (_dl_signal_error)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 16:47 ` Adhemerval Zanella Netto
@ 2024-04-08 16:58 ` H.J. Lu
2024-04-08 17:18 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 7+ messages in thread
From: H.J. Lu @ 2024-04-08 16:58 UTC (permalink / raw)
To: Adhemerval Zanella Netto; +Cc: libc-alpha
On Mon, Apr 8, 2024 at 9:47 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 08/04/24 13:06, H.J. Lu wrote:
> > _dl_signal_error may be called with objname == NULL. _dl_exception_create
> > checks objname == NULL. But fatal_error doesn't. Check objname before
> > calling fatal_error. This fixes BZ #31596.
>
> Do we have a reproducer for this? The one from BZ#31596 does seems to trigger
> it.
We don't. But it may happen in theory.
> > ---
> > elf/dl-catch.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/elf/dl-catch.c b/elf/dl-catch.c
> > index 2109516dba..8ef7a4c706 100644
> > --- a/elf/dl-catch.c
> > +++ b/elf/dl-catch.c
> > @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
> > __longjmp (lcatch->env[0].__jmpbuf, 1);
> > }
> > else
> > - fatal_error (errcode, objname, occasion, errstring);
> > + {
> > + if (objname == NULL)
> > + objname = "";
> > + fatal_error (errcode, objname, occasion, errstring);
> > + }
> > }
> > rtld_hidden_def (_dl_signal_error)
> >
--
H.J.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 16:58 ` H.J. Lu
@ 2024-04-08 17:18 ` Adhemerval Zanella Netto
2024-04-08 17:39 ` H.J. Lu
0 siblings, 1 reply; 7+ messages in thread
From: Adhemerval Zanella Netto @ 2024-04-08 17:18 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha
On 08/04/24 13:58, H.J. Lu wrote:
> On Mon, Apr 8, 2024 at 9:47 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 08/04/24 13:06, H.J. Lu wrote:
>>> _dl_signal_error may be called with objname == NULL. _dl_exception_create
>>> checks objname == NULL. But fatal_error doesn't. Check objname before
>>> calling fatal_error. This fixes BZ #31596.
>>
>> Do we have a reproducer for this? The one from BZ#31596 does seems to trigger
>> it.
>
> We don't. But it may happen in theory.
Why not add the test on fatal_error instead? Because if we are adding
possible check for argument where we are not sure that it might trigger,
it is clear to me to add where the issue might happen (besides that our
policy is to avoid adding fixes without proper reproducers).
Also, I think we should close the BZ#31596 as a notabug because the
reproducer does not actually trigger an issue and it is misleading
that this is a glibc issue.
>
>>> ---
>>> elf/dl-catch.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/elf/dl-catch.c b/elf/dl-catch.c
>>> index 2109516dba..8ef7a4c706 100644
>>> --- a/elf/dl-catch.c
>>> +++ b/elf/dl-catch.c
>>> @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
>>> __longjmp (lcatch->env[0].__jmpbuf, 1);
>>> }
>>> else
>>> - fatal_error (errcode, objname, occasion, errstring);
>>> + {
>>> + if (objname == NULL)
>>> + objname = "";
>>> + fatal_error (errcode, objname, occasion, errstring);
>>> + }
>>> }
>>> rtld_hidden_def (_dl_signal_error)
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 17:18 ` Adhemerval Zanella Netto
@ 2024-04-08 17:39 ` H.J. Lu
2024-04-08 17:55 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 7+ messages in thread
From: H.J. Lu @ 2024-04-08 17:39 UTC (permalink / raw)
To: Adhemerval Zanella Netto; +Cc: libc-alpha
On Mon, Apr 8, 2024 at 10:18 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 08/04/24 13:58, H.J. Lu wrote:
> > On Mon, Apr 8, 2024 at 9:47 AM Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 08/04/24 13:06, H.J. Lu wrote:
> >>> _dl_signal_error may be called with objname == NULL. _dl_exception_create
> >>> checks objname == NULL. But fatal_error doesn't. Check objname before
> >>> calling fatal_error. This fixes BZ #31596.
> >>
> >> Do we have a reproducer for this? The one from BZ#31596 does seems to trigger
> >> it.
> >
> > We don't. But it may happen in theory.
>
> Why not add the test on fatal_error instead? Because if we are adding
> possible check for argument where we are not sure that it might trigger,
> it is clear to me to add where the issue might happen (besides that our
> policy is to avoid adding fixes without proper reproducers).
void
_dl_signal_exception (int errcode, struct dl_exception *exception,
const char *occasion)
{
struct rtld_catch *lcatch = get_catch ();
if (lcatch != NULL)
{
*lcatch->exception = *exception;
*lcatch->errcode = errcode;
/* We do not restore the signal mask because none was saved. */
__longjmp (lcatch->env[0].__jmpbuf, 1);
}
else
fatal_error (errcode, exception->objname, occasion, exception->errstring);
}
Will exception->objname ever be NULL?
> Also, I think we should close the BZ#31596 as a notabug because the
> reproducer does not actually trigger an issue and it is misleading
> that this is a glibc issue.
>
> >
> >>> ---
> >>> elf/dl-catch.c | 6 +++++-
> >>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/elf/dl-catch.c b/elf/dl-catch.c
> >>> index 2109516dba..8ef7a4c706 100644
> >>> --- a/elf/dl-catch.c
> >>> +++ b/elf/dl-catch.c
> >>> @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
> >>> __longjmp (lcatch->env[0].__jmpbuf, 1);
> >>> }
> >>> else
> >>> - fatal_error (errcode, objname, occasion, errstring);
> >>> + {
> >>> + if (objname == NULL)
> >>> + objname = "";
> >>> + fatal_error (errcode, objname, occasion, errstring);
> >>> + }
> >>> }
> >>> rtld_hidden_def (_dl_signal_error)
> >>>
>
>
--
H.J.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] elf: Check objname before calling fatal_error
2024-04-08 17:39 ` H.J. Lu
@ 2024-04-08 17:55 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella Netto @ 2024-04-08 17:55 UTC (permalink / raw)
To: H.J. Lu; +Cc: libc-alpha
On 08/04/24 14:39, H.J. Lu wrote:
> On Mon, Apr 8, 2024 at 10:18 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 08/04/24 13:58, H.J. Lu wrote:
>>> On Mon, Apr 8, 2024 at 9:47 AM Adhemerval Zanella Netto
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 08/04/24 13:06, H.J. Lu wrote:
>>>>> _dl_signal_error may be called with objname == NULL. _dl_exception_create
>>>>> checks objname == NULL. But fatal_error doesn't. Check objname before
>>>>> calling fatal_error. This fixes BZ #31596.
>>>>
>>>> Do we have a reproducer for this? The one from BZ#31596 does seems to trigger
>>>> it.
>>>
>>> We don't. But it may happen in theory.
>>
>> Why not add the test on fatal_error instead? Because if we are adding
>> possible check for argument where we are not sure that it might trigger,
>> it is clear to me to add where the issue might happen (besides that our
>> policy is to avoid adding fixes without proper reproducers).
>
> void
> _dl_signal_exception (int errcode, struct dl_exception *exception,
> const char *occasion)
> {
> struct rtld_catch *lcatch = get_catch ();
> if (lcatch != NULL)
> {
> *lcatch->exception = *exception;
> *lcatch->errcode = errcode;
>
> /* We do not restore the signal mask because none was saved. */
> __longjmp (lcatch->env[0].__jmpbuf, 1);
> }
> else
> fatal_error (errcode, exception->objname, occasion, exception->errstring);
> }
>
> Will exception->objname ever be NULL?
From the Florian example [1], most of failures where _dl_signal_error is called
with NULL are for malloc failure or any other error. Maybe another option would
to actually pass the objname in such cases.
[1] https://patchwork.sourceware.org/project/glibc/patch/877ch7vmab.fsf@oldenburg.str.redhat.com/
>
>> Also, I think we should close the BZ#31596 as a notabug because the
>> reproducer does not actually trigger an issue and it is misleading
>> that this is a glibc issue.
>>
>>>
>>>>> ---
>>>>> elf/dl-catch.c | 6 +++++-
>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/elf/dl-catch.c b/elf/dl-catch.c
>>>>> index 2109516dba..8ef7a4c706 100644
>>>>> --- a/elf/dl-catch.c
>>>>> +++ b/elf/dl-catch.c
>>>>> @@ -126,7 +126,11 @@ _dl_signal_error (int errcode, const char *objname, const char *occasion,
>>>>> __longjmp (lcatch->env[0].__jmpbuf, 1);
>>>>> }
>>>>> else
>>>>> - fatal_error (errcode, objname, occasion, errstring);
>>>>> + {
>>>>> + if (objname == NULL)
>>>>> + objname = "";
>>>>> + fatal_error (errcode, objname, occasion, errstring);
>>>>> + }
>>>>> }
>>>>> rtld_hidden_def (_dl_signal_error)
>>>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-08 17:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-08 16:06 [PATCH] elf: Check objname before calling fatal_error H.J. Lu
2024-04-08 16:39 ` Sunil Pandey
2024-04-08 16:47 ` Adhemerval Zanella Netto
2024-04-08 16:58 ` H.J. Lu
2024-04-08 17:18 ` Adhemerval Zanella Netto
2024-04-08 17:39 ` H.J. Lu
2024-04-08 17:55 ` Adhemerval Zanella Netto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).