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

[Eduardo Otubo]

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.

Thanks for the help :-)

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com