[PATCH 2/3] tests: Don't pass INT_MAX to virFileReadAll()

[Michal Prívozník]

On 6/14/21 2:35 PM, Peter Krempa wrote:
> On Mon, Jun 14, 2021 at 14:30:26 +0200, Michal Prívozník wrote:
>> On 6/14/21 2:26 PM, Peter Krempa wrote:
>>> On Mon, Jun 14, 2021 at 14:14:47 +0200, Michal Prívozník wrote:
>>>> On 6/14/21 1:31 PM, Tim Wiederhake wrote:
>>>>> On Mon, 2021-06-14 at 13:06 +0200, Michal Privoznik wrote:
>>>>>> In a few occasions in tests we pass INT_MAX to
>>>>>> virFileReadLimFD(). This is not safe because virFileReadAll()
>>>>>> will call virFileReadLimFD() under the hood which takes the limit
>>>>>> and adds 1 to it.
>>>>>
>>>>> Calling virFileReadAll with "INT_MAX - 1" looks funny. Is it possible
>>>>> to check for "maxlen >= INT_MAX" in virFileReadLimFD instead?
>>>>
>>>> Actually, I don't understand why we need to add 1 in the first place.
>>>> I'll push the other two patches and send v2 for this that removes the +1.
>>>
>>> It's so that it guarantees that a file of 'maxlen' length is read
>>> completely and the terminating '\0' is in the resulting string.
>>>
>>> Removing the '+ 1' would change this kind of semantics, which may
>>> require audit of all callers.
>>>
>>
>> I'm not sure that's correct behaviour. I mean, if I specify that the
>> limit should be X, then at the most X bytes should be read, not X+1.
>>
>> Also, virFileReadLimFD() uses saferead_lim() which upon successful
>> return makes sure the returned string is properly terminated.
> 
> saferead_lim indeed terminates the string properly. virFileReadLimFD
> uses the '+ 1' to see whether there are exactly "maxlen" chars in the
> file or potentially more than that. That's why it actually reads 1 more
> than the requested maximum.
> 
> Whether that makes sense is obviously questionable and will require
> audit of all callers
> 

OTOH, now that I made the change locally, there's no difference between
virFileReadHeaderFD() and virFileReadLimFD(). IOW, if there is a caller
that wants to read a file fully it won't have a way to do that.
Although, I've looked at all calls of virFileReadLimFD() (and all
transitive places like virFileReadAll() and virFileReadAllQuiet()) and
all pass arbitrary limits but never retry with bigger limit on error.

Special casing INT_MAX in virFileReadLimFD() looks weird to me.

So I guess, in the end I'll put this into "known bug" section and carry
on with my life. Unless we want to do something as wild as:

  s = saferead_lim(fd, (size_t)maxlen + 1, &len);

Michal