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

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?