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

[Matthieu Longo]

On 2025-10-14 11:38, Matthieu Longo wrote:
> 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

Sorry Jan, I sent the new revision but forgot to add you in CC.

Matthieu