Make warnings treated as errors for dispatch.c and protect.c

[matthiasblaesing]

This follows an advise given by David Keller, to replace a unittest
to ensure correct version of strerror_r is used.

Quote:

I'm afraid the strerror(EBADF) can also produce Bad file number
depending on the libc (e.g. Android libc vs Glibc).

If I were you, I'd remove the strerror_r() unit test as the new code
will trigger a warning if the wrong strerror_r() is picked (invalid
conversion from int to char*: warning: initialization makes pointer
from integer without a cast) and treat warnings as errors with -Werror.

[matthiasblaesing]

@DavidKeller could you please have a look at this?

The basic idea is, that I did not want to touch the build of libffi, but only affect the build of the JNA specific parts. I had to fix/suppress some warnings:

• unused function parameters in dispatch.c needed to be marked as such
• warnings about unknown warning options need to be suppressed, as the MacOS X build fails, as the clobbered warning was not known
• warnings for clobbered variables are ignored. My interpretation is, that GCC sees potential clobbering, but if I interpreted the protect semantics correctly, the problematic setjmp/longjmp combination is only invoked on the error path, that does not make use of the problematic variables and raises an exception
• warning C4996 needs to be suppressed for MSVC (strdup is marked as deprecated)

Do the changes look sane to you?

[DavidKeller]

Hello @matthiasblaesing,

• unused function parameters in dispatch.c needed to be marked as such

Sure.

• warnings about unknown warning options need to be suppressed, as the MacOS X build fails, as the clobbered warning was not known

Your change seems fine.
If anyway you want to be very cautious, you may want to remove all warning silencers -Wno-* and rework the code.

• warnings for clobbered variables are ignored. My interpretation is, that GCC sees potential clobbering, but if I interpreted the protect semantics correctly, the problematic setjmp/longjmp combination is only invoked on the error path, that does not make use of the problematic variables and raises an exception

Replacing the MEMCPY/SET macros with functions ensure not automatic variable is clobbered :-) (at the expense of a stack frame setup).

static inline void
MEMCPY(JNIEnv * env, void * dest, const void * src, size_t n)
{
  PSTART();
  memcpy(dest, src, n);
  PEND(env);
}

static inline void
MEMSET(JNIEnv * env, void * s, int c, size_t n)
{
  PSTART();
  memset(s, c, n);
  PEND(env);
}

But the original code (and my proposal) seems undefined behavior to me:

• There is only one jmp_buf (protect.h:123) shared by all threads using JNA, which is UB.

If, in a multithreaded program, a longjmp() call employs an env buffer that was initialized by a call to setjmp() in a different thread, the behavior is undefined

Thread local variables will help but at what cost ?

• warning C4996 needs to be suppressed for MSVC (strdup is marked as deprecated)

As a general rule, I believe it's better to correct warnings instead of silencing it when possible :-) Here you can work around using a dedicated function with a non-reserved C standard name and use it everywhere.

static inline char *
STRDUP(char * src)
{
#ifdef _MSC_VER
    return _strdup(src);
#else
    return strdup(src);
#endif
}

Regards

[twall]

"Protected mode" (setjmp/longjmp) was only intended to be used in a single
threaded development environment (at least for *nix variants). No attempt
was ever made to make it robust for production use, since it requires use
of libjsig to function properly.

On Mon, Oct 3, 2016 at 11:33 AM, David Keller [email protected]
wrote:

Hello @matthiasblaesing https://github.com/matthiasblaesing,

• unused function parameters in dispatch.c needed to be marked as such

Sure.

• warnings about unknown warning options need to be suppressed, as the
  MacOS X build fails, as the clobbered warning was not known

Your change seems fine.
If anyway you want to be very cautious, you may want to remove all warning
silencers -Wno-* and rework the code.

• warnings for clobbered variables are ignored. My interpretation is,
  that GCC sees potential clobbering, but if I interpreted the protect
  semantics correctly, the problematic setjmp/longjmp combination is only
  invoked on the error path, that does not make use of the problematic
  variables and raises an exception

Replacing the MEMCPY/SET macros with functions ensure not automatic
variable is clobbered :-) (at the expense of a stack frame setup).

static inline void
MEMCPY(JNIEnv * env, void * dest, const void * src, size_t n)
{
PSTART();
memcpy(dest, src, n);
PEND(env);
}

static inline void
MEMSET(JNIEnv * env, void * s, int c, size_t n)
{
PSTART();
memset(s, c, n);
PEND(env);
}

But the original code (and my proposal) seems undefined behavior to me:

• There is only one jmp_buf (protect.h:123) shared by all threads
  using JNA, which is UB
  http://man7.org/linux/man-pages/man3/setjmp.3.html.

If, in a multithreaded program, a longjmp() call employs an env buffer
that was initialized by a call to setjmp() in a different thread, the
behavior is undefined

Thread local variables will help but at what cost ?

• warning C4996 needs to be suppressed for MSVC (strdup is marked as
  deprecated)

As a general rule, I believe it's better to correct warnings instead of
silencing it when possible :-) Here you can work around using a dedicated
function with a non-reserved C standard name
https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
and use it everywhere.

static inline char *
STRDUP(char * src)
{
#ifdef _MSC_VER
return _strdup(src);
#else
return strdup(src);
#endif
}

Regards

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#712 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrTuKyJfAXooVAdr9H0r74zfPXhf3ks5qwSA9gaJpZM4KL19P
.

[DavidKeller]

As the protected mode is optional, is the cost implied by thread local jmp_buf (on *nix) acceptable for you ?
This would allow debugging on multi-threading programs.

[twall]

Nominally that'd be ok, but I think when I originally did this there was
some uncertainty around the interaction between signal handlers and threads
(I wouldn't want to just assume that the thread handling the signal was the
one I needed to restore). Using a single, global buffer was a lazy way to
remove the uncertainty.

On Tue, Oct 4, 2016 at 12:07 PM, David Keller [email protected]
wrote:

As the protected mode is optional, is the cost implied by thread local
jmp_buf (on *nix) acceptable for you ?
This would allow debugging on multi-threading programs.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#712 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrU2uBWyoGJn3dRFlYw6mo02Rxrchks5qwnmxgaJpZM4KL19P
.

[DavidKeller]

On Linux :

A signal may be generated (and thus pending) for a process as a whole (e.g., when sent using kill(2)) or for a specific thread (e.g., certain signals, such as SIGSEGV and SIGFPE, generated as a consequence of executing a specific machine-language instruction are thread directed, as are signals targeted at a specific thread using pthread_kill(3))

On FreeBSD:

Whether the signal is directed at the process in general or at a specific thread depends on how it is generated.

As you said, there is no cross-platform guarantee (i.e. POSIX) on threads, hence I agree that using thread local wouldn't provide any real guarantee. I suspect this is a grey area where there is no standard way to recover from SIGSEV. Too bad.

[matthiasblaesing]

Thank you David and Timothy for the feedback and discussion. From the raised pointers:

• silencing the clobber warning: I added some more words to the Makefile why I think it is appropriate. There is only one codepath, that makes use of the problematic longjmp construct and this does not use the variable, but raises an exception.
• deprecated strdup in MSVC: yes both options (suppressing the warning and working around it) were on my mind with equal preference. So I took a slightly different route, that uses the basis that was already there there was already a define referring to _strdup)

I pushed a new revision with the described changes. This stage is tested as follows:

• Windows SDK 7.1 32bin/64bit: Compiles clean + No test regressions
• Visual Studio 2015: 32bit/64bit: Compiles clean
• OpenBSD 64bit: Compiles clean
• Linux 64bit (Ubuntu): Compiles clean + No test regressions
• Android Toolchain: Compiles clean