[libcxx Windows][PATCH] asprintf / vasprintf fix.

[Howard Hinnant]

Below is an anonymously donated patch for libcxx on Windows.  For those interested in libcxx on Windows, please review and recommend either accepting or rejecting this patch.

Thanks,
Howard

------------------
Please find:
a) an attached diff of a proposed fix,
b) an attached test case,
c) this explanation!
 
The vasprintf function in libcxx, as implemented on windows in src\support\win32\support.cpp is broken and it corrupts memory.

Since asprintf calls vasprintf, it too will corrupt memory if called.
 
The exact line causing the corruption is even warned by the compiler itself. The error below is from the svn clang build of libcxx on mingw (via ninja make):
----
[26/27] Building CXX object lib\CMakeFiles\cxx.dir\__\src\support\win32\support.cpp.obj
c:\libcxx\src\support\win32\support.cpp:33:23: warning: expression which evaluates to zero treated as a null pointer constant of type 'char *' [-Wnon-literal-null-conversion]
        sptr[count] = '\0';
----
The warning is highlighting a redundant line of code that is trying to write to an array of pointers instead of an array of characters.
 
If further proof is needed, a test case is attached that demonstrates the problem. It will do that on any system it can successfully compile and run on, though it was written for and tested on mingw.
 
The test case is small, a single file, and includes easy instructions at the top the file.
 
However, the test case should not be neccessary to diagnose the problem as it is obvious and exactly warned about by the compiler.
 
The current code in libcxx that the above error message is referring to, is shown later below, followed by code proposed to be used instead.
 
There are two solutions to the problem:
 
a) A minimum solution is to just remove the line of code with the warning on, the "sptr[count] = '\0';" statement, as it causes the corruption and is not even neccessary.
 
b) An arguably better solution, and the one proposed, is a new version of the function that replaces the old one. A diff file is attached that implements this solution.
Solution a) or b) is acceptable.
 
The perceived value in the b) version of the code proposed is:

a) It uses only vsnprintf, not both vsnprintf AND vsprintf. This theoretically lessens the implementation dependencies by 1.
b) It protects the library should the implementations of these two functions not be match.
c) vsprintf (no n) has no size parameter, so such a problem could cause overwrite in in-exact or mismatching implementations. The proposed version does not suffer that problem.
d) mingw and libcxx mix and match msvc and gcc functions, such setups make implementation mismatch a possibility even if by accident.
e) The proposed version uses just vsnprintf function but also ensures it is consistent with itself and returns an error if not.
 
The current and proposed functions are included below for inspection and are also in the attached test case.
 
If the attached patch is applied, the current vasprintf version shown below will be replaced with the proposed version shown after it.
After consideration, if you opt NOT to use the attached patch, option b, just delete the null assignment line from the existing code in svn and that is option a) implemented.
 
I intend to submit a further patch shortly to this same file, but as this change is of lesser risk of needing to be backed out, then the next patch, I have submitted them separately.
 
// Exact same code as is IN libcxx right now.
int current_vasprintf( char **sptr, const char *__restrict fmt, va_list ap )
{
    *sptr = NULL;
    int count = vsnprintf( *sptr, 0, fmt, ap );
    if( (count >= 0) && ((*sptr = (char*)malloc(count+1)) != NULL) )
    {
        vsprintf( *sptr, fmt, ap );
        sptr[count] = '\0'; // Delete me. Should and does generate a warning on a good compiler that shows something is wrong.
    }
    return count;
}
// Proposed version of asprintf to replace the current version.
// Like sprintf, but when return value >= 0 it returns a pointer to a malloc'd string in *sptr.
// If return >= 0, use free to delete *sptr.
int proposed_vasprintf( char **sptr, const char *__restrict fmt, va_list ap )
{
    *sptr = NULL;
    int count = vsnprintf( NULL, 0, fmt, ap ); // Query the buffer size required.
 if( count >= 0 ) {
     char* p = static_cast<char*>(malloc(count+1)); // Allocate memory for it.
     if ( p == NULL )
         return -1;
        if ( vsnprintf( p, count+1, fmt, ap ) == count ) // We should have used exactly what was required.
            *sptr = p;
        else { // Otherwise something is wrong, likely a bug in vsnprintf. If so free the memory and report the error.
            free(p);
            return -1;
        }
    }
    return count;
}

-------------- next part --------------
A non-text attachment was scrubbed...
Name: support.cpp.asprintf-1.diff
Type: application/octet-stream
Size: 1324 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130330/c4f75bea/attachment.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: asprintf_test_case.cpp
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130330/c4f75bea/attachment.ksh>