MySQL++

MariaDB 10.4.x needs mysql_library_init() for escape_string_no_conn()
Login

MariaDB 10.4.x needs mysql_library_init() for escape_string_no_conn()

(1) By Luke Mewburn (lukemewburn) on 2020-07-07 02:02:14 [link]

## Summary

MariaDB 10.4.13 needs `mysql_library_init()` called before `mysql_escape_string()` or `mysql_cset_escape_slashes()` are called without a connection. See <https://jira.mariadb.org/browse/MDEV-20116>

Without calling mysql_library_init() first, various test programs that test without connection fault with SEGV.

## Fix

I worked around this with the following patch. This method does not require specific changes to MySQL++ consumer apps besides a recompile. The maintainers may prefer a different solution.

        Ensure mysql_library_init() is called in escape_string_no_conn()
        before mysql_escape_string().  Fixes SEGV crashes in test applications.
        Refer to https://jira.mariadb.org/browse/MDEV-20116 for more information.

        diff -pu -r mysql++-3.2.5-orig/lib/dbdriver.cpp mysql++-3.2.5/lib/dbdriver.cpp
        --- mysql++-3.2.5-orig/lib/dbdriver.cpp 2020-02-28 07:31:27.000000000 +1100
        +++ mysql++-3.2.5/lib/dbdriver.cpp      2020-07-06 13:24:52.966530258 +1000
        @@ -196,6 +196,7 @@ size_t
         DBDriver::escape_string_no_conn(std::string* ps, const char* original,
                        size_t length)
         {
        +       mysql_library_init(0, 0, 0);
                if (ps == 0) {
                        // Can't do any real work!
                        return 0;
        diff -pu -r mysql++-3.2.5-orig/lib/dbdriver.h mysql++-3.2.5/lib/dbdriver.h
        --- mysql++-3.2.5-orig/lib/dbdriver.h   2020-02-28 07:31:27.000000000 +1100
        +++ mysql++-3.2.5/lib/dbdriver.h        2020-07-06 13:25:04.762282341 +1000
        @@ -266,6 +266,7 @@ public:
                static size_t escape_string_no_conn(char* to, const char* from,
                                size_t length)
                {
        +               mysql_library_init(0, 0, 0);
                        return mysql_escape_string(to, from,
                                        static_cast<unsigned long>(length));
                }




## Details

(Note: this is on CentOS 7.8, MariaDB 10.4.13, compiling with devtoolset-7 from SCL using `scl enable devtoolset-7 './configure && make'`. The newer compiler should not be specifically triggering this issue).

When building MySQL++ 3.2.5 with MariaDB 10.4.13, various test applications SEGV:

    ./dtest
    Running unit tests: test_manip FAILED (139)

    ./exrun: line 66: 29400 Segmentation fault      (core dumped) LD_LIBRARY_PATH=. $TOOL ./$PROG $*

Further analysis shows that the code is faulting in `mysqlpp::DBDriver::escape_string_no_conn()`:

    env LD_LIBRARY_PATH=. gdb ./test_manip
    (gdb) run
    Starting program: test_manip 
    Program received signal SIGSEGV, Segmentation fault.
    0x00007ffff7b9ec00 in mysql_cset_escape_slashes () from /lib64/libmariadb.so.3
    Missing separate debuginfos, use: debuginfo-install MariaDB-shared-10.4.13-1.el7.centos.x86_64 keyutils-libs-1.5.8-3.el7.x86_64 krb5-libs-1.15.1-46.el7.x86_64 libcom_err-1.42.9-17.el7.x86_64 libgcc-4.8.5-39.el7.x86_64 libselinux-2.5-15.el7.x86_64 libstdc++-4.8.5-39.el7.x86_64 openssl-libs-1.0.2k-19.el7.x86_64 pcre-8.32-17.el7.x86_64 zlib-1.2.7-18.el7.x86_64
    (gdb) bt
    #0  0x00007ffff7b9ec00 in mysql_cset_escape_slashes () from /lib64/libmariadb.so.3
    #1  0x00007ffff794872d in mysqlpp::DBDriver::escape_string_no_conn (length=28, 
        from=0x60a9c0 "Doodle me, James, doodle me!", to=0x60ac40 "") at ./lib/dbdriver.h:270
    #2  mysqlpp::DBDriver::escape_string_no_conn (ps=ps@entry=0x7fffffffdd08, 
        original=0x60a9c0 "Doodle me, James, doodle me!", length=length@entry=28) at ./lib/dbdriver.cpp:217
    #3  0x00007ffff79512a3 in mysqlpp::Query::escape_string (this=this@entry=0x7fffffffdee0, 
        ps=ps@entry=0x7fffffffdd08, original=<optimized out>, length=length@entry=28)
        at ./lib/query.cpp:120
    #4  0x00007ffff794ab91 in mysqlpp::operator<< (o=..., o@entry=..., in=...) at ./lib/manip.cpp:73
    #5  0x0000000000402dd8 in explicit_query_quote<char*> (
        test=test@entry=0x7fffffffe1e0 "Doodle me, James, doodle me!", len=len@entry=28)
        at ./test/manip.cpp:59
    #6  0x0000000000403501 in test<char*> (test=0x7fffffffe1e0 "Doodle me, James, doodle me!", len=28)
        at ./test/manip.cpp:149
    #7  0x0000000000401dc8 in main () at ./test/manip.cpp:162

When I apply the patch above and rebuild, the tests succeed. Some of the output of the examples fails, but at least there's no more coredumps:

    ./dtest -uTESTUSER -pTESTPASS
    Running unit tests: 16 tests succeeded
    Running examples:
       resetdb simple1 simple2 simple3 store_if for_each multiquery tquery1
       resetdb tquery2 tquery3 tquery4
       resetdb ssqls1 ssqls2 ssqls3 ssqls4 ssqls5 ssqls6 load_jpeg cgi_jpeg
    Testing ssqlsxlat -i examples/*.ssqls -o...pass 1, pass 2, diffdiff: /tmp/dtest-ssxgv2-pass1-: No such file or directory
    diff: /tmp/dtest-ssxgv2-pass2-: No such file or directory
    [...]

(2) By Warren Young (tangent) on 2020-07-07 05:11:19 in reply to 1 [link]

> I worked around this with the following patch

I don't like redundantly calling that function where you do. It's a fairly hot path, so we should not make calls that are 99.9% of the time pointless.

`mysql_library_init()` is automatically called in the normal use cases.

In this specific case, the reason your call path fails is that trying to escape a MySQL/MariaDB string without a connection is somewhat bogus from the start, because proper escaping depends on settings that can vary by DB server configuration and by per-database configuration *on* that server.

When you tell the library to escape your string without that info, it's giving you a kind of best-guess result, which may not be correct.

Bottom line, I'd say the proper solution is to either:

1. Get a `Connection` first, and call through that. Then the underlying C API library will be initted, and the escaping will be done properly.

2. Call `mysql_library_init()` *once* from your program's startup code.

(3) By Luke Mewburn (lukemewburn) on 2020-07-07 06:18:19 in reply to 2 [link]

The issue is in the mysql++ test applications dumping core, not in my code.

I instead propose the following fixes:

1. Change the `CommandLine::CommandLine()` to call `mysql_library_init()`, since all the test apps appear to use that?
2. Explicitly document that `mysql_library_init()` should be called at the start of an application before using:
  1. `DBDriver::escape_string_no_conn()`
  2. `Query::escape_string()` without a connection
  3. `SQLStream::escape_string()` with a connection


thoughts?

(4) By Warren Young (tangent) on 2020-07-07 11:49:22 in reply to 3 [link]

> The issue is in the mysql++ test applications dumping core, not in my code.

Didn't notice that. This has to be a C API level regression of some kind, because the code as-is has been working for years and years.

> Change the CommandLine::CommandLine() to call mysql_library_init()

Better.

> Explicitly document that mysql_library_init() should be called at the start of an application before using: 

No, because some other bit of code might have initted the library via another path.

Bottom line, this is supposed to be an internal hidden detail of the C API's implementation, not something normal code is supposed to care about. At least, that's what it was back when all of this was designed. Obviously these DBMSes have gone through multiple changes of hands, so now we're getting mismatches as new minds come along and decide that old decisions are no longer correct, breaking code like mine.

> thoughts?

You're welcome to provide a patch for this.

(5) By Warren Young (tangent) on 2020-07-07 11:51:11 in reply to 4 [link]

> The issue is in the mysql++ test applications dumping core, not in my code.

Also, this explains why we're calling these functions without a Connection: we don't want to have a MySQL DBMS up and running just to run the tests. We're happy with best-effort escaping and quoting in this case.

It isn't good practice normally, but in a test program...

(6) By Luke Mewburn (lukemewburn) on 2020-07-08 04:45:15 in reply to 4 [link]

> > Change the CommandLine::CommandLine() to call mysql_library_init()

> Better.

Actually, that turns out not to work, because the 3 test apps that coredump don't use CommandLine. I've instead just added the call to `mysql_library_init()` to the start of `main()` in:

* test/manip.cpp
* test/qssqls.cpp
* test/sqlstream.cpp

That fixes the coredumps; all `./dtest` unit tests pass now, and if I supply DB creds all of the examples pass too.

Patch below.


> > Explicitly document that mysql_library_init() should be called at the start of an application before using:

> No, because some other bit of code might have initted the library via another path.

The `mysql_library_init()`should be safe to call multiple times in an application. The MariaDB implementation uses `pthread_once()` to ensure that it's only invoked once. The method already gets called implicitly by `mysql_init()` anyway for **each** Connection constructed my mysql++ anyway.


> Bottom line, this is supposed to be an internal hidden detail of the C API's implementation, not something normal code is supposed to care about. At least, that's what it was back when all of this was designed. Obviously these DBMSes have gone through multiple changes of hands, so now we're getting mismatches as new minds come along and decide that old decisions are no longer correct, breaking code like mine.

It is an unfortunate behaviour change made in the MariaDB connector that now effectively requires `mysql_library_init()` when previous code linked against the older MySQL connector "just worked" (even if it shouldn't have).

I still think it may be beneficial to document (even just a a FAQ) to the end users of your library who are also using the (newer) MariaDB connector implementation that in the edge case of "using escaping without at least constructing a Connection once" should call `mysql_library_init()` to avoid the run-time SEGV experienced in the testsuite here caused by the behaviour change in the upstream MariaDB database connector. YMMV.

Here's a diff that worked.

```
diff --git i/test/manip.cpp w/test/manip.cpp
index 13c6e0a..a0264b1 100644
--- i/test/manip.cpp
+++ w/test/manip.cpp
@@ -155,6 +155,8 @@ test(T test, size_t len)
 int
 main()
 {
+       mysql_library_init(0, 0, 0);
+
        char s[] = "Doodle me, James, doodle me!";
        const size_t len = strlen(s);
 
diff --git i/test/qssqls.cpp w/test/qssqls.cpp
index 14062b9..a87d941 100644
--- i/test/qssqls.cpp
+++ w/test/qssqls.cpp
@@ -59,6 +59,8 @@ sql_create_19(test,
 int
 main()
 {
+       mysql_library_init(0, 0, 0);
+
        Query q(0);             // don't pass 0 for conn parameter in real code
        test empty(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, false,
                        Date(), Time(), DateTime(), "", sql_blob());
diff --git i/test/sqlstream.cpp w/test/sqlstream.cpp
index e0c127a..d17c73f 100644
--- i/test/sqlstream.cpp
+++ w/test/sqlstream.cpp
@@ -65,6 +65,8 @@ sql_create_23(test,
 int
 main()
 {
+       mysql_library_init(0, 0, 0);
+
        SQLStream sqls(0);              // don't do this in real code
        test empty(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, false,
                        Date(), Time(), DateTime(),"","","","","","");

```

(7) By Warren Young (tangent) on 2020-07-10 20:36:33 in reply to 6

[Applied](d8def9fe7b2). Thanks!