Comment 10 for bug 308060

I (finally) finished this up today. I spot checked various static buffers and functions that could have potential problems and overall things seem good. Some areas of note:

1. unsafe use of wcscpy() and strcpy():

msn/xmlParser.cpp has:
#ifdef _XMLWIDECHAR
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)wcscpy(c1,c2); }
        ...
#else
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)strcpy(c1,c2); }
        ...
#endif

This is potentially dangerous if the input is not sanitized. A better, but untested, implementation might be something like:
#ifdef _XMLWIDECHAR
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
           XMLSTR *dest = (XMLSTR)wcsncpy(c1,c2,sizeof(c1));
           dest[sizeof(dest)-1] = L'\0';
           return dest;
        }
        ...
#else
        ...
        XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
            XMLSTR *dest = (XMLSTR)strncpy(c1,c2,sizeof(c1));
            dest[sizeof(dest)-1] = '\0';
            return dest;
        }
        ...
#endif

2. Use of sprintf with a static buffer and no bounds checking:

At line 356 of msn/xmlParser.cpp in XMLNode::openFileHelper():
        sprintf(message,
#ifdef _XMLWIDECHAR
            "XML Parsing error inside file '%S'.\n%S\nAt line %i, column %i.\n%s%S%s"
#else
            "XML Parsing error inside file '%s'.\n%s\nAt line %i, column %i.\n%s%s%s"
#endif
            ,filename,XMLNode::getError(pResults.error),pResults.nLine,pResults.nColumn,s1,s2,s3);

where 'filename' is an unsanitized parameter to XMLNode::openFileHelper(). The good news is that openFileHelper() doesn't appear to be used within libmsn. It would be best to fix this to future-proof its use.

3. Does not check the return value of fread()

In XMLNode::parseFile() from xmlParser.cpp. A short read would result in buf containing uncleared memory. This function appears to be only used in XMLNode::openFileHelper(), which, as mentioned, doesn't appear to be used by libmsn. As before, it would be best to check the return value for possible future use of XMLNode::openFileHelper().

4. Contains an embedded md5 implementation. Noteworthy, but not a security concern.

CONCLUSION: Please fix the use of strcpy() and wcscpy() as discussed in point 2. The XML file parsing could be skipped, but it would be nice if it were fixed also.