Comment 18 for bug 308060

Kees Cook wrote:
> On Fri, Mar 20, 2009 at 02:09:33PM -0000, Aurélien Gâteau wrote:
>> http://launchpadlibrarian.net/24149888/02-avoid_potential_buffer_overrun.diff
>
> "n" should be verified to be !=0, so that result[-1] isn't used.
[snip]

> This change does not have the same functionality (in toXMLStringUnSafe):
[snip]
> the original can copy up to 4 characters (notice the lack of breaks).
>
> Also, nothing in the loop is testing that "length" is remaining positive.
> This should be checked in toXMLStringUnSafe, CreateXMLStringR (if nResult
> is ever >= length, abort), and in _tcscpy (where it should abort if n<0).
[snip]

I agree with these remarks (shame on me for the switch() error), I went
to fix those bugs, but after further inspection, I believe the changes
are not necessary or at least not at the right place:

_tcspy() is called from CreateXMLStringR() and toXMLStringUnSafe() only.
- CreateXMLStringR() can be called with a null pointer, in this case it
compute the necessary buffer size. This is what CreateXMLString(), the
only caller, does.

- toXMLStringUnSafe() is called from CreateXMLStringR(), which is fine
and from toXML(), which compute the source string length before.

All in all I think the code is fine, or that the fix should be on the
malloc()/realloc() calls which are done after computing the buffer lengths.