diff mbox series

[09/11] locale: Fix UB on add_locale_uint32_array

Message ID 20250507142110.3452012-10-adhemerval.zanella@linaro.org
State New
Headers show
Series Add initial support for --enable-ubsan | expand

Commit Message

Adhemerval Zanella May 7, 2025, 2:17 p.m. UTC
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(+)

Comments

Florian Weimer May 20, 2025, 12:49 p.m. UTC | #1
* 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
Adhemerval Zanella June 2, 2025, 1:09 p.m. UTC | #2
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.
Florian Weimer June 2, 2025, 1:29 p.m. UTC | #3
* 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
Adhemerval Zanella June 2, 2025, 1:56 p.m. UTC | #4
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.
Florian Weimer June 2, 2025, 2:01 p.m. UTC | #5
* 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
Adhemerval Zanella June 2, 2025, 4:01 p.m. UTC | #6
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 mbox series

Patch

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);
 }