[PATCH v3 04/26] bfd/elf-attrs.c: use ASCII character '(0x27) for single quote

[Jan Beulich]

On 11.06.2025 15:32, Matthieu Longo wrote:
> On 2025-06-11 10:23, Richard Earnshaw (lists) wrote:
>> On 09/05/2025 16:12, Matthieu Longo wrote:
>>> ---
>>>   bfd/elf-attrs.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/bfd/elf-attrs.c b/bfd/elf-attrs.c
>>> index bc653d780de..b4a57f4803c 100644
>>> --- a/bfd/elf-attrs.c
>>> +++ b/bfd/elf-attrs.c
>>> @@ -104,7 +104,7 @@ vendor_obj_attrs_v1_size (bfd *abfd, int vendor)
>>>          list = list->next)
>>>       size += obj_attr_v1_size (list->tag, &list->attr);
>>>   
>>> -  /* <size> <vendor_name> NUL 0x1 <size> */
>>> +  /* <size> <vendor_name> NUL 0x1 <size>  */
>>>     return (size
>>>   	  ? size + 10 + strlen (vendor_name)
>>>   	  : 0);
>>> @@ -117,7 +117,7 @@ bfd_elf_obj_attrs_v1_size (bfd *abfd)
>>>     size = vendor_obj_attrs_v1_size (abfd, OBJ_ATTR_PROC);
>>>     size += vendor_obj_attrs_v1_size (abfd, OBJ_ATTR_GNU);
>>>     if (size > 0)
>>> -    size += sizeof(uint8_t); /* <format-version: ‘A’>  */
>>> +    size += sizeof(uint8_t); /* <format-version: 'A'>  */
>>>     return size;
>>>   }
>>>   
>>> @@ -212,7 +212,7 @@ write_obj_attr_section_v1 (bfd *abfd, bfd_byte *buffer, bfd_vma size)
>>>   {
>>>     bfd_byte *p = buffer;
>>>   
>>> -  /* <format-version: ‘A’>  */
>>> +  /* <format-version: 'A'>  */
>>>     *(p++) = 'A';
>>>   
>>>     for (int vendor = OBJ_ATTR_FIRST; vendor <= OBJ_ATTR_LAST; ++vendor)
>>
>> I can't approve this patch (or reject it), but I do wonder why you think it's needed.  There's nothing in the commit message to give justification for it's need.
>>
>> R.
> 
> The previous discussion was here: 
> https://inbox.sourceware.org/binutils/72219ed1-147d-4dd3-b503-363d981528a2@arm.com/
> 
> I forgot to update the commit message.
> Please let me know if this description is good enough.
> 
> Commit title:
> Remove non-ASCII characters causing false positive security warnings
> 
> Description:
> Arm's internal review tool complains about diffs containing non-ASCII 
> characters. The message points to a security risk explained in [1] and 
> [2]. The warnings from the review tool are a reasonable protection 
> against a potential avenue of surreptitious source code alteration, 
> relying on the usage of confusable Unicode characters. Instances that 
> are generating false positives should be removed, unless the presence of 
> such characters is necessary for the job at hand.
> 
> Even if the usage of curly simple quote seems legitimate and not 
> confusing, the cost of supporting it in comparison of the cost of 
> removing the false positive does not tip the scale in favor of the first 
> option.
> 
> This patch removes the curly simple quotes in bfd/elf-attrs.c, and 
> replaces them with a single quote ASCII character (0x27).
> 
> [1]: LWN article: https://lwn.net/Articles/874951/
> [2]: Associated paper: https://trojansource.codes/trojan-source.pdf

I remain unconvinced, fwiw. Unless overridden by Nick or Alan, I think I
would reject such a change (despite the earlier arguments in its favor).
If there was an override, I'd then request that it be written down
explicitly that non-ASCII characters may not be used in binutils sources.
Plus I'd point out that this can be limiting; see e.g. use of non-ASCII
in
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/tests/x86_emulator/simd-sha.c;hb=refs/heads/staging
where at least Σ is used simply to stay in sync with respective doc.

Jan