Created attachment 96402 [details] [review]
proposed patch ][ (missing strerror_s)

My mingw32 installation is also missing strerror_s() function, thus I updated the compat.c accordingly.

I'm also notified about missing asprintf/vasprintf functions (implicit declarations) by the gcc compiler, but that doesn't cause a build break (I'll see later how it'll work in a runtime and report back if there will be any issue/patch for the p11-kit).

Where is it documented that MSVC strerror() is thread-safe? Could you post the link so I can add it to the patch?

(In reply to comment #0)
> Created attachment 96361 [details] [review] [review]
> proposed patch
> 
> There is no p11-kit-proxy.so when building p11-kit on windows (under
> mingw32), thus move the install-exec-hook into !OS_WIN32 branch, as is done
> in the attached patch. (I do not know why I cannot change the attachment's
> type to patch.)

Thanks. I've committed and merged this patch.

> There is alo a change in AM_INIT_AUTOMAKE(), a removal of the "serial-tests"
> argument, which is not available under mingw32, but I do not know the actual
> impact of this argument removal, thus feel free to ignore it (so I didn't
> not add it within the actual patch file).

It probably just affects tests. Upstream we've started to require automake 1.12 so we can get the testing behavior we need. But if you wish to carry a patch removing automake 1.10, that's fine.

(In reply to comment #2)
> Where is it documented that MSVC strerror() is thread-safe? Could you post
> the link so I can add it to the patch?

It is not thread safe [1]. It is just a lame replacement for a missing function during mingw32 compilation.

[1] http://msdn.microsoft.com/en-us/library/zc53h9bh.aspx

(In reply to comment #4)
> (In reply to comment #2)
> > Where is it documented that MSVC strerror() is thread-safe? Could you post
> > the link so I can add it to the patch?
> 
> It is not thread safe [1]. It is just a lame replacement for a missing
> function during mingw32 compilation.
> 
> [1] http://msdn.microsoft.com/en-us/library/zc53h9bh.aspx

Hmmm, the whole point of using our own strerror() is so that we can be thread safe. Could you use strerror_s() in your patch instead?

I would use it if the mingw32 has it available, but it doesn't. Maybe some later version of it will, I do not know, but the version I used ~3 months ago did not have it.

An alternative would be to use strerror() on WIN32 and add a critical section or mutex locking around it.

Hello,
I am interested in this issue too, so pardon me jumping in...

Regarding thread safety of Microsoft's strerror - its source is available with VC++ installation, in [VS install dir]\VC\crt\src\strerror.c. 
Hope it is OK to refer to it here, for this purpose.

As can be seen from that source, it uses a static per-thread (or thread-local) buffer. This should be thread-safe, in the meaning that every thread gets its own instance of this static buffer. If a thread were preempted in some unusual way (like APC, coroutine, fiber...) this static buffer may be overwritten. But IMHO this is unlikely for a "normal" Windows application, so no additional sync is needed.

Please correct me if my observation is wrong or does not apply to MinGW threads.

Obviously strerror_s is better, as it does not use static buffer at all, but using it breaks compatibility with WinXP.

We could check for availability of strerror_s in runtime (using GetProcAddress) but this seems to me too much for a small compatibility fixup, a proper implementation of strerror_r should be done in the MinGW project.

Regards,
Pavel A.

(In reply to comment #8)
> Hello,
> I am interested in this issue too, so pardon me jumping in...
> 
> Regarding thread safety of Microsoft's strerror - its source is available
> with VC++ installation, in [VS install dir]\VC\crt\src\strerror.c. 
> Hope it is OK to refer to it here, for this purpose.

Is this the same strerror() that MinGW links to? If not, then we could use strerror() in MSVC cases, but not MinGW.

Stef, Milan,

My understanding (maybe wrong) is that MinGW runtime library can bind to one of Microsoft DLLs: the msvcrt.dll or a "versioned" mcvcrtNN from some VC++ version.

So it looks that strerror() for MinGW app _always_ links to either of MSVCRT libraries, and it is exactly same strerror as for VC++ apps.

If developer of application which uses p11 chooses to use a VC++ runtime, the whole issue won't exist, as strerror_s would be available.

But this does not address the original issue: support WinXP with the default MSVCRT linkage.

Regarding thread safety of MinGW, a search indeed turns over many issues - mostly specific to C++, and IIRC none of these are recent.
Anyway, if a MinGW app suffers from threading problems, strerror is probably the least of them.

Is there a way to add config option to use VC++ runtime for Windows,
or to specify a minimal target Windows version (as MinGW recommends)?
The p11 library users could select the best variant for them, and the compatibility workaround can also test this condition, and use strerror_s or... ?


Regards,
Pavel

Below is a "thread safe" implementation that behaves like strerror_s, but it is also much heavier...

-- PavelA

#include <string.h>
#include <errno.h>
#include <stdlib.h>

int __cdecl __MINGW_NOTHROW
strerror_r(int errnum, char *buf, size_t sizebuf);

int __cdecl __MINGW_NOTHROW
strerror_r(int errnum, char *buf, size_t sizebuf)
{
    int n = sys_nerr;
    const char *p;
    if (errnum < 0 || errnum >= n)
        p = sys_errlist[n];
    else
        p = sys_errlist[errnum];
    if (buf == NULL || sizebuf == 0)
        return EINVAL;
    strncpy(buf, p, sizebuf);
    buf[sizebuf-1] = 0;
    return 0;
}

(In reply to comment #11)
> Below is a "thread safe" implementation that behaves like strerror_s, but it
> is also much heavier...

This looks great. I'm assuming it builds on Windows? Could someone provide it in patch form, and test it for the build cases reported in this bug? Doing it this way would certainly remove all doubt about thread-safety in implementations of strerror() on Windows.

(In reply to comment #12)

Stef,
I don't have my original build environment unfortunately, this was written and tested with mingw 3.20 on Windows XP.

Regards,
P.

Created attachment 102036 [details] [review]
Fix for implementation of strerror_r for WinXP (when streror_s is missing in msvcrt.dll)

When target Windows version has strerror_s
(based on _WIN32_WINNT macro in compat.h)
implement strerror_r via existing strerror_s;
else use own code, similar to strerror_s.

Thanks. Pushed to git master.

Thank you a lot for your help, Stef and Milan.

--p.

Pardon for continuing this long thread. Just stumbled upon the resolution of the strerror_s issue in the other MinGW project, mingw-w64:
File mingw-w64-crt/secapi/strerror_s.c

http://sourceforge.net/p/mingw-w64/mingw-w64/ci/d710b9af4038a2c2ff7871cd6181a24a6d5c7e37/

In their CRT library, strerror_s() is always available.
They detect presence of strerror_s in runtime with GetProcAddress; if not present (on WinXP or earlier) call their own implementation - which uses... the not-thread-safe strerror(). Ahem.

-- pa