Forum OpenACS Development: localtime_s

Request notifications

Collapse
Posted by Maurizio Martignano on

Dear all,

please notice that in the file reentrant.c the following lines

    int errNum;

    NS_NONNULL_ASSERT(clock != NULL);

    errNum = localtime_s(&tlsPtr->ltbuf, clock);
    if (errNum != 0) {
        NsThreadFatal("ns_localtime", "localtime_s", errNum);
    }

when compiled with MS VC 2019 x64, do compile but break at run time, with errno = 22, while the following lines

    Tls *tlsPtr = GetTls();
    errno_t errNum;

    NS_NONNULL_ASSERT(clock != NULL);
 
    errNum = _localtime32_s(&tlsPtr->ltbuf, clock);
    if (errNum != 0) {
        NsThreadFatal("ns_localtime", "_localtime32_s", errNum);
    }

though generating a warning at compile time, do not break at run time.

Hope it helps, Maurizio

Collapse
2: Re: localtime_s (response to 1)
Posted by Gustaf Neumann on
Maurizio,

many thanks for reporting. I wonder, why this did not show up earlier, since the usage of localtime_s with time_t was there at least since 5 years.

I've addressed this issue via the change [1]. Please double check.

-g
PS: the NaviServer developer list is a better place to post this.
[1] https://bitbucket.org/naviserver/naviserver/commits/ce82fd694aaac0dfd316ac658f4481f27f3d92c4

Collapse
3: Re: localtime_s (response to 2)
Posted by Maurizio Martignano on
Dear Gustaf,
thank you for fixing the code!

Trying to answer your question, I believe that yes the problem was there, hidden in the code, but that it actually got exposed only recently (24th of January) when in the file winthread.c the following two lines

wait.usec /= 1000;
if (wait.sec < 0 || (wait.sec == 0 && wait.usec <= 0)) {

got changed into

msec = Ns_TimeToMilliseconds(&wait);
if (msec < 0) {

So the function Ns_TimeToMilliseconds got called and the problem exposed.

Again thank you,
Maurizio

Collapse
4: Re: localtime_s (response to 3)
Posted by Gustaf Neumann on
Thanks, this explains it.
Collapse
5: Re: localtime_s (response to 2)
Posted by Maurizio Martignano on
Dear Gustaf,
first of all sorry if I continue this discussion where I started it. Next similar issue will be published on Naviserver developer list. Anyhow I had to change your fix into this:
errNum = _localtime32_s(&tlsPtr->ltbuf, clock);
/*
if (sizeof(clock) == 4) {
errNum = _localtime32_s(&tlsPtr->ltbuf, clock);
} else {
errNum = _localtime64_s(&tlsPtr->ltbuf, clock);
}
*/
Why? Because the parameter const time_t *clock comes/goes from/to the "sec" field of variables of type Ns_Time, that is:
typedef struct Ns_Time {
long sec;
long usec;
} Ns_Time;
So, basically, we are mixing up "long" with "time_t". But these two types do not have the same size in Windows 64, where, we have:
sizeof(long) = 4.
sizeof(time_t) = 8.
sizeof(__time32_t) = 4.
while on Linux 64, we have:
sizeof(long) = 8.
sizeof(time_t) = 8.
I hope this clarifies the issue,
Maurizio
Collapse
6: Re: localtime_s (response to 1)
Posted by Gustaf Neumann on
Maurizio, the diagnosis is right, but the cure is not.

ns_localtime() is supposed to be interface compatible with Linux's localtime(3), and receives therefore as argument (const time_t *timep); the function was called in all but one place with the proper type in NaviServer, in one case (in the file sched.c) the long value of Ns_Time.sec was passed incorrectly to it (the compiler should have complained about this).

The updated version in the repository should be fine now with win32 and win64.

Collapse
7: Re: localtime_s (response to 6)
Posted by Maurizio Martignano on
Thank you Gustaf,
once the bug in sched.c is removed, you can come back to the original implementation of ns_localtime, leaving the code cleaner:
struct tm *
ns_localtime(const time_t *timep)
{

Tls *tlsPtr = GetTls();
int errNum;

NS_NONNULL_ASSERT(timep != NULL);

errNum = localtime_s(&tlsPtr->ltbuf, timep);
if (errNum != 0) {
NsThreadFatal("ns_localtime", "localtime_s", errNum);
}

return &tlsPtr->ltbuf;
}
Again thank you,
Maurizio
Collapse
8: Re: localtime_s (response to 7)
Posted by Gustaf Neumann on
actually no, since the Microsoft API needs a different call when time_p is 32 or 64 bit.
Collapse
9: Re: localtime_s (response to 8)
Posted by Maurizio Martignano on
Well... actually no!
The compiler inserts automatically the proper version of the function to be called. This is definition found in the actual Microsoft time.h file used in 64 bit:
_Check_return_wat_
static __inline errno_t __CRTDECL localtime_s(
_Out_ struct tm* const _Tm,
_In_ time_t const* const _Time
)
{
return _localtime64_s(_Tm, _Time);
}
So there's no need to check which of the two versions are to be used. I wrote the previous remark after having first checked the proper functioning of the system with the fix I proposed.

Anyhow it is your call.

Maurizio

Collapse
11: Re: localtime_s (response to 9)
Posted by Gustaf Neumann on
ok, if you say so, i will adjust this. It is anyhow strange that the API offers two different calls.
Collapse
10: Re: localtime_s (response to 9)
Posted by Maurizio Martignano on
Sorry, I will try to explain better my point.

The following is from Microsoft docs:
"localtime_s is an inline function which evaluates to _localtime64_s, and time_t is equivalent to __time64_t. If you need to force the compiler to interpret time_t as the old 32-bit time_t, you can define _USE_32BIT_TIME_T. Doing this will cause localtime_s to evaluate to _localtime32_s. This is not recommended because your application may fail after January 18, 2038, and it is not allowed on 64-bit platforms."

In other words, it doesn't matter if you compile with the 32 bit or 64 bit compiler: if you do not specify that define, time_t is equivalent to __time64_t and localtime_s to _localtime64_s; on the contrary if you specify that define, something you can do only with the 32 bit compiler, time_t is equivalent to __time_32_s and localtime_s is equivalent to _localtime32_s.

Naviserver build system, especially the file "Makefile.win32" already takes properly care of this define (_USE_32BIT_TIME_T).

So nothing needs to be done in the "reentrant.c" code.