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

[Jan Beulich]

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