[rrd-users] Valgrind error rrd_update.c:982

Mon Jun 16 09:54:40 CEST 2014

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.
Matthias

2014-06-16 9:50 GMT+02:00 Tobias Oetiker <tobi at oetiker.ch>:
> 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 <abo at minkirri.apana.org.au>:
>> > 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 <matthias.h.nagel at gmail.com> 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: matthias.h.nagel at gmail.com
>> >> ICQ: 499797758
>> >> Skype: nagmat84
>> >>
>> >> _______________________________________________
>> >> rrd-users mailing list
>> >> rrd-users at lists.oetiker.ch
>> >> https://lists.oetiker.ch/cgi-bin/listinfo/rrd-users
>> >>
>> >
>> >
>> >
>> > --
>> > Donovan Baarda <abo at minkirri.apana.org.au>
>>
>>
>>
>>
>
> --
> Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
> www.oetiker.ch tobi at oetiker.ch +41 62 775 9902


-- 
Matthias Nagel
Parkstraße 27
76131 Karlsruhe

Festnetz: +49-721-96869289
Mobil: +49-151-15998774
e-Mail: matthias.h.nagel at gmail.com
ICQ: 499797758
Skype: nagmat84