[PATCH v1 1/5] ld: fix segfault caused by untagged stub sections

[Matthieu Longo]

On 2025-10-14 08:23, Jan Beulich wrote:
> On 13.10.2025 19:01, Matthieu Longo wrote:
>> On 2025-10-13 16:30, Jan Beulich wrote:
>>> On 13.10.2025 17:22, Matthieu Longo wrote:
>>>> On 2025-09-26 14:13, Jan Beulich wrote:
>>>>> On 18.09.2025 17:06, Matthieu Longo wrote:
>>>>>> --- a/bfd/bfd-in2.h
>>>>>> +++ b/bfd/bfd-in2.h
>>>>>> @@ -834,6 +834,12 @@ typedef struct bfd_section
>>>>>>         const char *linked_to_symbol_name;
>>>>>>       } map_head, map_tail;
>>>>>>     
>>>>>> +  /* Indicate that the section contains branch veneers.  This is used when
>>>>>> +     support for non-contiguous memory regions is enabled.  The veneers have
>>>>>> +     to be allocated to the same memory region as the code they are refered
>>>>>> +     by, i.e. they cannot be moved to a subsequent memory region.  */
>>>>>> +  bool veneer;
>>>>>
>>>>> Why would you put a bool between two pointer-sized fields, thus introducing
>>>>> yet more padding, when in fact existing padding could be used. There's a set
>>>>> of bitfields further up from here, and you could simply add a single-bit
>>>>> field there, for example. Generic code changes are okay with that change,
>>>>> but arch-specific ones will need arch maintainer approval.
>>>>
>>>> Here is the new attribute as a flag:
>>>>
>>>> diff --git a/bfd/section.c b/bfd/section.c
>>>> index 5f0cf6e71cb..f110ed77363 100644
>>>> --- a/bfd/section.c
>>>> +++ b/bfd/section.c
>>>> @@ -379,6 +379,12 @@ CODE_FRAGMENT
>>>>     .     when memory read flag isn't set. *}
>>>>     .#define SEC_COFF_NOREAD            0x40000000
>>>>     .
>>>> +.  {* Indicate that the section contains branch veneers.  This is used when
>>>> +.     support for non-contiguous memory regions is enabled.  The
>>>> veneers have
>>>> +.     to be allocated to the same memory region as the code they are
>>>> refered
>>>> +.     by, i.e. they cannot be moved to a subsequent memory region.  *}
>>>> +.#define SEC_VENEER                 0x80000000
>>>> +.
>>>>     .  {*  End of section flags.  *}
>>>>     .
>>>>     .  {* Some internal packed boolean fields.  *}
>>>>
>>>>
>>>> With the tagging of the stub section on AArch64:
>>>>
>>>> diff --git a/ld/emultempl/aarch64elf.em b/ld/emultempl/aarch64elf.em
>>>> index 91d58d8fe5a..b00c9460da2 100644
>>>> --- a/ld/emultempl/aarch64elf.em
>>>> +++ b/ld/emultempl/aarch64elf.em
>>>> @@ -193,7 +193,7 @@ elf${ELFSIZE}_aarch64_add_stub_section (const char
>>>> *stub_sec_name,
>>>>       lang_output_section_statement_type *os;
>>>>       struct hook_stub_info info;
>>>>
>>>> -  flags = (SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_CODE
>>>> +  flags = (SEC_ALLOC | SEC_LOAD | SEC_READONLY | SEC_CODE | SEC_VENEER
>>>>               | SEC_HAS_CONTENTS | SEC_RELOC | SEC_IN_MEMORY | SEC_KEEP);
>>>>       stub_sec = bfd_make_section_anyway_with_flags (stub_file->the_bfd,
>>>>                                                     stub_sec_name, flags);
>>>>
>>>> Same thing on AArch32 and PowerPC.
>>>>
>>>> Is this what you suggested ?
>>>
>>> No, sorry. I said "bit field", not "section flag". I'd rather not see you
>>> use the last available section flag. Right below where you put the new
>>> #define is a comment "Some internal packed boolean fields" - that's where
>>> I thought your new flag would go.
>>
>> What about the below ?
> 
> Yes, with one cosmetic request though:
> 
>> diff --git a/bfd/section.c b/bfd/section.c
>> index 5f0cf6e71cb..424e53029d9 100644
>> --- a/bfd/section.c
>> +++ b/bfd/section.c
>> @@ -428,6 +428,12 @@ CODE_FRAGMENT
>>    .  {* Nonzero if section contents should not be freed.  *}
>>    .  unsigned int alloced:1;
>>    .
>> +.  {* Indicate that the section contains branch veneers.  This is used when
>> +.     support for non-contiguous memory regions is enabled.  The
>> veneers have
>> +.     to be allocated to the same memory region as the code they are
>> refered
>> +.     by, i.e. they cannot be moved to a subsequent memory region.  *}
>> +.  unsigned int veneer : 1;
>> +.
>>    .  {* Bits used by various backends.  The generic code doesn't touch
>>    .     these fields.  *}
>>    .
>> @@ -723,11 +729,11 @@ INTERNAL
>>    .  {* segment_mark, sec_info_type, use_rela_p, mmapped_p, alloced,
>> *}        \
>>    .     0,            0,             0,          0,         0,           \
>>    .                                                                      \
>> -.  {* sec_flg0, sec_flg1, sec_flg2, sec_flg3, sec_flg4, sec_flg5,    *}
> 
> Please can these all stay on one line, while you ...
> 
>>          \
>> +.  {* veneer, sec_flg0, sec_flg1, sec_flg2, sec_flg3, sec_flg4,      *}
> 
> ... add a new line to put the new initializer on? (Overall the macro likely
> wants switching to C99 dedicated initializers, at which point the comments
> can all go away, as can the many 0s and NULL-s. But that's for another day,
> I guess.)
> 
> Jan

Fixed in the next revision.

Matthieu