Bug 42865 - MySQL native driver: free() on non-heap pointer; in debug mode abort()
Summary: MySQL native driver: free() on non-heap pointer; in debug mode abort()
Status: CLOSED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
Master old -3.6
Hardware: All All
: medium major
Assignee: Lionel Elie Mamane
URL:
Whiteboard: target:3.6.0 target:3.5.5
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-12 16:40 UTC by Lionel Elie Mamane
Modified: 2012-05-23 05:46 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2011-11-12 16:40:09 UTC
Open a .odb file that uses the native MySQL driver. Click on "tables" in the left column to see the list of available tables.

free() is called on a pointer that was not malloc()ed; on a dbgutil build, this leads to an immediate abort() since commit e0c72547ce57c25db61ec3da6c2f2f2792348c7d, which enables libstdc++ debug mode in dbgutil build.

The root cause is the interplay of how libstdc++ handles empty std::string and mysqlc.uno.so's version script:

libstdc++ does not allocate heap memory for an empty std::string, but makes the data pointer point to a constant, the symbol _S_empty_rep_storage (initialised by the linker). The std::string destructor does something along the lines of:

if (the_string.pData != &_S_empty_rep_storage)
   free(the_string.pData)

Normally, the symbol _S_empty_rep_storage is unique global across the whole process (GNU extension to ELF), so this works well. But the "local: *;" in mysqlc.uno.so's version link script overrides that and makes the _S_empty_rep_storage of mysqlc.uno.so different from the _S_empty_rep_storage of libmysqlcppconn.so. But mysqlc.uno.so's getTables calls libmysqlcppconn.so's getTables, and gets (among others) empty strings as result (wrapped in a SQLString); the first occurrence is the name of the catalog of the table (MySQL does not support catalogs).

When that empty string is destructed within mysqlc.uno.so, the destructor calls free() on libmysqlcppconn.so's _S_empty_rep_storage, and boum!


The best fix would be to add

{
    global:
        _ZNSs4_Rep20_S_empty_rep_storageE;
};

to all version link scripts (to get rid of that problem *everywhere*), but that is not allowed. I filed a request to allow that in GNU ld (http://sourceware.org/bugzilla/show_bug.cgi?id=13406), but even if we get my wish, we need another solution until that version of ld can become our baseline... years away.

Note that exporting that symbol in UDK_3_0_0 (or any other named version) does *not* work, it needs to be the anonymous version to be compatible with system DSOs such as mysqlcppconn.so.


Removing the "local: *;" from all link scripts would lead to massive symbol leak, defeating the purpose of link version scripts. Unless we do something like *automatically* adding each and every symbol there, except for "_ZNSs4_Rep20_S_empty_rep_storageE", e.g. with a script that does:

1) Link a first time without .map file
2) use nm/"objdump -T" to get symbols
3) add to .map file every symbol not already mentioned in another section, except for _ZNSs4_Rep20_S_empty_rep_storageE.
4) Link again with .map file


Anybody got a better idea?


Note that this problem is probably not MySQL-specific, nor Base-specifing; it will crop up with any external C++ lib that provides us with (potentially empty) std::string.
Comment 1 Caolán McNamara 2011-11-14 01:17:16 UTC
This presumably comes from -D_GLIBCXX_DEBUG which is fragile when linking to external c++ libraries that weren't built with that enabled.

This is with internal libmysqlcppconn right ?, In --enable-dbgutil are we passing  -D_GLIBCXX_DEBUG to the compiler when building both libmysqlcppconn.so and mysqlc.uno.so or are we missing it in one ? 

Or is there no degree of hackery which will suffice and it's a matter of dropping -D_GLIBCXX_DEBUG when building in --enable-dbgutil mode with enabled mysql connector.

caolanm->mstahl: a fun tangle for you to poke through I guess :-)
Comment 2 Lionel Elie Mamane 2011-11-14 02:21:51 UTC
(In reply to comment #1)
> This presumably comes from -D_GLIBCXX_DEBUG which is fragile when linking to
> external c++ libraries that weren't built with that enabled.

My analysis is that the root problem does not come from -D_GLIBCXX_DEBUG; the latter merely transforms a "survivable error" into an abort().

> This is with internal libmysqlcppconn right ?

Yes, that is forced by ./configure in --enable-dbgutil

> In --enable-dbgutil are we
> passing  -D_GLIBCXX_DEBUG to the compiler when building both libmysqlcppconn.so
> and mysqlc.uno.so or are we missing it in one ? 

I checked, we pass it in both.

> Or is there no degree of hackery which will suffice and it's a matter of
> dropping -D_GLIBCXX_DEBUG when building in --enable-dbgutil mode with enabled
> mysql connector.

Dropping  -D_GLIBCXX_DEBUG is not a complete fix; it might let LibreOffice survive the error, but free() is still called on a pointer it should not be called on.
Comment 3 Lionel Elie Mamane 2011-11-14 03:03:09 UTC
(In reply to comment #2)
> (In reply to comment #1)

>> Or is there no degree of hackery which will suffice

Well, I described two different hackeries that could work in my original message:

1) Remove "local: *;" from the map file, replacing it (or not) with a list of all symbols (except _ZNSs4_Rep20_S_empty_rep_storageE and the ones matched by other rules)

2) Patched GNU ld; we *certainly* don't want to go that route :-)

>> and it's a matter of
>> dropping -D_GLIBCXX_DEBUG when building in --enable-dbgutil mode with enabled
>> mysql connector.

> Dropping  -D_GLIBCXX_DEBUG is not a complete fix; it might let LibreOffice
> survive the error, but free() is still called on a pointer it should not be
> called on.

Actually, after further testing, -D_GLIBCXX_DEBUG seems to make a difference with respect to the problematic symbol; that symbol seems to be exported as a "global unique" symbol only when compiled with -D_GLIBCXX_DEBUG. So possibly dropping -D_GLIBCXX_DEBUG is a solution after all.
Comment 4 Michael Stahl (CIB) 2011-11-14 14:21:00 UTC
hmmm... very mysterious...

nm solver/unxlngx6/lib/libmysqlcppconn.so
 00000000004790c0 u _ZNSs4_Rep20_S_empty_rep_storageE

man nm says:
    "u" The symbol is a unique global symbol.  This is a GNU extension to the
        standard set of ELF symbol bindings.  For such a symbol the dynamic
        linker will make sure that in the entire process there is just one
        symbol with this name and type in use.

this symbol is there because when defining _GLIBCXX_DEBUG various
explicit template instantiations are disabled by defining
_GLIBCXX_EXTERN_TEMPLATE to 0, among them:

  // Inhibit implicit instantiations for required instantiations,
  // which are defined via explicit instantiations elsewhere.
 #if _GLIBCXX_EXTERN_TEMPLATE > 0
  extern template class basic_string<char>;

the "extern template" apparently prevents GCC from generating any
definitions from the template.

but with the "unique" global linkage that should not be a problem,
as the symbol should be resolved to one definition.

and indeed that is what seems to be happening for me here:

LD_DEBUG=bindings ./soffice

     19250:	binding file /data/lo/core/solver/unxlngx6/installation/opt/share/extensions/mysql-connector-ooo/libmysqlcppconn.so [0] to /data/lo/core/solver/unxlngx6/installation/opt/program/../ure-link/lib/libstdc++.so.6 [0]: normal symbol `_ZNSs4_Rep20_S_empty_rep_storageE'

so i don't understand what is going wrong here:
Lionel, can you check whether your symbol resolves
properly to libstdc++.so.6 as well?

also, it does not crash for me but i do not actually connect to a database,
i just get an error message if i click on "tables"; is that supposed to crash?
Comment 5 Michael Stahl (CIB) 2011-11-14 14:37:39 UTC
err, oops, somehow i haven't read your report properly,
as in i completely missed that there is another library
(mysql.uno.so) involved (and the darn thing isn't even
delivered to the solver...).

nm mysqlc/unxlngx6/lib/mysqlc.uno.so
 00000000003e12e0 b _ZNSs4_Rep20_S_empty_rep_storageE

indeed it has the same thing as non-global symbol...
Comment 6 Lionel Elie Mamane 2011-11-14 19:59:34 UTC
(In reply to comment #4)

> also, it does not crash for me but i do not actually connect to a database,
> i just get an error message if i click on "tables"; is that supposed to crash?

You need to actually connect to a database, enter login information and click "OK". The crash (well, the abort()) happens in mysqlc.uno.so's ODatabaseMetaData::getTables():

    try {
        std::auto_ptr< sql::ResultSet> rset( meta->getTables(...);
        rtl_TextEncoding encoding = m_rConnection.getConnectionEncoding();
        sql::ResultSetMetaData * rs_meta = rset->getMetaData();
        sal_uInt32 columns = rs_meta->getColumnCount();
        while (rset->next()) {
            std::vector< Any > aRow(1);
            bool informationSchema = false;
            for (sal_uInt32 i = 1; (i <= columns) && !informationSchema; ++i) {
                sql::SQLString columnStringValue = rset->getString(i);
                ...
            }
        ...
    }

For i=1, this is the table's catalog. As MySQL does not have catalogs, this is a constant std::string("") (wrapped in a sql::SQLString), from mysqlcppconn.so's MySQL_Prepared_ResultSet::getString:

        if (*result_bind->rbind[columnIndex - 1].is_null) {
                return sql::SQLString("");
        }

The sql::SQLString object is just a wrapper around std::string:

namespace sql
{
        class CPPCONN_PUBLIC_FUNC SQLString
        {
                std::string realStr;
         // methods
         }
}


So, the sql::SQLStrong.realStr object is constructed in mysqlcppconn.so, with its pData pointer to libstdc++'s _S_empty_rep_storage. But it is destructed in mysql.uno.so, which has its own local _S_empty_rep_storage.



(In reply to comment #5)
> nm mysqlc/unxlngx6/lib/mysqlc.uno.so
>  00000000003e12e0 b _ZNSs4_Rep20_S_empty_rep_storageE

> indeed it has the same thing as non-global symbol...

And that's because of mysql.uno.so's link version script file (mysqlc/source/mysqlc.map -> mysqlc/unxlngx6/misc/mysqlc_mysqlc.uno.map). That symbol matches the:

UDK_3_0_0 {
    local:
        *;
};

If you remove that wildcard and re-link, mysql.uno.so has _S_empty_rep_storage as "u" and all is well.
Comment 7 Stephan Bergmann 2011-11-15 01:53:33 UTC
For the specific case of the mysqlc.uno dynamic library, an easy solution is to let it use VISIBILITY_HIDDEN=TRUE and SHL1USE_EXPORTS=name instead of SHL1VERSIONMAP=mysqlc.map.  Pushed as <http://cgit.freedesktop.org/libreoffice/core/commit/?id=d805a6c0931ee5529de89f0b431e649f92bc34e8> "Use visibility instead of version map file for mysqlc.uno."

Lionel, can you check whether that solves the abort for you?
Comment 8 Lionel Elie Mamane 2011-11-15 02:54:23 UTC
(In reply to comment #7)
> For the specific case of the mysqlc.uno dynamic library, an easy solution is to
> let it use VISIBILITY_HIDDEN=TRUE and SHL1USE_EXPORTS=name instead of
> SHL1VERSIONMAP=mysqlc.map.  Pushed as
> <http://cgit.freedesktop.org/libreoffice/core/commit/?id=d805a6c0931ee5529de89f0b431e649f92bc34e8>
> "Use visibility instead of version map file for mysqlc.uno."

> Lionel, can you check whether that solves the abort for you?

Yes, it does, but:

1) Won't that break binary compatibility of mysql-connector-ooo compiled/linked against a version with this patch with a LO without this patch and/or vice-versa? That would be OK-ish if active only in debug mode (as binary compatibility is sacrificed anyway by using -D_GLIBCXX_DEBUG), except that it make *yet* a difference between debug and non-debug mode, and those are best kept at a minimum.

2) More importantly, IMO we have a bigger problem; we need to apply whathever solution we choose over the *whole* of LO.

The situation is that we cannot pass a std::string (even as member of another class) that is not guaranteed not to be empty from one library to another (if one of them uses a .map file with "local: *;") or from one library to the main executable. As LO is structured as a boatload of libraries, we'll have other problems like that... Luckily LO does not use std::string ubiquitously, but still:

$ git grep -l -E '#include[[:space:]]*<string>' | wc -l
156

over many different modules:

$ git grep -l -E '#include[[:space:]]*<string>' | sed 's@/.*@@' | sort -u | wc -l
43


Also "luckily" the problem arises only in debug mode, although I'm not sure it would not arise on e.g. a basic_string<int> in non-debug mode (since that would not have an explicit template specialisation).
Comment 9 Stephan Bergmann 2011-11-15 04:58:08 UTC
1  "Won't that break binary compatibility"  No, I don't think so:

If the old --enable-dbgutil built mysqlc.uno.so (with the symbol as "b") is used in any LO installation, it will trigger the bug discussed in this issue.

If the new --enable-dbgutil biult mysqlc.uno.so (with the symbol as "u") is used in any LO installation, that installation will consistently use the symbol from libstdc++.so.6 (or perhaps mysqlc.uno.so or any other object exporting it as "u").

If the old or new --disable-dbgutil built mysqlc.uno.so (with the symbol as *UND*) is used in any LO installation, it will resolve the symbol against libstdc++.so.6 (or perhaps consistently against any other object exporting it as "u").

(All modulo me understanding all this correctly.)


2  "need to apply whathever solution we choose over the *whole* of LO"  Yes:

The only dynamic libraries that likely need a version map file at all (as they are part of the URE interface) are cppu, cppuhelper, sal, and salhelper.  None of them currently has the described problem.

All other dynamic libraries should use the visibility feature.  They need to be adapted accordingly.
Comment 10 Michael Stahl (CIB) 2011-11-15 12:15:14 UTC
have tracked down all locally defined _S_empty_rep_storage symbols,
and replaced the version script with explicit SAL_DLLEXPORT and
hidden visibility, so i only have global _S_empty_rep_storage
symbols now.

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c506e1852af6605c97b2194df95a0810fd42b3aa
http://cgit.freedesktop.org/libreoffice/core/commit/?id=5f1799f57978bb9accfe59fb5bc38d01686441b6
http://cgit.freedesktop.org/libreoffice/core/commit/?id=a9da5a0bd3631370dfed0c285440c3252c6daf3a
http://cgit.freedesktop.org/libreoffice/core/commit/?id=09eabcfd91cdc94228b758c393daa5aaab3de378
http://cgit.freedesktop.org/libreoffice/core/commit/?id=48dbaa517f1a72a9bf11fa4db7425510c86c677b
http://cgit.freedesktop.org/libreoffice/core/commit/?id=1fb5eb2162d79a44f5bb90627c1ca340d1322129

have various extra options and extensions enabled,
but probably i'm not building every library...

by the way, gbuild currently does not even support mapfiles,
only explicit SAL_DLLEXPORT,
so as we will gradually convert the rest of the libraries
this kind of problem should go away automatically.

it seems to be working fine:

      2253:	binding file /data/lo/core/solver/unxlngx6/installation/opt/program/../share/extensions/mysql-connector-ooo/mysqlc.uno.so [0] to /data/lo/core/solver/unxlngx6/installation/opt/program/../ure-link/lib/libstdc++.so.6 [0]: normal symbol `_ZNSs4_Rep20_S_empty_rep_storageE'

      2253:	binding file /data/lo/core/solver/unxlngx6/installation/opt/program/libmozabdrvlo.so [0] to /data/lo/core/solver/unxlngx6/installation/opt/program/../ure-link/lib/libstdc++.so.6 [0]: normal symbol `_ZNSs4_Rep20_S_empty_rep_storageE'

Lionel, can you please verify that with the changes you
no longer get crashes, and also that you don't have a local
symbol anywhere.

for so in */unxlngx6/lib/*.so; do echo $so; nm $so | grep S_empty_rep_storage; done
Comment 11 Lionel Elie Mamane 2011-11-16 02:51:53 UTC
(In reply to comment #9)
> 1  "Won't that break binary compatibility"  No, I don't think so:

> ---> discussion of _S_empty_rep_storage symbol

My worry was not wrt to the _S_empty_rep_storage symbol, but wrt to the symbols exported versioned by the old map file: component_writeInfo and component_getFactory. "Old" mysqlc.uno.so had these symbols with version UDK_3_0_0, but "new" mysqlc.uno.so has them unversioned. But maybe LO looks for these symbols versioned *and* unversioned when loading a .uno.so?
Comment 12 Stephan Bergmann 2011-11-16 03:42:48 UTC
Re Comment 11:  Ah, no problem there.  Those component_* symbols are only accessed via dlsym, so no version information involved.
Comment 13 Lionel Elie Mamane 2011-11-16 10:26:52 UTC
(In reply to comment #10)

> Lionel, can you please verify that with the changes you
> no longer get crashes, and also that you don't have a local
> symbol anywhere.

> for so in */unxlngx6/lib/*.so; do echo $so; nm $so | grep S_empty_rep_storage;
> done

Yes, that returns only "global unique" symbols. Connecting to a MySQL DB and browsing tables does not cause a crash.
Comment 14 Michael Stahl (CIB) 2011-11-17 07:47:28 UTC
guess we can close this then.

btw, thanks for the excellent bug report Lionel!
Comment 15 Not Assigned 2012-05-18 07:42:17 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5729da18a1852d842bd7de05d9d7fdcbec1e9ad5

fdo#42865: privatized unique empty string symbol:
Comment 16 Not Assigned 2012-05-23 05:46:28 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-3-5":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7cbaadc92f797ac718d9c49036796b4047683ca9&g=libreoffice-3-5

fdo#42865: privatized unique empty string symbol:


It will be available in LibreOffice 3.5.5.