text2pcap.c define trickery breaks strdup() handling with recent glibc

This issue was migrated from bug 3161 in our old bug tracker.

Original bug information:

Reporter: Mike Frysinger
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: x86
Version: Git

Mike Frysinger said:

Build Information:

glibc-2.9

linux-2.6.28

gcc-4.3.2

the text2pcap.c file tries to setup defines it shouldnt be testing:
#ifndef __USE_XOPEN
#  define __USE_XOPEN
#endif

POSIX states that you should only be _XOPEN_SOURCE:
http://www.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html

with recent glibc versions, defining just _XOPEN_SOURCE results in the prototype for strptime(), but the prototype for strdup() gets dropped.  thus the implicit prototype has an "int" return value which is 32bits even on a 64bit system.  you can see how this quickly gets bad.

this is because the value of _XOPEN_SOURCE has meaning and simply defining it says that it has a value of 0.  it should be defined to 600 or 700 in order for things to work properly.

--- text2pcap.c
+++ text2pcap.c
@@ -90,7 +90,7 @@
 #  define __USE_XOPEN
 #endif
 #ifndef _XOPEN_SOURCE
-#  define _XOPEN_SOURCE
+#  define _XOPEN_SOURCE 600
 #endif

 #include <ctype.h>

added uigtk versiondev labels

Jaap Keuter said:

Have a look at this thread: http://ethereal.cs.pu.edu.tw/lists/ethereal-dev/200201/msg00367.html

Mike Frysinger said:

i dont see how that thread has any bearing on the statements ive made ... it doesnt contradict anything ive said

Jaap Keuter said:

It shows the origins of these defines. Together with the SVN logs that should give a clear picture.

Mike Frysinger said:

the logs shows just people trying different defines and hoping things work. that's how things like __USE_XOPEN got in there. defining _XOPEN_SOURCE to nothing is wrong as it's too old and causes string prototypes to be omitted.

i dont have any opensolaris platform to test, but i'd think that they'd be doing the right thing as well and respect a properly defined _XOPEN_SOURCE value like the POSIX spec documents.

Jaap Keuter said:

That is the fun of cross platform applications, you have got to be sure that changes like this work on *all* platforms.

The relevant commits are in two groups: 4624, 4629, 4630 and 24044, 25426. The current define of _XOPEN_SOURCE came in with 24044 (from bug 2183). Together with the referenced thread I think we should be careful with Solaris / HP-UX etc.
I would suggest going with
 #ifndef _XOPEN_SOURCE
-#  define _XOPEN_SOURCE
+#  define _XOPEN_SOURCE 500
 #endif
first and see how much we can wean off further. (Note: using 500 here, not 600, as that is enough according to the strdup man page, without opening up the C99 can-o-worms). Also influenced are epan/ftypes/ftype-time.c and editcap.c.

Mike Frysinger said:

if you dont want to do it like the POSIX spec states because you want to support broken systems, then i would just start marking things with OS-specific ifdefs

#ifdef __solaris__
<do extensions or whatever>
#elif defined __hpux__
<do whatever crap>
#else
<do POSIX standard stuff -- i.e. only _XOPEN_SOURCE 500>
#endif

then there shouldnt be much of this "i'm too afraid to make any changes as i'm worried i'll break old crappy systems"

Jaap Keuter said:

I'm sorry we're living in a crappy systems world. This is a request for people having access to these 'crappy systems' to verify what can and can't be done, starting with changing "_XOPEN_SOURCE" to "_XOPEN_SOURCE 500" and go from there.

Mike Frysinger said:

which is why i suggested protecting things with OS-specific defines. needing confirmation from a variety of platforms which are not widely used is time consuming and unnecessarily holds back other people.

Petr Sumbera said:

*** Bug #3307 (closed) has been marked as a duplicate of this bug. ***

Jeff Morriss said:

Not sure if this is needed any more--we don't use strdup() any more (instead we use g_strdup()). Does anyone see any problems with the status quo here?

Petr Sumbera said:

Without the patch from comment #1 it still fails on Solaris (Wireshark 1.3.5):

source='text2pcap.c' object='text2pcap.o' libtool=no \
        DEPDIR=.deps depmode=none /bin/sh ./depcomp \
        /ws/onnv-tools/SUNWspro/SS12/bin/cc -DHAVE_CONFIG_H -I. -I. -I.  -I. -I./wiretap     -I/usr/include -I/usr/include/pcap -I/usr/include/kerberosv5 '-DPLUGIN_DIR="/usr/lib/wireshark/plugins/1.3.5"'  -DNEW_PACKET_LIST -D_U_="" -xO3 -m32 -xarch=sparc -xspace -W0,-Lt -Xa  -xildoff -xc99=all -W2,-xwrap_int -I/usr/include/pcre -I/usr/include -D_REENTRANT -D_POSIX_PTHREAD_SEMANTICS -D_PTHREADS -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12   -I/usr/include/pcap -I/usr/include/kerberosv5 -c text2pcap.c
"/usr/include/sys/feature_tests.h", line 353: #error: "Compiler or options invalid for pre-UNIX 03 X/Open applications  and pre-2001 POSIX applications"
cc: acomp failed for text2pcap.c
gmake[2]: *** [text2pcap.o] Error 2

Jeff Morriss said:

(In reply to comment #‍11)
> Without the patch from comment #‍1 it still fails on Solaris (Wireshark 1.3.5):

I assume you meant comment #‍0; is this Nevada or Open Solaris? (Solaris 10 works fine for me.)

Does define _XOPEN_SOURCE 500 also work?

Petr Sumbera said:

(In reply to comment #12)  
> (In reply to comment #11)  
> > Without the patch from comment #1 it still fails on Solaris (Wireshark 1.3.5):  
>  
> I assume you meant comment #0; is this Nevada or Open Solaris?  (Solaris 10  
> works fine for me.)  

Opps. Correct. I meant comment #0.

Yes, I'm building on Nevada/OpenSolaris.

The compiler is: cc: Sun C 5.9 SunOS_sparc Patch 124867-11 2009/04/30

And it seems that gcc is just fine. At least when compiling problematic module with:
gcc (GCC) 3.4.3 (csl-sol210-3_4-20050802).

> Does define _XOPEN_SOURCE 500 also work?  

No it doesn't..

Also when compiling the module with Sun Studio without "-xc99=all" it went fine..

Jeff Morriss said:

(In reply to comment #13)  
> Also when compiling the module with Sun Studio without "-xc99=all" it went  
> fine..  

How did that get there?  My understanding is Wireshark is targeted at C89 and we specifically should not be using any C99 features so does it really make sense to use this flag?

This would mean this problem should also show up on Solaris 10 if C99 is turned on since its feature_requests.h has the same test in it:

/*
 * It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 application
 * using c99.  The same is true for POSIX.1-1990, POSIX.2-1992, POSIX.1b,
 * and POSIX.1c applications. Likewise, it is invalid to compile an XPG6
 * or a POSIX.1-2001 application with anything other than a c99 or later
 * compiler.  Therefore, we force an error in both cases.
 */
#if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) && !defined(_XPG6))
#error "Compiler or options invalid for pre-UNIX 03 X/Open applications \
        and pre-2001 POSIX applications"
#elif !defined(_STDC_C99) && \
        (defined(__XOPEN_OR_POSIX) && defined(_XPG6))
#error "Compiler or options invalid; UNIX 03 and POSIX.1-2001 applications \
        require the use of c99"
#endif

Sure enough: adding "-std=c99" to gcc causes the same failure.

Petr Sumbera said:

(In reply to comment #14)  
> (In reply to comment #13)  
> > Also when compiling the module with Sun Studio without "-xc99=all" it went  
> > fine..  
>  
> How did that get there?  My understanding is Wireshark is targeted at C89 and  
> we specifically should not be using any C99 features so does it really make  
> sense to use this flag?  

Well, we try to build FOSS applications with this option by default in Solaris (sfw gate).
But I can now confirm that without this option Wireshark 1.3.5 will build fine.
(I have vague feeling that in past some Wireshark module required C99 mode)

So in general Wireshark has no problem with C99 but this one.. It's little bit pity. But if Wiresahrk is really targeted for C89 we can maybe close this bug then..

Gerald Combs said:

(In reply to comment #‍15)
> So in general Wireshark has no problem with C99 but this one.. It's little bit
> pity. But if Wiresahrk is really targeted for C89 we can maybe close this bug
> then..

As README.developer says,

"In general, don't use C99 features since some C compilers used to compile Wireshark don't support C99 (E.G. Microsoft C)."

We still support Visual C++ 6.0, which predates C99.

Petr Sumbera said:

(In reply to comment #16)  
> As README.developer says,  
>  
> "In general, don't use C99 features since some C compilers used to compile  
> Wireshark don't support C99 (E.G. Microsoft C)."  

Yes. But usually C89 code can be compiled with C99 mode enabled (meaning that both compilers can be used). Of course I'm not suggesting using C99 features here :-)

Jeff Morriss said:

Going from _XOPEN_SOURCE (=0) to _XOPEN_SOURCE=600 would be moving us from XPG3 (1989) to XPG6 (2001).

The question really is will this break compilation on some other (probably older) OS? Solaris and Linux don't seem to mind, but I have no clue about others... Might another OS decide that it doesn't implement level 600 and bail?

Guy Harris said:

Windows shouldn't matter here - the #define trickery:

/*
 * Just make sure we include the prototype for strptime as well
 * (needed for glibc 2.2) but make sure we do this only if not
 * yet defined.
 */
#ifndef __USE_XOPEN
#  define __USE_XOPEN
#endif
#ifndef _XOPEN_SOURCE
#  define _XOPEN_SOURCE
#endif

/*
 * Defining _XOPEN_SOURCE is needed on some platforms, e.g. platforms
 * using glibc, to expand the set of things system header files define.
 *
 * Unfortunately, on other platforms, such as some versions of Solaris
 * (including Solaris 10), it *reduces* that set as well, causing
 * strptime() not to be declared, presumably because the version of the
 * X/Open spec that _XOPEN_SOURCE implies doesn't include strptime() and
 * blah blah blah namespace pollution blah blah blah.
 *
 * So we define __EXTENSIONS__ so that "strptime()" is declared.
 */
#ifndef __EXTENSIONS__
#  define __EXTENSIONS__
#endif

is there to attempt to get a "give me the whole damn namespace" build option for UN*Xes - Windows' Win32 environment doesn't claim to be POSIX-compliant so the only standards namespace issues are "strict C89 namespace" vs. "we don't protect the namespace".

Wireshark is, I think, neither "a POSIX-conforming application" nor "an XSI-conforming application", as it requires interfaces other than those published by POSIX or XSI (or the Single UNIX Specification).  It's our job to avoid using stuff put into the name space by any of the extensions we use.

The way you request "give me the whole damn namespace" might be platform-dependent; it appears you define _DARWIN_C_SOURCE in OS X, but I'm not sure how you do it in Linux+glibc, *BSD for various values of *, Solaris, HP-UX, AIX, etc.

Jeff Morriss said:

OK, I changed it to set _XOPEN_SOURCE to 600 in rev 39501. We've got a while before the next release to find out if breaks other systems...

Jeff Morriss said:

(In reply to comment #20)  
> OK, I changed it to set _XOPEN_SOURCE to 600 in rev 39501.  We've got a while  
> before the next release to find out if breaks other systems...  

Unfortunately Solaris 10's feature_tests.h doesn't like this when NOT compiling in C99 mode (I'm using gcc): it wants _XOPEN_SOURCE to be something *less* than 600.

So in r41182 I changed it to simply not define _XOPEN_SOURCE on Solaris at all.  It remains set to 600 on other OS's.

closed

mentioned in issue #3307 (closed)