[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] openvzGetVEID: don't leak (memory + file descriptor)



Eric Blake wrote:
> According to Jim Meyering on 2/25/2010 11:30 AM:
>
> ACK on plugging the leak.  However,...

Thanks for the review.

>> @@ -979,18 +980,12 @@ int openvzGetVEID(const char *name) {
>>          return -1;
>>      }
>>
>> -    if (fscanf(fp, "%d\n", &veid ) != 1) {
>> +    ok = fscanf(fp, "%d\n", &veid ) == 1;
>
> You're still keeping with fscanf.

Yes.  There are plenty of bigger fish to fry, for now ;-)
As it is, this is more of a rearrangement than I generally
prefer "just to fix a leak".

> Isn't that dangerous, since fscanf is
> undefined in the presence of integer overflow (that is, if fp sends more

True, if it reads 4294967296 (2^32), fscanf is almost guaranteed --
in practice, not by any standard, of course -- to interpret it as 0.

> decimal digits than fit in veid)?  This seems like one of the reasons that
> coreutils completely prohibits *scanf (another being buffer overflow
> exploits with %s, but that's not relevant to this chunk of code).

If we could magically remove all uses of *scanf, atoi, etc.,
that'd be great ;-)  But making so many changes is a little
risky now, considering how little test coverage we have.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]