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

[Richard Earnshaw]

On 11/06/2025 15:07, Jan Beulich wrote:
> 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

I think any change in policy should be done as a separate discussion, 
not in the middle of a complex patch set like this.

I'd drop the patch from this series; it's out of scope.

By all means raise the issue on the list, but it should be clear that 
it's a question to the maintainers in general.

R.