[PATCH v1 1/1] readelf: invalid error message triggered when last tag is an empty string

[Matthieu Longo]

On 2025-06-18 12:55, Jan Beulich wrote:
> On 18.06.2025 13:07, Matthieu Longo wrote:
>> On 2025-06-18 11:42, Jan Beulich wrote:
>>> On 18.06.2025 12:27, Matthieu Longo wrote:
>>>> Disclaimer: this issue cannot occur with Object Attributes v1 (OAv1) because
>>>> a value of '\0' (empty string) for a tag with a string value is considered
>>>> as the default value for the attribute, and consequently is eliminated
>>>> from the output object during the serialization.
>>>>
>>>> In the context of OAv2 [1], an empty string is a valid value for a string
>>>> attribute tag, and can be saved as a tag might not have a default value.
>>>
>>> Is there a word (or more) missing in the latter part of this sentence?
>>
>> All the words are here, but my phrasing is probably poor.
>>
>> What I meant is that:
>> 1. an empty string is a valid value for a tag for both OAv1 and OAv2.
>> 2. contrarily to OAv1, with OAv2, subsections can be required and so
>> tags might need to be present even if the value is an empty string.
>> 3. the OAv2 serializer won't drop the default values.
>>
>> What about phrasing it like the below. Is it better ?
>>
>> An empty string is a valid value for a NTBS tag in both OAv1 and OAv2
>> [1] cases. However, contrarily to OAv1, a OAv2 subsection can be
>> required and so, tags in this subsection might have to be present even
>> if the value is the default. To comply with this requirement, the OAv2
>> serializer won't drop the default values.
> 
> It reads better to me, yes.
> 
>>>> --- a/binutils/readelf.c
>>>> +++ b/binutils/readelf.c
>>>> @@ -17779,12 +17779,14 @@ display_tag_value (signed int tag,
>>>>      else if (tag & 1)
>>>>        {
>>>>          /* PR 17531 file: 027-19978-0.004.  */
>>>> -      size_t maxlen = (end - p) - 1;
>>>> +      size_t maxlen = end - p;
>>>>    
>>>>          putchar ('"');
>>>>          if (maxlen > 0)
>>>>    	{
>>>> -	  print_symbol_name ((int) maxlen, (const char *) p);
>>>> +	  maxlen -= 1; /* Remove \0 from the character count.  */
>>>
>>> How do you know it's \0 that sits there?
>>
>> "else if (tag & 1)" checks whether it is a NTBS or a ULEB128.
>> So anything in this block assumes that the value is an NTBS.
>> If the value does not end with '\0', it means that it is a corrupt
>> string. In this case, the last character won't be printed.
>>
>> I think it might make sense to check whether the last character is `\0`
>> or not, and if it is not, then we fall back to printing out the error
>> message.
>>
>> I will address the issue in the next revision.
> 
> That or word the comment in a way that it's true for all input that can
> make it there.
> 
>>>> +	  if (maxlen > 0) /* Don't try to print an empty string.  */
>>>> +	    print_symbol_name ((int) maxlen, (const char *) p);
>>>
>>> Am I understanding correctly that for OAv2 this will then need further
>>> modifying, as (if I understand the description correctly) an empty
>>> string there is not the same as no tag at all.
>>
>> No, it won't need further modification.
> 
> Then I guess I'm confused. Either for OAv1 or for OAv2 there is a
> difference between "no tag" and "empty string tag", according to my
> understanding of what you wrote in old and new descriptions. Hence if
> for OAv2 you don't need to make further changes, is it perhaps that for
> OAv1 it is wrong to elide the printing of an empty string?
 > > Jan

As I mentioned previously, the OAv1 serializer prunes the default values 
so there is nothing left to print.
https://github.com/bminor/binutils-gdb/blob/86a5d1eb9b03ff391a551bf5594c0aa9898f9989/bfd/elf-attrs.c#L154-L156

Matthieu