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;
}
support.cpp.asprintf-1.diff
Description: Binary data
#include <cstdio>
#include <cassert>
#include <cstdarg>
// TEST CASE TO SHOW LIBCXX BUG IN VASPRINTF. COMPILES ON MINGW AND MAYBE
OTHERS.
// ON WHATEVER SYSTEM IT SUCCEEDS IN COMPILING ON, IT SHOULD DEMONSTRATE THE
PROBLEM IN LIBCXX FOR MINGW.
// IT SHOULD FAIL THE FIRST TIME WITH AN ASSERTION AND PRINTF A MESSAGE TO SHOW
THE BUG.
// ONCE THIS HAPPENS, CHANGE THE LINE BELOW FROM 0 TO 1 TO ENABLE THE PROPOSED
VERSION THAT FIXES THE BUG.
// THEN COMPILE AND RUN AGAIN.
// IF NO ASSERTION/MESSAGE RESULTS, THE BUG SHOULD BE FIXED.
#define USE_CURRENT_ASPRINTF 0
// 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;
}
// 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'; // Should and does generate a warning on a good
compiler, shows you something is wrong.
}
return count;
}
// Exact same code as is in libcxx, support\win32\support.cpp except modified
// to have a new name, that wont clash and to conditionally call either the
current or the proposed vasprintf.
int libcxx_asprintf(char **sptr, const char *__restrict fmt, ...)
{
va_list ap;
va_start(ap, fmt);
#if USE_CURRENT_ASPRINTF
int result = current_vasprintf(sptr, fmt, ap);
#else
int result = proposed_vasprintf(sptr, fmt, ap);
#endif
va_end(ap);
return result;
}
static void test_asprintf_overwrite()
{
char* const expected_value = ( char* ) -1; // Any non 0 value is fine.
char msg[] = { "hello" };
const int msg_len = sizeof msg - 1;
char* sptr[msg_len+1] = { NULL }; // First element of sptr is
initialized to NULL, it should change.
sptr[msg_len] = expected_value; // And last element is initialized to a
non 0 expected value.
// asprintf SHOULD change sptr[0] to point to a newly malloc'd string,
// containing the same contents as msg and that's all.
// asprintf should NOT change any other element of sptr, epsecially the
last element.
// If it does, it has to be because of corruption by asprintf/vasprintf
(one calls the other).
// as no other elements of sptr are passed to asprintf and if asprintf is
not called, the change does not occur.
libcxx_asprintf( &sptr[0], "%s", msg );
if ( sptr[0] == NULL ) {
printf( "asprintf/vsprintf test case is wrong\n" );
assert( false );
}
if ( sptr[ msg_len ] != expected_value ) {
printf( "asprintf/vsprintf has caused corruption\n" );
assert( false );
}
printf( "asprintf is working!\n" );
}
int main()
{
test_asprintf_overwrite();
}
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
