Bug 846602 - MySQL C API missing THR_KEY_mysys
MySQL C API missing THR_KEY_mysys
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: mysql (Show other bugs)
17
All Linux
unspecified Severity high
: ---
: ---
Assigned To: Tom Lane
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-08 04:35 EDT by Boris Kolpackov
Modified: 2012-12-20 10:39 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 830757
Environment:
Last Closed: 2012-12-20 10:39:46 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Fix mysql C connector TLS memory leak (2.31 KB, patch)
2012-09-08 05:36 EDT, Stephane Carrez
no flags Details | Diff

  None (edit)
Description Boris Kolpackov 2012-08-08 04:35:54 EDT
+++ This bug was initially created as a clone of Bug #830757 +++

Description of problem:

Fedora's build of libmysqlclient hides the THR_KEY_mysys extern variable. This variable contains the thread-specific storage key used by MySQL. ODB (ORM for C++) uses this key to work around issues in automatic MySQL thread initialization/termination (more details below). As a result, ODB fails to compile with Fedora's version of the MySQL client library while working fine with all other distributions (Debian/Ubuntu, SUSELinux) as well as with the MySQL project-provided builds of the library.   

Version-Release number of selected component (if applicable):

MySQL 5.5

How reproducible:

Always.

Steps to Reproduce:

Try to link the following test program:

#include <pthread.h>
extern pthread_key_t THR_KEY_mysys;
int main () {return pthread_getspecific (THR_KEY_mysys) != 0;}

gcc test.c -lmysqlclient_r


Actual results:

/tmp/ccc4WJxN.o: In function `main':
test.c:(.text+0x6): undefined reference to `THR_KEY_mysys'


Expected results:

No errrors.


Additional info:

I realize that this hiding of undocumented symbols is intentional. However, there doesn't seem to be a way to implement automatic thread initialization/termination (mysql_thread_init()/mysql_thread_end() calls for each thread) without access to this key. Here is a more detailed description of what we are doing here:

MySQL requires that each thread that uses the MySQL API first call mysql_thread_init() and at the end call mysql_thread_end(). If the thread fails to call mysql_thread_end(), then MySQL will block the main thread at program termination and wait for this threads to call mysql_thread_end(). If that doesn't happen, then it prints an error message to STDERR. Not a very user-friendly behavior. 

In ODB we are using pthread thread-specific storage to implement automatic handling of this initialization/termination. The idea is basically to call mysql_thread_init() before the first call to the MySQL API and then use the TLS destructor to make sure mysql_thread_end() will get called before the thread exits. This "almost" works without THR_KEY_mysys. The "almost" part comes from this behavior of the Linux pthread implementation (this could also be standard-mandated, I am not sure). When a thread exist the TLS machinery goes over all the TLS slots and calls destructors for those that specified one. Apparently, for those that didn't specify the destructor, it sets the value to zero. So what happens in the case of MySQL is that pthread might set the MySQL TLS slot value to 0 before calling the ODB slot destructor. When ODB destructor calls mysql_thread_end(), MySQL examines its slot value, sees that it is 0,
and ignores the call, assuming this thread has already been terminated or was never initialized.

The only way that we could think of how to resolve this is to cache the MySQL's slot value and then restore it just before calling mysql_thread_end(). And for that we need access to THR_KEY_mysys.

I see that you already export some undocumented/internal functions that are used
by other software (e.g., PHP). I realize this is not ideal, but would it be possible to add THR_KEY_mysys (defined in mysys/my_thr_init.c) to this list?

--- Additional comment from tgl@redhat.com on 2012-06-11 09:02:05 EDT ---

If you can get upstream to agree that this should be documented as part of their API, I'll be happy to un-hide it.  But without that, relying on it seems like a great way to shoot yourself in the foot.  They have changed undocumented symbols before and doubtless will do so again.  This one is almost certainly not intended to be used by client code ...

--- Additional comment from endoflife@fedoraproject.org on 2012-08-07 14:58:27 EDT ---

This message is a notice that Fedora 15 is now at end of life. Fedora
has stopped maintaining and issuing updates for Fedora 15. It is
Fedora's policy to close all bug reports from releases that are no
longer maintained. At this time, all open bugs with a Fedora 'version'
of '15' have been closed as WONTFIX.

(Please note: Our normal process is to give advanced warning of this
occurring, but we forgot to do that. A thousand apologies.)

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, feel free to reopen
this bug and simply change the 'version' to a later Fedora version.

Bug Reporter: Thank you for reporting this issue and we are sorry that
we were unable to fix it before Fedora 15 reached end of life. If you
would still like to see this bug fixed and are able to reproduce it
against a later version of Fedora, you are encouraged to click on
"Clone This Bug" (top right of this page) and open it against that
version of Fedora.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

The process we are following is described here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping
Comment 1 Boris Kolpackov 2012-08-08 04:41:37 EDT
I doubt I will be able to convince Oracle to do this. And I agree that this is not something that should be used directly, in normal circumstances. However, the problem is that there is no other way to work around the issue I described above. So this boils down to the choice between "leaving things broken because there is no clean way to fix them" and "using an undocumented mechanism to make things work".
Comment 2 Tom Lane 2012-08-08 10:11:41 EDT
My opinion on this has not changed since F15: I'm not exposing that variable unless upstream agrees it's meant to be public.

I think that the situation you describe might well be considered a flaw in the mysql_thread_init/mysql_thread_end API, in that it doesn't play nicely with pthreads.  But I don't propose to invent a Fedora-specific solution for that, either.

You really need to raise this issue upstream, ie, file a bug at http://bugs.mysql.com asking for an API change or another suggestion of how to do it without relying on access to undocumented stuff.
Comment 3 Boris Kolpackov 2012-08-14 11:12:46 EDT
> But I don't propose to invent a Fedora-specific solution for that, either.

Well, in a sense this is really a problem invented by Fedora. All other builds of libmysqlclient (including the "official" ones supplied by MySQL) don't hide this symbol. So one could argue that it is part of the interface.

In any case, I've submitted the bug, though I wouldn't hold my breath:

http://bugs.mysql.com/bug.php?id=66379
Comment 4 James Cape 2012-08-31 07:38:03 EDT
@Tom,

This policy also breaks the Percona-PAM-plugin <URL:http://www.percona.com/doc/percona-pam-for-mysql/> which lets you create users in MySQL that get authenticated via PAM.

Is there some advantage to Fedora being more strict about the MySQL API than Oracle is?

@Boris

That MySQL bug is marked private, could you update?
Comment 5 Boris Kolpackov 2012-08-31 08:37:56 EDT
James, I don't see where I can change it to not be "private". In fact I don't see any indication that it is private.
Comment 6 James Cape 2012-08-31 08:40:56 EDT
Hrm, I just get "You do not have access to bug #66379," whether logged in or not.
Comment 7 Tom Lane 2012-08-31 10:08:36 EDT
Curious, I see the access-denied page too, though I'm pretty certain I was able to see that bug when Boris first filed it.
Comment 8 Tom Lane 2012-08-31 10:21:28 EDT
(In reply to comment #4)
> Is there some advantage to Fedora being more strict about the MySQL API than
> Oracle is?

Well, yes: it provides considerably more security that Fedora packages won't break in the face of future mysql updates.  In my other packaging duties, I have expended a great deal of time recently fixing packages that relied on undocumented or deprecated libpng and libtiff APIs, and thus broke in the recent major version updates for those libraries.  So I do not consider it at all hypothetical that this symbol is likely to change or go away in some future upstream update.

There is also the undeniable fact that when not filtered, libmysqlclient pollutes the application link namespace with hundreds of undocumented symbols, creating risks for breakages due to conflicts.
Comment 9 James Cape 2012-08-31 10:54:38 EDT
There's a couple things to disagree with here: Firstly, I think if a library puts a function in their headers, then it should show up in exports. Maybe it belongs in a libmysqlprivate.so, but that's upstream's decision to make.

Secondly, the risk of conflict breakage is something developers have to deal with anyways (because no other distro does this), so at best this delays the inevitable, at worst it just balkanizes Linux even further.

The way it's structured right now, if I write something on RHEL/CentOS, Ubuntu,  etc., that depends on something in mysql_com.h, I can compile it on Fedora but it won't actually load, and I can't fix it without distributing my own MySQL package, or copying/pasting the function actually in the library *and* the headers into my plugin.

And let's be honest here, no project lead that isn't getting paid to make it go on Fedora is going to do that. They're going to throw up their hands and say "Fedora is unsupported, but you can uninstall the Fedora mysql package and use one of the MySQL-server-break-all-the-deps packages online."

In the reverse, someone can write stuff on Fedora that compiles, links, and runs just fine, but unintentionally includes a conflict with a symbol that's not getting stripped on another platform. For them, they'll run into the same problem in reverse---it compiles but doesn't link/work/run w/o crashing because of a symbol conflict in !Fedora.

I guess I just don't see the advantage of doing this: people will have to fix those conflicts if they want their code to run on anything but Fedora (which they do), and people will have to expend extra effort to make stuff that works everywhere else work on Fedora (which, to be honest, they won't).
Comment 10 Tom Lane 2012-08-31 11:53:25 EDT
Meh.  I still think it's a seriously bad idea to be depending on THR_KEY_mysys, and rather than shooting the messenger you should be pressing upstream to provide a supported solution to this type of problem.  Otherwise, when (not if) they break it, what are you going to do?

However, seeing that we now have reports of two different applications relying on THR_KEY_mysys, I'll relent and include that symbol in the filter as of the next builds.

BTW, as for your argument that Fedora is unlike other distros in this respect: the long-term plan is to contribute the filter back to upstream.  I no longer have any expectation that Oracle would accept it, because they seem to have a pretty strong NIH complex, but MariaDB for instance well might.
Comment 11 Tom Lane 2012-08-31 12:08:14 EDT
(In reply to comment #9)
> There's a couple things to disagree with here: Firstly, I think if a library
> puts a function in their headers, then it should show up in exports.

BTW, so far as I can see, THR_KEY_mysys is not declared in any header.  Not just any exported header, but any header.  MySQL could mark it static in my_thr_init.c tomorrow, and not break any of their code nor any client application that conforms to any documented API.  So if you don't realize that you're standing on very thin ice here, I'd suggest that you wake up and smell the coffee.  But I'm tired of arguing about it, so will do as you ask.
Comment 12 James Cape 2012-08-31 13:53:41 EDT
I'm not concerned (particularly) about THR_KEY_mysys, I'm concerned about get_tty_password, in mysql_com.h (and more generally, everything in mysql_com.h).
Comment 13 Tom Lane 2012-08-31 15:06:56 EDT
Well, the mysql include files are pretty much of a mess; it's very hard to tell what they intend to be publicly visible and what not, so I've mainly gone on the basis of exposing only functions that are documented in the user-facing documentation.  In the case of mysql_com.h, there's stuff in there that is *clearly* server-side only, such as the UDF-related struct declarations --- so I think it's nuts to claim that the entire file represents guaranteed client-library API.  A lot of it doesn't appear to be meant to be client-facing at all; if it were, they'd have made more effort to use names including "my" or "mysql", don't you think?  A comparison of different major release series shows that they've not been terribly concerned with API stability of the functions declared there, either, again implying that they don't necessarily think these are exported for client use.
Comment 14 James Cape 2012-08-31 16:14:35 EDT
Doesn't that explicitly preclude anybody from ever using Fedora to write a server-side plugin for MySQL---which, according to their docs includes:

- storage engines
- authentication plugins (like the Percona-PAM-plugin, or an IPA plugin)
- auditing plugins
- information_schema plugins
- semi-synchronous replication

In terms of UDFs, those structs are:

"...primarily intended for UDF authors to debug UDFs and to verify what MySQL passes to the UDF." Which means you can't use Fedora to write something like this:

http://dev.mysql.com/doc/refman/5.5/en/ha-memcached-interfaces-mysqludf.html
Comment 15 Boris Kolpackov 2012-09-01 06:28:18 EDT
Tom, I am not really arguing with you. I understand your motivation with hiding the symbols and would agree with them *if* we were leaving in an ideal world. But consider my side of the story as well: 

1. MySQL has screwed up threading support.

2. Oracle doesn't think there is anything wrong.

3. We found an ugly, undocumented, dangerous, etc, etc, workaround. At this point our options basically are: (a) leave things broken and (b) use the workaround and keep our fingers crossed. In the ideal world we would choose (a) and say something idealistic like "By not providing support for MySQL, we will make our users put pressure on Oracle". But in the real world we choose (b) because otherwise nobody is going to use our stuff with MySQL.

4. Then comes Tom Lane and while meaning well, breaks our workaround.

If you read the bug report I submitted with Oracle, you will see that those people are just not reasonable (when I told the guy why can't they use pthread TLS destructors in libmysqlclient to fix their threading support, he replied that they cannot use C++ constructors/destructors because it is a C-only library; and that guy is one of the core developers).
Comment 16 Stephane Carrez 2012-09-08 05:34:49 EDT
Boris, I'm not able to look at your bug report as well and I fully agree with you.

I've fixed the MySQL connector in my environment so that it uses the
thread key cleanup operation which is a POSIX 1003.1c standard
(defined in... 1995!!!).

I've filed another bug report with a description and the patch.

See http://bugs.mysql.com/bug.php?id=66740

In case MySQL blocks my bug report, I've written a detailed description
with the patch on the following article:

http://blog.vacs.fr/index.php?post/2012/09/07/Fixing-the-mysql_thread_end-cleanup-bug-in-libmysql

Feel free to integrate it!
(I'm not using Fedora but Ubuntu; the patch applies to MySQL C connector 6.0.2).
Comment 17 Stephane Carrez 2012-09-08 05:36:57 EDT
Created attachment 610958 [details]
Fix mysql C connector TLS memory leak

The patch to fix the MySQL C connector and avoid memory leak due to my_thread_init not releasing the TLS data.
Comment 18 Tom Lane 2012-09-29 14:51:39 EDT
(In reply to comment #16)
> Boris, I'm not able to look at your bug report as well and I fully agree
> with you.
> 
> I've fixed the MySQL connector in my environment so that it uses the
> thread key cleanup operation which is a POSIX 1003.1c standard

Stephane, I think that's clearly a superior solution, but it doesn't fix Boris' problem until it's applied everywhere --- which pretty much means upstream has to accept it.  Also, I'm up against the freeze deadline for Fedora 18, and this patch seems a bit too risky to be pushing in at the last minute.  So what I'm going to do for the moment is go ahead and export THR_KEY_mysys.

BTW, the mysql 5.5.28 release notes contain an object lesson in the dangers of exporting everything in sight:

   * The libmysqlclient_r client library exported symbols from
     yaSSL that conflict with OpenSSL. If a program linked against
     that library and libcurl, it could crash with a segmentation
     fault. (Bug #14068244)

Filtering everything that's not specifically intended to be exported provides some protection against that sort of thing ...
Comment 19 Fedora Update System 2012-09-30 01:07:01 EDT
mysql-5.5.28-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mysql-5.5.28-1.fc17
Comment 20 Fedora Update System 2012-09-30 01:07:25 EDT
mysql-5.5.28-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/mysql-5.5.28-1.fc16
Comment 21 Fedora Update System 2012-09-30 01:07:46 EDT
mysql-5.5.28-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mysql-5.5.28-1.fc18
Comment 22 Fedora Update System 2012-09-30 14:23:04 EDT
Package mysql-5.5.28-1.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing mysql-5.5.28-1.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-15119/mysql-5.5.28-1.fc18
then log in and leave karma (feedback).
Comment 23 John Pilkington 2012-10-20 11:36:57 EDT
I'm using MythTV 0.25-fixes under Fedora 16 x86_64.  Since installing this package I have been seeing 'Threaded File Writer took a long time' warnings when recording TV and, usually, doing some other disk intensive operation; the quoted duration has been up to 6 seconds.  I believe this is new, and the package changelog pointed me here.  Attempts to revert to 5.5.7-1 using cached packages and my preferred package manager Smart send CPU to 100%, apparently for ever, until killed.  The initial upgrade to 5.5.8-1 had to be done after other packages in the same download but eventually seemed successful.

I posted this heads-up on the MythTV users list.  The one response seems unrelated to Fedora or mysql. 

http://www.gossamer-threads.com/lists/mythtv/users/531074#531074
Comment 24 Tom Lane 2012-10-20 11:53:36 EDT
(In reply to comment #23)
> I'm using MythTV 0.25-fixes under Fedora 16 x86_64.  Since installing this
> package I have been seeing 'Threaded File Writer took a long time' warnings
> when recording TV and, usually, doing some other disk intensive operation;
> the quoted duration has been up to 6 seconds.  I believe this is new, and
> the package changelog pointed me here.

I see no reason to think that that has anything to do with mysql, and it certainly isn't related to the problem discussed in this bug.

> Attempts to revert to 5.5.7-1 using
> cached packages and my preferred package manager Smart send CPU to 100%,
> apparently for ever, until killed.

Dunno what Smart is, but you might want to file a bug about that with them.

> I posted this heads-up on the MythTV users list.  The one response seems
> unrelated to Fedora or mysql. 
> http://www.gossamer-threads.com/lists/mythtv/users/531074#531074

The MythTV patch discussed in the linked threads seems like probably what you need.
Comment 25 John Pilkington 2012-10-20 13:06:15 EDT
Thanks for the quick response.
Comment 26 Fedora Update System 2012-12-20 10:39:51 EST
mysql-5.5.28-1.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.