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

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.