supposed reentrant versions of library functions such as localtime_r() are not really thread safe

[ashkang]

localtime_r() updates the internal static jtm object first, then moves it's data into result structure. This is wrong, since multiple instances of re-entrant functions might do this resulting in a corrupted output. Re-entrant versions of library function calls must update results passed to them first, then update the static jtm object.

[ashkang]

This proposed patch to fix re-entrant versions of libjalali function calls breaks POSIX backward compatibility of these family of functions as they in fact must touch libjalali's internal static struct as well. Constructing user supplied data from static version is wrong here. What we're doing right now goes like this: First we update our internal static struct, then we copy our internal static struct's data to user supplied source verbatim which is wrong. It must be the other way around.

[reith]

On Sun, 01 Jun 2014 08:00:48 -0700 Ashkan notifications@github.com wrote:

First we update our internal static struct, then we copy our internal static struct's data to user supplied source verbatim which is wrong. It must be the other way around.

I had seen this in glibc but still don't know whether this behaviour is documented by POSIX standards or is just an implementation method choosen by implementors?

[ashkang]

I had seen this in glibc but still don't know whether this behaviour is documented by POSIX standards or is just an implementation method choosen by implementors?

Unfortunately, I do not have a copy of POSIX.1-2001 at the moment to explicitly check for that behaviour. However, Linux man-pages version 3.63 for localtime_r (available here) clearly states that their implementation is conforming to POSIX.1-2001. The rest is well documented. Moreover, I reckon it's safe to assume that the logical choice here for a function that by signature definitions is bound to return a pointer to a struct is to return a meaningful data. That data of course in a multi-threaded application is unreliable and possibly incorrect. But a single-threaded application which uses eg. localtime_r() can still get the correct result in this legacy mode. We must avoid breaking this convention (or any other legacy conventions for that matter) unless it's absolutely necessary.

[reith]

well, this approach fail when someone incorrectly call localtime_r and tries to get result from internal structure. Other approach, still is not safe when used incorrectly. For example if someone called localtime in his single-threaded context, then makes some threads and in those threads calls localtime_r, internal structure would contain invalid data.

[ashkang]

For example if someone called localtime in his single-threaded context, then makes some threads and in those threads calls localtime_r, internal structure would contain invalid data.

The use of localtime() and internal data struct are obsolete and must be avoided. As soon as the concept of threads and synchronization jumps in, it's results should not be trusted for good reasons. Still, we're trying to be legacy-approach-friendly here. There are no reasons as to why a single-threaded application using localtime_r() shouldn't work properly.

[reith]

On Wed, 04 Jun 2014 03:44:54 -0700 Ashkan notifications@github.com wrote:

The use of localtime() and internal data struct is obsolete and must be avoided.

This is actually what proposed patch does. Here re-entrant functions never touch internal data structures. Apparently you want them to fill internal strcutre too, right?

This proposed patch to fix re-entrant versions of libjalali function calls breaks POSIX backward compatibility of these family of functions as they in fact must touch libjalali's internal static struct as well.

I'm consfused.

There are no reasons as to why a single-threaded application using localtime_r() shouldn't work properly.

It'll still work properly. In this use case localtime_r would supplied by a pointer to a structure, fill it with result and return that pointer again and this is very expected IMO. This works if invoked in single-threaded or multi-threaded context. It fails just when someone makes a structure for result of a function, pass it to function and get it back filled with correct data but he feels he should get result from an internal structure, Not the structure he made it himself for his task.

So if your concern is this weird scenario, simply copying result to internal structure would do the trick, since it is in single-thread context, thread locking is not needed. If it called in multi-threaded context.. well relying on internal structure would be stupid enough then. I pushed a commit doing this.

[ashkang]

It'll still work properly. In this use case localtime_r would supplied by a pointer to a structure, fill it with result and return that pointer again and this is very expected IMO.

You're right and this code snippet actually shows why. I was under the impression that the return value and supplied structure in case of localtime_r() are actually two different objects. Hence you could actually have something like this: struct tm* p = localtime_r(&t, NULL);. This turned out to be wrong. Your assumption is correct since the return value as a pointer to struct tm is actually nothing more than the struct we just supplied to our localtime_r() call earlier. That again ironically, makes your second patch incorrect. Your previous patch is correct as is.

#include <time.h>
#include <stdio.h>

int main()
{
    time_t now = time(NULL);
    struct tm t;
    struct tm* p;
    p = localtime_r(&now, NULL);
    printf("(internal): %d:%d:%d\n", p->tm_hour, p->tm_min, p->tm_sec);
    printf("(supplied): %d:%d:%d\n", t.tm_hour, t.tm_min, t.tm_sec);
    printf("supplied: %p, internal: %p\n", &t, p);
    return 0;
}