| Message ID | 20250507142110.3452012-10-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for --enable-ubsan | expand |
* Adhemerval Zanella: > The ubsan triggers: > > UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 > > The obstack_grow is only required if there is extra elements to be > inserted (n_elems > 0). > --- > locale/programs/locfile.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c > index b54fcbbceb..7907c949ea 100644 > --- a/locale/programs/locfile.c > +++ b/locale/programs/locfile.c > @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, > { > align_locale_data (file, LOCFILE_ALIGN); > record_offset (file); > + if (n_elems == 0) > + return; > obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); > maybe_swap_uint32_obstack (&file->data, n_elems); > } We should fix the declaration of obstack_grow instead, to align with the new declarations for memcpy et al. Thanks, Florian
On 20/05/25 09:49, Florian Weimer wrote: > * Adhemerval Zanella: > >> The ubsan triggers: >> >> UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 >> >> The obstack_grow is only required if there is extra elements to be >> inserted (n_elems > 0). >> --- >> locale/programs/locfile.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c >> index b54fcbbceb..7907c949ea 100644 >> --- a/locale/programs/locfile.c >> +++ b/locale/programs/locfile.c >> @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, >> { >> align_locale_data (file, LOCFILE_ALIGN); >> record_offset (file); >> + if (n_elems == 0) >> + return; >> obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); >> maybe_swap_uint32_obstack (&file->data, n_elems); >> } > > We should fix the declaration of obstack_grow instead, to align with the > new declarations for memcpy et al. To accept lenght equal 0? It might be an option, I am not sure which is the best option.
* Adhemerval Zanella Netto: > On 20/05/25 09:49, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> The ubsan triggers: >>> >>> UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 >>> >>> The obstack_grow is only required if there is extra elements to be >>> inserted (n_elems > 0). >>> --- >>> locale/programs/locfile.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c >>> index b54fcbbceb..7907c949ea 100644 >>> --- a/locale/programs/locfile.c >>> +++ b/locale/programs/locfile.c >>> @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, >>> { >>> align_locale_data (file, LOCFILE_ALIGN); >>> record_offset (file); >>> + if (n_elems == 0) >>> + return; >>> obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); >>> maybe_swap_uint32_obstack (&file->data, n_elems); >>> } >> >> We should fix the declaration of obstack_grow instead, to align with the >> new declarations for memcpy et al. > > To accept lenght equal 0? It might be an option, I am not sure which is the > best option. Yes, use the nonnull_if_nonzero if available, or nothing otherwise. Thanks, Florian
On 02/06/25 10:29, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 20/05/25 09:49, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> The ubsan triggers: >>>> >>>> UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 >>>> >>>> The obstack_grow is only required if there is extra elements to be >>>> inserted (n_elems > 0). >>>> --- >>>> locale/programs/locfile.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c >>>> index b54fcbbceb..7907c949ea 100644 >>>> --- a/locale/programs/locfile.c >>>> +++ b/locale/programs/locfile.c >>>> @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, >>>> { >>>> align_locale_data (file, LOCFILE_ALIGN); >>>> record_offset (file); >>>> + if (n_elems == 0) >>>> + return; >>>> obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); >>>> maybe_swap_uint32_obstack (&file->data, n_elems); >>>> } >>> >>> We should fix the declaration of obstack_grow instead, to align with the >>> new declarations for memcpy et al. >> >> To accept lenght equal 0? It might be an option, I am not sure which is the >> best option. > > Yes, use the nonnull_if_nonzero if available, or nothing otherwise. These interfaces are implemented as macros, so we will need to make them proper functions for this. And since we do export them, I think we will either need add some hacks to enable it only for c99 or higher, or add internal interfaces. Both seems not ideal. gnulib still uses macro though and does not handle length to 0.
* Adhemerval Zanella Netto: > On 02/06/25 10:29, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >> >>> On 20/05/25 09:49, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> The ubsan triggers: >>>>> >>>>> UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 >>>>> >>>>> The obstack_grow is only required if there is extra elements to be >>>>> inserted (n_elems > 0). >>>>> --- >>>>> locale/programs/locfile.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c >>>>> index b54fcbbceb..7907c949ea 100644 >>>>> --- a/locale/programs/locfile.c >>>>> +++ b/locale/programs/locfile.c >>>>> @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, >>>>> { >>>>> align_locale_data (file, LOCFILE_ALIGN); >>>>> record_offset (file); >>>>> + if (n_elems == 0) >>>>> + return; >>>>> obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); >>>>> maybe_swap_uint32_obstack (&file->data, n_elems); >>>>> } >>>> >>>> We should fix the declaration of obstack_grow instead, to align with the >>>> new declarations for memcpy et al. >>> >>> To accept lenght equal 0? It might be an option, I am not sure which is the >>> best option. >> >> Yes, use the nonnull_if_nonzero if available, or nothing otherwise. > > These interfaces are implemented as macros, so we will need to make them > proper functions for this. And since we do export them, I think we will > either need add some hacks to enable it only for c99 or higher, or add > internal interfaces. Both seems not ideal. > > gnulib still uses macro though and does not handle length to 0. Ahh, so it's essentially the same memcpy/memset issue? Assuming that's what the macro uses? Thanks, Florian
On 02/06/25 11:01, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 02/06/25 10:29, Florian Weimer wrote: >>> * Adhemerval Zanella Netto: >>> >>>> On 20/05/25 09:49, Florian Weimer wrote: >>>>> * Adhemerval Zanella: >>>>> >>>>>> The ubsan triggers: >>>>>> >>>>>> UBSAN: Undefined behaviour in programs/locfile.c:644:3 null pointer passed as argument 2, nonnull attribute declared at unknown:0:0 >>>>>> >>>>>> The obstack_grow is only required if there is extra elements to be >>>>>> inserted (n_elems > 0). >>>>>> --- >>>>>> locale/programs/locfile.c | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c >>>>>> index b54fcbbceb..7907c949ea 100644 >>>>>> --- a/locale/programs/locfile.c >>>>>> +++ b/locale/programs/locfile.c >>>>>> @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, >>>>>> { >>>>>> align_locale_data (file, LOCFILE_ALIGN); >>>>>> record_offset (file); >>>>>> + if (n_elems == 0) >>>>>> + return; >>>>>> obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); >>>>>> maybe_swap_uint32_obstack (&file->data, n_elems); >>>>>> } >>>>> >>>>> We should fix the declaration of obstack_grow instead, to align with the >>>>> new declarations for memcpy et al. >>>> >>>> To accept lenght equal 0? It might be an option, I am not sure which is the >>>> best option. >>> >>> Yes, use the nonnull_if_nonzero if available, or nothing otherwise. >> >> These interfaces are implemented as macros, so we will need to make them >> proper functions for this. And since we do export them, I think we will >> either need add some hacks to enable it only for c99 or higher, or add >> internal interfaces. Both seems not ideal. >> >> gnulib still uses macro though and does not handle length to 0. > > Ahh, so it's essentially the same memcpy/memset issue? Assuming that's > what the macro uses? Yes, the ubsan issue is at the memcpy/memset call.
diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c index b54fcbbceb..7907c949ea 100644 --- a/locale/programs/locfile.c +++ b/locale/programs/locfile.c @@ -641,6 +641,8 @@ add_locale_uint32_array (struct locale_file *file, { align_locale_data (file, LOCFILE_ALIGN); record_offset (file); + if (n_elems == 0) + return; obstack_grow (&file->data, data, n_elems * sizeof (uint32_t)); maybe_swap_uint32_obstack (&file->data, n_elems); }