Include files parsed incorrectly?

[mouse07410]

Mac OS X 10.11.6, Xcode-8.0, Macports-installed OpenSSL-1.0.2j, current master OpenSC.

File src/tools/pkcs15-tool.c is missing <string.h> include, and include file <stdio.h> is not included correctly - as evidenced in the compiler output:

. . . . .
  CC       pkcs15-tool.o
pkcs15-tool.c:346:2: warning: implicitly declaring library function 'snprintf' with type 'int (char
      *, unsigned long, const char *, ...)' [-Wimplicit-function-declaration]
        snprintf(title, sizeof(title), "%s (%lu bytes): ", kind, (unsigned long) data_len);
        ^
pkcs15-tool.c:346:2: note: include the header <stdio.h> or explicitly provide a declaration for
      'snprintf'
pkcs15-tool.c:1150:17: warning: implicitly declaring library function 'strdup' with type
      'char *(const char *)' [-Wimplicit-function-declaration]
                return (u8 *) strdup(pincode);
                              ^
pkcs15-tool.c:1150:17: note: include the header <string.h> or explicitly provide a declaration for
      'strdup'
. . . . .

This patch (placing both includes before "config.h" and XOPEN_SOURCE definition) cures these warnings:

$ git diff upstream/master -- src/tools/pkcs15-tool.c
diff --git a/src/tools/pkcs15-tool.c b/src/tools/pkcs15-tool.c
index 7eeff44..08cbdd3 100644
--- a/src/tools/pkcs15-tool.c
+++ b/src/tools/pkcs15-tool.c
@@ -18,22 +18,26 @@
  * License along with this library; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+
+#include <stdio.h>
+#include <string.h>
+
 #include "config.h"

 #define _XOPEN_SOURCE 500
 #include <assert.h>
 #include <ctype.h>
+
 #ifdef _WIN32
 #ifdef __MINGW32__
 // work around for https://sourceforge.net/p/mingw-w64/bugs/476/
 #include <windows.h>
-#endif
+#endif /* __MINGW32__ */
 #include <shellapi.h>
 #include <tchar.h>
 #else
 #include <ftw.h>
-#endif
-#include <stdio.h>
+#endif /* _WIN32 */

[frankmorgner]

man 3 snprintf:

Feature Test Macro Requirements for glibc (see feature_test_macros(7)):

       snprintf(), vsnprintf():
           _BSD_SOURCE || _XOPEN_SOURCE >= 500 || _ISOC99_SOURCE || _POSIX_C_SOURCE >= 200112L;
           or cc -std=c99

Try:

#define _XOPEN_SOURCE 500
#include <stdio.h>

[mouse07410]

What's the point of trying something that was there before, and did not work (aka caused compiler warnings)? (Since on my machines WIN32 is never defined, the effective code was exactly as you suggested - and it was causing those compiler warnings that report of mis-compilation.) And it doesn't address the lack of <string.h> include.

Also, when the manuals contradict the observed behavior, I tend to follow the behavior and avoid problems - IMHO better to be with the working code than to be right with broken code. ;-)

[dengert]

What are you suggesting? That we make a change for you that may cause problems for others?

I would ask why is pkcs11-tool.c the only place #define _XOPEN_SOURCE 500 is used in OpenSC?

#include <config.h> should always be first. has #define HAVE_STRING_H which should be used to test if string.h should be included.

[mouse07410]

What are you suggesting?

I'm suggesting silencing compiler warnings that could lead to mis-compiled code, potentially maybe even exploitable, who knows.

That we make a change for you that may cause problems for others?

Should I matter less than somebody else - one of those mysterious "others"?

And yes, you should make a change that improves compile-ability of the code, and reduces issues with it (such as compiler assuming a wrong return type for a function). Even if this change is "for me".

#include <config.h> should always be first.

OK, makes sense.

#define HAVE_STRING_H which should be used to test if string.h should be included.

OK, makes sense.

I would ask why is pkcs15-tool.c the only place #define _XOPEN_SOURCE 500 is used in OpenSC?

I would ask why it's there in the first place, because it appears to screw the includes on the current compilers.

Oh, and this is what the relevant part of stdio.h looks on Darwin (this file does not have _XOPEN_SOURCE anywhere):

#if __DARWIN_C_LEVEL >= 200112L || defined(_C99_SOURCE) || defined(__cplusplus)
__BEGIN_DECLS
int      snprintf(char * __restrict __str, size_t __size, const char * __restrict __format, ...) __printflike(3, 4);
int      vfscanf(FILE * __restrict __stream, const char * __restrict __format, va_list) __scanflike(2, 0);
int      vscanf(const char * __restrict __format, va_list) __scanflike(1, 0);
int      vsnprintf(char * __restrict __str, size_t __size, const char * __restrict __format, va_list) __printflike(3, 0);
int      vsscanf(const char * __restrict __str, const char * __restrict __format, va_list) __scanflike(2, 0);
__END_DECLS
#endif /* __DARWIN_C_LEVEL >= 200112L || defined(_C99_SOURCE) || defined(__cplusplus) */

Clang compiler on Mac OS (both clang-3.9 from Macports, and clang from Xcode-8.0.0) reports

__DARWIN_C_LEVEL defined: 900000

And this is the proposed change that accommodates your recommendations (and silences compiler warnings):

diff --git a/src/tools/pkcs15-tool.c b/src/tools/pkcs15-tool.c
index 17c3fad..9607218 100644
--- a/src/tools/pkcs15-tool.c
+++ b/src/tools/pkcs15-tool.c
@@ -18,22 +18,28 @@
  * License along with this library; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+
 #include "config.h"

+#include <stdio.h>
+#if defined(HAVE_STRING_H)
+#include <string.h>
+#endif /* HAVE_STRING_H */
+
 #define _XOPEN_SOURCE 500
 #include <assert.h>
 #include <ctype.h>
+
 #ifdef _WIN32
 #ifdef __MINGW32__
 // work around for https://sourceforge.net/p/mingw-w64/bugs/476/
 #include <windows.h>
-#endif
+#endif /* __MINGW32__ */
 #include <shellapi.h>
 #include <tchar.h>
 #else
 #include <ftw.h>
-#endif
-#include <stdio.h>
+#endif /* _WIN32 */

 #ifdef ENABLE_OPENSSL
 #if defined(HAVE_INTTYPES_H)

[dengert]

Using git blame shows #define _XOPEN_SOURCE 500 (and all the if defs you want to change ) were introduced in 44694a0 on 10/7/2016 by @frankmorgner in #873

You and Frank should work this out and if #define _XOPEN_SOURCE 500 is needed on some systems, get it covered by an #if def.

[mouse07410]

I'd say that my (proposed) patch is correct. It only lacks #ifdef ... guard around _XOPEN_SOURCE ....

Since @frankmorgner added that #define _XOPEN_SOURCE 500, he probably knows what system(s) that he uses required it, and can put the #if defined(?) guard around that #define ... statement. I don't really have a say there (except ensuring that MacOS is excluded) because none of my systems needs that define.

[dengert]

looks like the nftw routine requires #define _XOPEN_SOURCE 500 as shown in an example:
https://linux.die.net/man/3/nftw

There are warning about using nftw.

Others have found that Mac OS requires 600 and other packages try not to use it if at all possible:
http:https://stackoverflow.com/questions/6330002/how-to-resolve-mac-os-xopen-source-error-in-xcode-while-compiling

http:https://stackoverflow.com/questions/5378778/what-does-d-xopen-source-do-mean

https://github.com/utahta/pythonbrew/blob/master/pythonbrew/patches/macosx/python25/patch-configure-no-posix-c-source.diff

https://bugs.python.org/issue17120

https://caml.inria.fr/mantis/view.php?id=7256

https://gitlab.labs.nic.cz/kslany/libisds/commit/9343ebfc1447952a6c0c601f99147ef969cdddeb

The last one above defines a test in configure.ac to see if it should be 500 or 600.

@mouse07410 can you try #define _XOPEN_SOURCE 600 as the only change?
If it works can you put an #ifdef something to set it to 600 on Mac OS and 500 for every one else?

[dengert]

I was commenting on your first patch. The second looks better, but still not correct. It may work in your environment.

The #define _XOPEN_SOURCE 500 (or 600) should be before everything accept config.h. On some configurations config.h could contain the version of _XOPEN_SOURCE to be used.

On Ubuntu 16.4, stdio.h includes features.h a 394 line file that just looks at all these XOPEN, ISO, ANSI and POSIX options. features.h is include by many other header files.

Your second patch would include stdio.h which the includes feature.h that sets _FEATURE_H so feature.h in only run once to test all the options. Placingit before #define _XOPEN_SOURCE 500 in effect negates setting _XOPEN_SOURCE becuse it was not set when features.h was run the first time for stdio.h.

I would suspect that string.h would then get included if the above was fixed.
If not, include with the #if defined(HAVE_STRING_H) It should still be after _XOPEN_SOURCE line.

Also look at the Mac OS man page for nftw and see if has some special requirements. nftw on linux required the #define _XOPEN_SOURCE 500 See if says it requires 600, which is what others are saying Mac OS requires.

@frankmorgner This problem was caused by your commit. How would you fix it?

[mouse07410]

@dengert, I see your point - but the references you mentioned are pretty old (he first one is from 2011). As far as I know, MacOS does not need _XOPEN_SOURCE now. There's no mentioning of XOPEN_SOURCE anywhere there.

Also, it looks like the OpenSC build process somehow suppresses compiler-defined flags. I know for a fact that all the compilers on MacOS (both from Xcode, and Macports-installed) define __DARWIN_C_LEVEL, which on Xcode-8.0.0 is 900000L.

This is the beginning of src/tools/pkcs15-tool.c:

. . . . .
#include "config.h"

#if !defined(__DARWIN_C_LEVEL)
#define _XOPEN_SOURCE 500
#endif /* __APPLE__ current MacOS */

#include <stdio.h>
#if defined(HAVE_STRING_H)
#include <string.h>
#endif /* HAVE_STRING_H */
. . . . .

And this is the compilation output, proving that the compiler does not have __DARWIN_C_LEVEL set when it compiles pkcs15-tool.c (is there a way to force the build process reveal what compiler flags are actually used? That "CC whatever" is not very helpful!).

. . . . .
  CC       pkcs15-tool.o
pkcs15-tool.c:355:2: warning: implicitly declaring library function 'snprintf' with type 'int (char *,
      unsigned long, const char *, ...)' [-Wimplicit-function-declaration]
        snprintf(title, sizeof(title), "%s (%lu bytes): ", kind, (unsigned long) data_len);
        ^
pkcs15-tool.c:355:2: note: include the header <stdio.h> or explicitly provide a declaration for 'snprintf'
. . . . .

When I comment out that define:

. . . . .
#include "config.h"

//#if !defined(__DARWIN_C_LEVEL)
//#define _XOPEN_SOURCE 500
//#endif /* __APPLE__ current MacOS */

#include <stdio.h>
#if defined(HAVE_STRING_H)
#include <string.h>
#endif /* HAVE_STRING_H */
. . . . .

the compilation does not produce any warning:

. . . . .
Making all in tools
  CC       opensc-tool.o
  CC       util.o
  CC       opensc-explorer.o
  CC       pkcs15-tool.o
  CC       pkcs15-crypt.o
  CC       pkcs11-tool.o
  CC       cardos-tool.o
  CC       eidenv.o
  CC       openpgp-tool.o
  CC       iasecc-tool.o
  CC       cryptoflex-tool.o
  CC       pkcs15-init.o
  CC       netkey-tool.o
. . . . .

I wrote a small test-program that contains

. . . . .
#if defined(__DARWIN_C_LEVEL)
        printf("__DARWIN_C_LEVEL defined: %ld\n", __DARWIN_C_LEVEL);
#else
        printf("__DARWIN_C_LEVEL not defined\n");
#endif
. . . . .

Here's the output:

$ clang -o flags flags.c
$ ./flags
__DARWIN_C_LEVEL defined: 900000
__APPLE__ defined
__APPLE_CC__ defined
. . . . .
__clang__ defined, __clang_version__="8.0.0 (clang-800.0.38)"
. . . . .

Perhaps it would be better if @frankmorgner could remove XCODE_SOURCE altogether (by the way, defining it to 600 did not seem to help so far).

[mouse07410]

Update

Here's the define that seems to work (though I'd still prefer not to have XCODE_SOURCE there in the first place, and I'd like to be able to have the actual compiler flags displayed rather than hidden):

diff --git a/src/tools/pkcs15-tool.c b/src/tools/pkcs15-tool.c
index 17c3fad..059a30b 100644
--- a/src/tools/pkcs15-tool.c
+++ b/src/tools/pkcs15-tool.c
@@ -18,22 +18,31 @@
  * License along with this library; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+
 #include "config.h"

+#if !defined(__APPLE__)
 #define _XOPEN_SOURCE 500
+#endif /* __APPLE__ current MacOS */
+
+#include <stdio.h>
+#if defined(HAVE_STRING_H)
+#include <string.h>
+#endif /* HAVE_STRING_H */
+
 #include <assert.h>
 #include <ctype.h>
+
 #ifdef _WIN32
 #ifdef __MINGW32__
 // work around for https://sourceforge.net/p/mingw-w64/bugs/476/
 #include <windows.h>
-#endif
+#endif /* __MINGW32__ */
 #include <shellapi.h>
 #include <tchar.h>
 #else
 #include <ftw.h>
-#endif
-#include <stdio.h>
+#endif /* _WIN32 */

 #ifdef ENABLE_OPENSSL
 #if defined(HAVE_INTTYPES_H)
@@ -44,10 +53,11 @@
 typedef unsigned __int32 uint32_t;
 #else
 #warning no uint32_t type available, please contact [email protected]
-#endif
+#endif /* HAVE_INTTYPES_H */
 #include <openssl/bn.h>
 #include <openssl/crypto.h>
-#endif
+#endif /* ENABLE_OPENSSL */
+
 #include <limits.h>

 #include "libopensc/pkcs15.h"

(As you can see, I like to have it explicit what endif belongs to what ifdef. It also helps debugging when somebody screws up with nested ifdefs, which occasionally happens.)

And here's the output:

. . . . .
Making all in tools
  CC       opensc-tool.o
  CC       util.o
  CC       opensc-explorer.o
  CC       pkcs15-tool.o
  CC       pkcs15-crypt.o
  CC       pkcs11-tool.o
  CC       cardos-tool.o
. . . . .

[dengert]

./configure --disable-silent-rules will cause the old full command line to be listed.

See:
http:https://nadeausoftware.com/articles/2011/12/c_c_tip_how_list_compiler_predefined_macros

gcc -dM -E -x c /dev/null
Find the defines from the compiler. On 64 bit Ubuntu 16.4:
STDC, GNUC, linux and unix are a few of the 248 defines from the compiler.

To see what the preprocessor does I have a very old script file make.cpp:
#!/bin/csh -x
mv $1 $1.save
make CPPFLAGS="-E" CXX="$CC -v -E" CC="$CC -v -E" $1 |& fold >& /tmp/$1.tmp
make CXX="$CC -v -E" CC="$CC -v -E" $1 |& fold >& /tmp/$1.tmp
touch $1
cat /tmp/$1.tmp $1 | sed -e 's/^[ \t]*$//' | uniq > $1.CPRE
rm /tmp/$1.tmp
mv $1.save $1

The -E option tells the compile to save the output of the preprocessor in the object file:
This is run in the build directory to build one object file:
export CC=gcc (set for your compiler.)
make.cpp pkcs15-tool.o
view pkcs15-tool.o.CPRE

[mouse07410]

Thank you!

Here are the flags by both compilers:
clang-flags.txt
gcc-flags.txt

It is interesting that __DARWIN_C_LEVEL does not show for either - perhaps it only gets set when the input is an actual C/C++ file (of course this does not explain why __DARWIN_C_LEVEL did not get set when compiler was invoked by OpenSC build for pkcs15-tool.c file, which clearly is a C file, but it matters less now that we have a workaround).

I noticed another define that might be useful: __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED.

Anyways, thanks - your script is very useful. And I wasn't aware of --disable-silent-rules - also extremely useful option.

Update
I was correct: giving a C file as an input to your command, like gcc -dM -E -x c tzzz.c (where tzzz.c file exists) gives

. . . . .
#define __DARWIN_C_ANSI 010000L
#define __DARWIN_C_FULL 900000L
#define __DARWIN_C_LEVEL __DARWIN_C_FULL
. . . . .

So must be some problem with the build that somehow suppresses these flags. Again, it doesn't matter all that much now.

[dengert]

Sounds like __DARWIN_C_LEVEL is not a compiler define, but set
  is a system header file:

http:https://stackoverflow.com/questions/32803754/what-is-the-darwin-c-level-c-preprocessor-symbol

On 10/23/2016 9:00 PM, Mouse wrote:


  Thank you!
  Here are the flags by both compilers:
    clang-flags.txt

    gcc-flags.txt
  It is interesting that __DARWIN_C_LEVEL does not
    show for either - perhaps it only gets set when the input is an
    actual C/C++ file (of course this does not explain why __DARWIN_C_LEVEL
    did not get set when compiler was invoked by OpenSC build for pkcs15-tool.c
    file, which clearly is a C file, but it matters less
    now that we have a workaround).
  I noticed another define that might be useful: __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED.
  Anyways, thanks - your script is very useful. And I wasn't
    aware of --disable-silent-rules - also extremely
    useful option.
  —
    You are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.







  {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSC/OpenSC","title":"OpenSC/OpenSC","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSC/OpenSC"}},"updates":{"snippets":[{"icon":"PERSON","message":"@mouse07410 in #886: Thank you!\r\n\r\nHere are the flags by both compilers:\r\n[clang-flags.txt](https://github.com/OpenSC/OpenSC/files/546805/clang-flags.txt) \r\n[gcc-flags.txt](https://github.com/OpenSC/OpenSC/files/546808/gcc-flags.txt)\r\n\r\nIt is interesting that `__DARWIN_C_LEVEL` does not show for either - perhaps it only gets set when the input is an actual C/C++ file (of course this does not explain why `__DARWIN_C_LEVEL` did not get set when compiler was invoked by OpenSC build for `pkcs15-tool.c` file, which clearly *is* a C file, but it matters less now that we have a workaround).\r\n\r\nI noticed another define that might be useful: `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED`.\r\n\r\nAnyways, thanks - your script is very useful. And I wasn't aware of `--disable-silent-rules` - also extremely useful option.\r\n\r\n"}],"action":{"name":"View Issue","url":"https://github.com/OpenSC/OpenSC/issues/886#issuecomment-255632362"}}}


-- 

Douglas E. Engert [email protected]

[mouse07410]

@frankmorgner @dengert what's the status of this issue? If the proposed patch is acceptable - when are you planning to merge it? If it's not acceptable - what's the problem with it?

[frankmorgner]

It's unfortunate that Apple doesn't use the correct API definitions. strdup is defined for X/Open 5, incorporating POSIX 1995, see https://stackoverflow.com/questions/5378778/what-does-d-xopen-source-do-mean for reference.

Since everyone is using newer versions by default, I suggest to simply remove #define _XOPEN_SOURCE 500 to resolve this discussion.

[dengert]

The above sounds reasonable.
But does "everyone is using newer versions by default" include Windows, mingw, and msys?
Are there any other environments that do not?
Is nftw available in these other environments?

[mouse07410]

1. I'm OK with simply removing _XOPEN_SOURCE. But it still leaves open the question of the order of the include files. I suggest using the order that my (proposed) patch shows.
2. The original pkcs15-tool.c does not include <string.h>. I think this should be remedied (like my patch does) by adding <string.h> if _HAVE_STRING_H is set.
3. In my experience it's easier to deal with nested ifdefs if endif for each is appropriately marked with comments. I recommend accepting that part of the patch.

[mouse07410]

Thank you!

What about adding <string.h>?

[mouse07410]

@frankmorgner let me ask again: please add

#if defined(HAVE_STRING_H)
#include <string.h>
#endif /* HAVE_STRING_H */

Also, Apple does not need _XOPEN_SOURCE=600.

[mouse07410]

@frankmorgner how much time do you need to figure that it's a good idea to include a .h file that defines a function used in the code?

"Apple stupidity", indeed.