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@xxxxxxxxxxxxxxxxxx
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list