Hi Mattias, I have created an issue on github:
https://github.com/oetiker/rrdtool-1.x/issues/504 cheers tobi Today Tobias Oetiker wrote: > Today Matthias Nagel wrote: > > > Hi, > > > care to provide a patch ? > > I will see what I can do after I got my own application (the one that > > uses rrdlib) running. > > thinking of it, I might actually have someone who might be > interested in doing it ... > > so check back before you do anything > > cheers > tobi > > > Matthias > > > > 2014-06-16 9:50 GMT+02:00 Tobias Oetiker <[email protected]>: > > > Hi Matthias, > > > > > > Today Matthias Nagel wrote: > > > > > >> Hi, > > >> > > >> > The setlocale() thread-safeness problems are well known, but there is > > >> > also a > > >> > rather significant lack of a decent widely adopted workaround. > > >> > > >> I know, but what I want to say is that according to this page [1] the > > >> rrd_update_r function is said to be thread-save but it isn't. Hence, > > >> if I had not stumbled across the first point (aka the memory bug) I > > >> would not have noticed this one. I would never have assumed that > > >> rrd_update_r uses setlocale. (Perhaps sometimes later in the future > > >> after I would have observed strange behaviour.) That's why I asked for > > >> a big warning at [1]. > > >> > > >> I do not know yet, if setlocale is the only problem with rrd_update_r, > > >> but at least I see a bug fix for this one. There is no need to use > > >> setlocale at all, if there was an interface like > > >> > > >> rrd_update_r( cont char* rrdFile, time_t ts, size_t argc, const char* > > >> desc[], const T* val[] ) > > >> > > >> for T being a numeric type. This way one could avoid the conversion to > > >> and from a string at all. The calling application is responsible of > > >> providing the correct datatype. That should even simplify rrd_update_r > > >> because there is no need to > > >> parse an argument of the pattern "<ts>:<val 1>:...:<val n>". > > > > > > since all we are doing with setlocale in rrdtool, is to get strtod to be > > > NOT locale sensitive. Another aproach would be to switch to a > > > nonlocale sensitive implementation of strtod and get rid of all the > > > setlocale calls. > > > > > > http://www.jbox.dk/sanos/source/lib/strtod.c.html > > > > > > care to provide a patch ? > > > > > > cheers > > > tobi > > > > > > > > >> Regards, Matthias > > >> > > >> [1] http://oss.oetiker.ch/rrdtool/prog/rrdthreads.en.html > > >> > > >> 2014-06-16 1:47 GMT+02:00 Donovan Baarda <[email protected]>: > > >> > The setlocale() thread-safeness problems are well known, but there is > > >> > also a > > >> > rather significant lack of a decent widely adopted workaround. This is > > >> > the > > >> > problem with many of the standard C libraries... they pre-date > > >> > threading, > > >> > and there was never consensus/standardizing on any thread safe > > >> > alternative. > > >> > > > >> > For any thread-safe alternative to work, everything needs to use it, > > >> > but > > >> > without a standard, you can pretty much guarantee that something you > > >> > link > > >> > will not use it, and some platforms won't even have it. > > >> > > > >> > So instead, your best option is often to just declare your code > > >> > non-thread-safe and recommend subprocesses instead... but then the > > >> > inter-process communication libs are not as well developed as the > > >> > threading > > >> > libs. > > >> > > > >> > Either that or completely embrace something like glib which replaces > > >> > nearly > > >> > everything in the standard C libs and only use other libs that also use > > >> > glib. When you do this you also have to accept that your code doesn't > > >> > look > > >> > like normal C any more, and you might have some platform restrictions. > > >> > > > >> > > > >> > > > >> > On 16 June 2014 08:39, Matthias Nagel <[email protected]> > > >> > wrote: > > >> >> > > >> >> Hello, > > >> >> thanks. In the meantime I patched my Debian packages locally. But I > > >> >> ran > > >> >> into another race condition. rrd_update_r() isn't thread-save, > > >> >> because the C > > >> >> locale is an application wide variable. Assume one has rrdlib (A) and > > >> >> some > > >> >> other library (B) and the execution order is as follows: > > >> >> > > >> >> (A1) old_locale = setlocale(...) > > >> >> (B1) old_locale = setlocale(...) > > >> >> (A2) // do some locale-dependent stuff > > >> >> (A3) setlocale( old_locale ) > > >> >> (B2) // do some locale-dependent stuff > > >> >> (B3) setlocale( old_locale ) > > >> >> > > >> >> Here, library B can be any library that also sets the global C locale > > >> >> within a different thread context. In the best case some strings are > > >> >> misinterpreted, in the worst case the memory gets corrupted :-( At the > > >> >> moment, I wrote a work-around by using an application wide mutex that > > >> >> must > > >> >> be locked by any thread that wants to call any library that might > > >> >> change the > > >> >> global C locale. But of course this isn't very nice. > > >> >> > > >> >> Are there any chances that the rrd_update_r function (and relatives) > > >> >> will > > >> >> be rewritten? For example, C++ locales are bounded to a specific > > >> >> stream and > > >> >> are not global. At least there should be big, BIG warning at > > >> >> http://oss.oetiker.ch/rrdtool/prog/rrdthreads.en.html that the C > > >> >> locale is > > >> >> subject to a race condition. > > >> >> > > >> >> Regards, Matthias > > >> >> > > >> >> Am Sonntag, 15. Juni 2014, 22:37:01 schrieb Tobias Oetiker: > > >> >> > Hi Matthias, > > >> >> > > > >> >> > yes you are right ... we fixed this in master, but not in the 1.4 > > >> >> > branch ... it is now ... > > >> >> > > > >> >> > cheers > > >> >> > tobi > > >> >> > > > >> >> > Today Matthias Nagel wrote: > > >> >> > > > >> >> > > Hello, > > >> >> > > > > >> >> > > I am writing a multi-threaded C++ application that uses rrdlib > > >> >> > > natively by calling rrd_update_r(). If I compile without > > >> >> > > optimazations and > > >> >> > > enable -ggdb everything seems to work fine. As soon as I switch > > >> >> > > to -O2 and > > >> >> > > disable -ggdb my apllication crashes at runtime. > > >> >> > > > > >> >> > > If it crashes the output is either > > >> >> > > > > >> >> > > *** glibc detected *** rrdtool: <something> > > >> >> > > > > >> >> > > or > > >> >> > > > > >> >> > > expected timestamp not found in data source from <input> > > >> >> > > > > >> >> > > but <input> is not the string that was given to rrd_update_r but > > >> >> > > unreadable garbage. Obviously, it is a memory corruption problem. > > >> >> > > Therefore, > > >> >> > > I ran the application under valgrind and I noticed that the > > >> >> > > problems comes > > >> >> > > from inside of the rrdlib. The message is > > >> >> > > > > >> >> > > > > >> >> > > ==11724== Invalid read of size 1 > > >> >> > > ==11724== at 0x4C2A051: __GI_strcmp (mc_replace_strmem.c:712) > > >> >> > > ==11724== by 0x5A4FF7F: setlocale (setlocale.c:210) > > >> >> > > ==11724== by 0x505D06B: _rrd_update (rrd_update.c:982) > > >> >> > > ==11724== Address 0x9deb0d0 is 0 bytes inside a block of size 12 > > >> >> > > free'd > > >> >> > > ==11724== at 0x4C27D4E: free (vg_replace_malloc.c:427) > > >> >> > > ==11724== by 0x5A4FCBD: setname (setlocale.c:173) > > >> >> > > ==11724== by 0x5A500B0: setlocale (setlocale.c:417) > > >> >> > > ==11724== by 0x505D02D: _rrd_update (rrd_update.c:974) > > >> >> > > > > >> >> > > Let's have a look at it: > > >> >> > > > > >> >> > > rrd_update.c:973: old_locale = setlocale(LC_NUMERIC, NULL); > > >> >> > > rrd_update.c:974: setlocale(LC_NUMERIC, "C"); > > >> >> > > rrd_update.c:982: setlocale(LC_NUMERIC, old_locale); > > >> >> > > > > >> >> > > The problem is obvious. The variable "old_locale" that is used at > > >> >> > > the > > >> >> > > 3rd line was assigned at the 1st line. But the 2nd call to > > >> >> > > "setlocale" freed > > >> >> > > the return value of the first call. According to the man pages > > >> >> > > the return > > >> >> > > value is a pointer to static memory and freed/allocated on every > > >> >> > > call. > > >> >> > > Actually the 2nd line (974) should be ommited and it should be > > >> >> > > > > >> >> > > rrd_update.c:973: old_locale = setlocale(LC_NUMERIC, "C" ); > > >> >> > > rrd_update.c:974: // deleted > > >> >> > > rrd_update.c:982: setlocale(LC_NUMERIC, old_locale); > > >> >> > > > > >> >> > > Why this double call to "setlocale" anyway? > > >> >> > > > > >> >> > > Best regards, Matthias > > >> >> > > > > >> >> > > > > >> >> > > > >> >> > > > >> >> > > >> >> -- > > >> >> Matthias Nagel > > >> >> Parkstraße 27 > > >> >> 76131 Karlsruhe > > >> >> > > >> >> Festnetz: +49-721-96869289 > > >> >> Mobil: +49-151-15998774 > > >> >> e-Mail: [email protected] > > >> >> ICQ: 499797758 > > >> >> Skype: nagmat84 > > >> >> > > >> >> _______________________________________________ > > >> >> rrd-users mailing list > > >> >> [email protected] > > >> >> https://lists.oetiker.ch/cgi-bin/listinfo/rrd-users > > >> >> > > >> > > > >> > > > >> > > > >> > -- > > >> > Donovan Baarda <[email protected]> > > >> > > >> > > >> > > >> > > > > > > -- > > > Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland > > > www.oetiker.ch [email protected] +41 62 775 9902 > > > > > > > > > > -- Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland www.oetiker.ch [email protected] +41 62 775 9902
_______________________________________________ rrd-users mailing list [email protected] https://lists.oetiker.ch/cgi-bin/listinfo/rrd-users
