1

EDIT: Thanks to everyone who pointed out the problem, and that it was discussed on Stack Overflow. I cast the last close vote myself.

A related question: neither CPP Reference on ostringstream or ostringstream::str state its a temporary. How did so many people know? Or is there different documentation I should have consulted?


I'm having a lot of trouble with memory errors under Debian 7.3 (x64) with GCC 4.7.2, -std=c++11 and std::ostringstream. Its leading to bizaare results like https://stackoverflow.com/questions/21260815/which-r-in-this-create-table-error-message.

The full blown program uses Sqlite. Running a reduced case from the command line vs. Valgrind prints 2 different errors. The entire reduced case program is available at Code Viewer (I thought it was kind of long to post the entire sample here). All it does is initializes Sqlite, opens a database, creates a table, closes the database, and unitializes the database. And it reports errors if they occur. There's nothing else occurring.

Here's part of the reduced case program that simply tries to create a table. If the table exists, it should produce an error (which it does):

ostringstream qs;
qs.str().reserve(96);

qs << "CREATE TABLE test";
qs << "(";
qs << "  userid INTEGER PRIMARY KEY AUTOINCREMENT,";
qs << "  username TEXT,";
qs << "  salt BLOB,";
qs << "  hmac BLOB";
qs << ");";

const char* stmt = qs.str().c_str();
AC_ASSERT(NULL != stmt);

rc = sqlite3_exec(db, stmt, NULL, NULL, &err);
AC_ASSERT(rc == SQLITE_OK);

if(rc != SQLITE_OK)
{
    ostringstream oss;
    oss.str().reserve(96);

    oss << "sqlite3_exec failed, error " << rc;
    LogError(oss.str().c_str());

    oss.clear(), oss.str("");
    oss << "Sqlite error: " << err;
    LogError(oss.str().c_str());

    // Break, handle error
}

However, under command line, the message is:

sqlite3_exec failed, error 1
Sqlite error: table 

Under Valgrind, the message is:

sqlite3_exec failed, error 1
Sqlite error: table test already exists

The program makes heavy use of ostringstream, and Valgrind produces nearly 13 issues centered around them, 9 of which include operator delete(void*) on the underlying basic_string. For example, one is shown below (and line 76 from t.cpp is const char* stmt = qs.str().c_str();):

==14318== Invalid read of size 1
==14318==    at 0x45ACC8: sqlite3_exec (sqlite3.c:94542)
==14318==    by 0x405D07: main (t.cpp:79)
==14318==  Address 0x5d89728 is 24 bytes inside a block of size 127 free'd
==14318==    at 0x4C27870: operator delete(void*) (vg_replace_malloc.c:502)
==14318==    by 0x530EB1F: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17)
==14318==    by 0x405CF1: main (t.cpp:76)

Does anyone have any ideas of what's going on here? Is it ostringstream? Or perhaps GCC 4.7.2? Or maybe Debian's port?

(And sorry for the open ended question. I've run out of things to do).

Community
  • 1
  • 1
jww
  • 97,681
  • 90
  • 411
  • 885
  • 4
    `const char* stmt = qs.str().c_str();` Bad bad bad. – Simple Jan 22 '14 at 12:36
  • @Simple lets see who is the first to find the duplicate ;) – BЈовић Jan 22 '14 at 12:37
  • @BЈовић - not that hard, I just wrote ".str().c_str() const char*" in the search field in the close form and this was one of the first suggestions :P – Kiril Kirov Jan 22 '14 at 12:40
  • 4
    I know this isn't the answer but `str()` returns a copy of the buffer so doing `str().reserve(x)` is futile. – David G Jan 22 '14 at 12:42
  • 1
    yeah, I was wondering whats this thing: `str().reserve(x)` doing – marcinj Jan 22 '14 at 12:45
  • Oh, shit.... I never knew that. And that damn [CCP Reference 1](http://www.cplusplus.com/reference/sstream/ostringstream/) and [CCP Reference 2](http://www.cplusplus.com/reference/sstream/ostringstream/str/) does not state it's a temporary. – jww Jan 22 '14 at 12:50
  • 2
    @noloader it does. Notice the return type is `string` not `string&`. [This reference](http://en.cppreference.com/w/cpp/io/basic_stringstream/str) explicitly mentions this possible bug. – Simple Jan 22 '14 at 12:55
  • Simple - returning a reference does not preclude it from being a non-temporary object (or am I missing something?). I always thought there was an underlying `string` backing the `stringstream`, and to get to it you simply called `.str()`. – jww Jan 22 '14 at 12:58

2 Answers2

6
const char* stmt = qs.str().c_str();

That extracts a temporary string from qs, takes a pointer to its content, then destroys the temporary leaving the pointer dangling. Using the pointer after this will give undefined behaviour.

To fix it, you could either assign the result of str() to a variable, so that it's no longer temporary, or use this expression as the argument to sqlite3_exec, so that the temporary survives until after that function call. (In the second case, you'd have to remove the first assertion; but that assertion is rather pointless anyway).

Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • Thanks Mike. Now that I know what's going on, I'll be able to fix things. Related: how did people know that was a temporary (have they been burned in the past, or is it just common knowledge)? The two references I used did not state that's a temporary object. Is that behavior specified by the C++ standard? – jww Jan 22 '14 at 13:06
  • 1
    @noloader: I can't speak for people, but I learnt about temporaries by being burned like this. It is specified behavoiur: `str()` returns a string by value; return values are temporary; temporaries are destroyed at the end of the full-expression that creates them (with an exception that doesn't apply here). – Mike Seymour Jan 22 '14 at 13:22
5

Take a closer look at this line: const char* stmt = qs.str().c_str();

qs.str() provides you a temporary std::string object from which you take a pointer to inner char* array. Once this line completes its execution, your pointer is no longer valid and something else may (and probably is) stored there.

Try doing this instead:

std::string strstmt(qs.str());
const char* stmt = strstmt.c_str();