Unintuitive pointer cross-dependency?

[Roman Matzutt]

Hello,

I recently stumbled upon a strange bug in our project which I could fix
(somehow), but I did not understand what was happening all along. Of
course, I do not want to leave it as it is.

Unfortunately, I cannot quickly provide a minimal working example, but
maybe something like this already occurred.

I was filling JSON objects with string values I obtained from a MySQL
database.

We had the following code:

MYSQL_RES *sqlres = NULL;
MYSQL_ROW row;

snprintf(query_buffer, SQL_QUERY_MAX_LENGTH,
"SELECT MAX(pid) FROM %s;", TABLE_SERVICE_AUTHORIZATIONS);

if (database_query() == -1) {
goto fail; /* Conditionally free memory if not NULL */
}

sqlres = mysql_store_result(conn);
row = mysql_fetch_row(sqlres);

tmpJson = json_pack(
"{sssO}",
"pid",
strdup(row[0]),
"devices",
json_deep_copy(json_object_get(
json_data, "security_configurations"
)));
mysql_free_result(sqlres);

When we later used json_decref(tmpJson), we got a double-free error.
I traced this down to be caused by the last line of the snippet, the
mysql_free_result(sqlres).

How does this interfere with each other?
I assumed that freeing sqlres could invalidate row (I don't know much
about MySQL internals...) and if json_decref tries to free the
corresponding string object...

But then I thought that using strdup() would already suffice to fix the
issue. However, the fix/workaround I came up with looks as follows:

char *test2 = strdup(row[0]);
mysql_free_result(sqlres);
tmpJson = json_pack("{sssO}", "pid", test2, "devices", ...)));
free(test2);

Can anyone explain what happened or even suggest a more elegant solution?

Kind regards,
Roman

[Petri Lehtinen]

This code looks safe to me, except it should leak memory instead of
double freeing it.

The string value is copied internally so you don't need to use
strdup(), just row[0] should be OK.

You should also use the 'o' format instead of 'O' because otherwise
you get a memory leak: json_deep_copy() returns a new reference and
the 'O' format further increases the reference count.

I have no idea why you get a double free, especially because the fix
is essentially the same, you just change the ordering a bit.

Petri

[Roman Matzutt]

Dear Petri,

thank you for your quick answer.
I am also puzzled by the initial strdup() not working.

I wanted to inform you that using strdup(row[0]) was my initial idea of
fixing the issue, I used only row[0] before this. I wanted to take the
risk of (temporarily) introducing memory leaks if it helps debugging.

Also, thank you for your remark on 'o' vs. 'O'.

To give further feedback, I quickly checked whether compiler
optimizations may cause the problem, but have a negative result.

While the "normal" settings produce a glibc error (the double-free) on a
seemingly unrelated later part of the code, where even gdb gets the
backtrace wrong, I tried to use gcc -O0 and to set
mallopt(M_MMAP_THRESHOLD, 4); as suggested in #glibc, without success.

I think I have to further investigate whether sql_free_result() has also
an implication on my MYSQL_ROW variable, i.e., the error eventually is
unrelated to libjansson, but I wanted to ask here first. :)

Kind regards,
Roman

[Roman Matzutt]

Dear Petri,

for clarification: I really think I did not use MySQL correctly and
mysql_free_result() is invalidating the MYSQL_ROW variable, thus row[0]
should be NULL.

Sorry to bother you with this, but thank you again for pointing out that
second memory leak.

Kind regards,
Roman

[Petri Lehtinen]

That's OK :)

Petri

[Roman Matzutt]

Dear Petri,

one more question that arose regarding the "double-free" error.
Could it be that json_decref() frees NULL pointers without further checking?

I found out that accessing a MYSQL_ROW after calling mysql_free_result()
causes segmentation faults for most operations, but regarding
libjansson, only json_decref() crashes.

Kind regards,
Roman

[Petri Lehtinen]

Roman Matzutt wrote:
> one more question that arose regarding the "double-free" error.
> Could it be that json_decref() frees NULL pointers without further
> checking?
>
> I found out that accessing a MYSQL_ROW after calling mysql_free_result()
> causes segmentation faults for most operations, but regarding
> libjansson, only json_decref() crashes.

Calling free() on a NULL pointer is a no-op and doesn't cause a crash.
The double free error you're seeing is a glibc debugging aid that
shows the message if free() is called on the same pointer twice.

If it's possible, you should try to run your program with Valgrind.
You'll probably get a better view to invalid memory reads and writes.

FWIW, each commit made to Jansson itself is automatically tested
through Valgrind on multiple platforms and compilers, and Valgrind
works very well for detecting memory problems.

Petri