Committed revision 178544. Howard
On Mar 30, 2013, at 11:48 AM, Howard Hinnant <[email protected]> wrote: > 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><asprintf_test_case.cpp>_______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
