[libvirt] [PATCH] phyp: Fixing possible buffer overflow

[Laine Stump]

  On 08/04/2010 03:00 PM, Eduardo Otubo wrote:
> Hello Laine,
>
> It's been quite a while since we had this discussion about my patch. 
> Sorry about the delay on replaying, I had a congress and I've been 
> sick in the last couple of days. Now going back to work. :-)
>
>> (this new patch is corrupt and doesn't apply. Not sure how you sent it,
>> but I'm guessing your mailer mangled it. It's usually better to only
>> send patches with git send-email, or to attach the output of git
>> format-patch to the mail, rather than pasting it inline)
>
> I don't know what happened, I always send patches using git 
> send-email. I'll recheck next times. Thanks.
>
>>
>> This version is closer *in location* to avoiding the overflow, but it's
>> still comparing the wrong index - it's looking at the index into the
>> full array (ret, ie "i") rather than into the small copy that is used to
>> convert (id_c, ie "j").
>>
>> Also, it still allows writing past the end of the array (i == j == 4, so
>> it can write to id_c[4]). Even allowing a write to id_c[3] would be a
>> problem, since that would overwrite the terminating NULL, thus creating
>> the possibility that virStrToLong_i could overrun the end of the array
>> (or, if virStrToLong_i fails, the VIR_ERROR following it would try to
>> print a string that is not NULL terminated - a *much* worse prospect).
>>
>> Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the loop
>> that I noted in my last mail has been left unchanged (and this memset
>> will be done every time the function is called, even if there is no
>> overflow of string length, so it is *guaranteed* the buffer will be
>> overflowed).
>>
>> Finally, in the case that the output of the exec is too long, this new
>> patch will simply exit from the loop early and return success, rather
>> than logging an error and returning failure.
>>
>> These problems *could* be fixed by 1) changing "i <= 4" to "j < 3", 2)
>> fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
>> case of "j >= 3" into an error instead of just exiting the loop.
>>
>> However... did you look at the counter-proposed patch in my previous
>> email? I'm assuming that either you didn't notice it, or that you're
>> trying to come up with a patch that makes the fewest changes possible
>> while fixing the perceived bug. I think that patch would be a better
>> idea for several reasons, and as long as we're fixing this function, we
>> may as well fix it in the best way possible.
>
> I just saw your patch now, seem reasonable. I understand your points 
> and I believe your patch fixes the bug pretty well. Applied here, 
> compiled and run with some tests, seems pretty ack for me.

I just want to verify: you applied the patch from my mail with 0 
changes, and built/tested with that, correct?