Bugs item #1733119, was opened at 2007-06-07 15:43
Message generated for change (Comment added) made by rrauenza
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1733119&group_id=976

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: libcurl
Group: wrong behaviour
Status: Open
Resolution: Fixed
Priority: 6
Private: No
Submitted By: rrauenza (rrauenza)
Assigned to: Daniel Stenberg (bagder)
Summary: socket.c assumes xopen getsockopt/getsockname

Initial Comment:

HPUX doesn't use the XOPEN getsockopt/getsockname interfaces by default (for backwards compatibility).

     int setsockopt(
          int s,
          int level,
          int optname,
          const void *optval,
          int optlen
      );

    _XOPEN_SOURCE_EXTENDED Only (UNIX 98)
      int getsockopt(
          int s,
          int level,
          int optname,
          void *optval,
          socklen_t *optlen
      );

When compiling 64bit, socklen_t is 64bit, but int is 32bit, so optlen ends up being 0 due the socklen being treated as a 32bit value.

Here's the required changes -- simply changing the parms to be ints instead of socklens when not using _XOPEN_SOURCE_EXTENDED and HPUX

$ rcsdiff connect.c
RCS file: connect.c,v
retrieving revision 1.1
rdiff -r1.1 connect.c
367a368,370
> #if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
> int size;
> #else
368a372
> #endif
407a412,414
> #if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
> int errSize = sizeof(err);
> #else
408a416
> #endif

----------------------------------------------------------------------

>Comment By: rrauenza (rrauenza)
Date: 2007-06-12 17:07

Message:
Logged In: YES
user_id=1309815
Originator: YES

I grabbed the CVS, but couldn't get autoconf to work.. I'll just grab
tomorrow's CVS auto-tarball.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2007-06-12 14:41

Message:
Logged In: YES
user_id=1110
Originator: NO

Many thanks, I just committed a slightly edited version of this patch and
I would be grateful if you could verify that it builds fine for you. I
added a check for inline to the configure script, so it should be fine to
use with 'inline'.

In my commit message I also speculate on your real name, and if you can
just confirm that I'll make sure to give you proper credit for your work!

----------------------------------------------------------------------

Comment By: rrauenza (rrauenza)
Date: 2007-06-12 13:49

Message:
Logged In: YES
user_id=1309815
Originator: YES

This works. I think this is probably the best option. Only problem is I'm
not sure it works with gcc. __inline should be the gcc equivalent, or just
static.

vega7077-root-># /usr/local/bin/diff -u curl-7.16.2/lib/setup_once.h
curl-7.16.2.fix3/lib/setup_once.h
--- curl-7.16.2/lib/setup_once.h 2007-04-11 06:10:22 -0700
+++ curl-7.16.2.fix3/lib/setup_once.h 2007-06-12 13:42:26 -0700
@@ -343,6 +343,83 @@
 #define EREMOTE WSAEREMOTE
 #endif
 
+#if defined (__LP64__) && defined(__hpux) && !
defined(_XOPEN_SOURCE_EXTENDED)
+#include <sys/socket.h>
+/* HP-UX has this oddity where it features a few functions that don't
work
+ with socklen_t so we need to convert to ints
+
+ This is due to socklen_t being a 64bit int under 64bit ABI, but the
+ pre-xopen (default) interfaces require an int, which is 32bits.
+
+ Therefore, Anytime socklen_t is passed by pointer, the libc function
+ truncates the 64bit socklen_t value by treating it as a 32bit value.

+
+ Note that some socket calls are allowed to have a NULL pointer for
+ the socklen arg.
+*/
+
+__inline int Curl_hp_getsockname(int s, struct sockaddr *name, socklen_t
*namelen)
+{
+ int rc;
+ if(namelen) {
+ int len = *namelen;
+ rc = getsockname(s, name, &len);
+ *namelen = len;
+ } else {
+ rc = getsockname(s, name, 0);
+ }
+ return rc;
+}
+
+__inline int Curl_hp_getsockopt(int s, int level, int optname, void
*optval,
+ socklen_t *optlen)
+{
+ int rc;
+ if(optlen) {
+ int len = *optlen;
+ rc = getsockopt(s, level, optname, optval, &len);
+ *optlen = len;
+ } else {
+ rc = getsockopt(s, level, optname, optval, 0);
+ }
+ return rc;
+}
+
+__inline int Curl_hp_accept(int sockfd, struct sockaddr *addr, socklen_t
*addrlen)
+{
+ int rc;
+ if(addrlen) {
+ int len = *addrlen;
+ rc = accept(sockfd, addr, &len);
+ *addrlen = len;
+ } else {
+ rc = accept(sockfd, addr, 0);
+ }
+ return rc;
+}
+
+
+__inline ssize_t Curl_hp_recvfrom(int s, void *buf, size_t len, int
flags,
+ struct sockaddr *from, socklen_t *fromlen) {
+ ssize_t rc;
+ if(fromlen) {
+ int fromlen32 = *fromlen;
+ rc = recvfrom(s, buf, len, flags, from, &fromlen32);
+ *fromlen = fromlen32;
+ } else {
+ rc = recvfrom(s, buf, len, flags, from, 0);
+ }
+ return rc;
+
+}
+
+#define getsockname(a,b,c) Curl_hp_getsockname((a),(b),(c))
+#define getsockopt(a,b,c,d,e) Curl_hp_getsockopt((a),(b),(c),(d),(e))
+#define accept(a,b,c) Curl_hp_accept((a),(b),(c))
+#define recvfrom(a,b,c,d,e,f) Curl_hp_recvfrom((a),(b),(c),(d),(e),(f))
+
+#endif
+
 
 #endif /* __SETUP_ONCE_H */

Did a make test...

test 527...OK (327 out of 347, remaining: 00:08)
test 528...OK (328 out of 347, remaining: 00:08)
test 529...OK (329 out of 347, remaining: 00:08)
test 530...OK (330 out of 347, remaining: 00:07)
test 531...OK (331 out of 347, remaining: 00:07)
test 532...OK (332 out of 347, remaining: 00:07)
test 533...OK (333 out of 347, remaining: 00:06)
test 534...OK (334 out of 347, remaining: 00:06)
test 535...OK (335 out of 347, remaining: 00:05)
test 537...OK (336 out of 347, remaining: 00:05)
test 538...OK (337 out of 347, remaining: 00:04)
sh: 10610 Terminated
sh: 10355 Terminated
sh: 11389 Terminated
TESTDONE: 310 tests out of 310 reported OK: 100%
TESTDONE: 347 tests were considered during 170 seconds.
TESTINFO: 37 tests were skipped due to these restraints:
TESTINFO: "no stunnel" 14 times (300, 301, 302, 303, 304, 305, 306, 307,
308, 400, 401, 403, 404, 509)
TESTINFO: "Resolving 'ip6-localhost' didn't work" 1 times (241)
TESTINFO: "curl lacks libz support" 5 times (220, 221, 222, 223, 224)
TESTINFO: "curl lacks scp support" 4 times (601, 603, 605, 607)
TESTINFO: "curl lacks idn support" 1 times (165)
TESTINFO: "curl lacks netrc_debug support" 6 times (130, 131, 132, 133,
134, 257)
TESTINFO: "curl lacks sftp support" 6 times (600, 602, 604, 606, 608, 609)

----------------------------------------------------------------------

Comment By: rrauenza (rrauenza)
Date: 2007-06-12 13:20

Message:
Logged In: YES
user_id=1309815
Originator: YES

I had to make a couple of modifications. But even with these, make test
doesn't work -- because the implementation doesn't get pulled into the
other a.out's?

make test
No suffix list.
Making all in data
No suffix list.
Making all in server
        /bin/sh ../../libtool --tag=CC --mode=link cc +DD64 -o
getpart testpart.o getpart.o strequal.o base64.o mprintf.o memdebug.o
timeval.o
cc +DD64 -o getpart testpart.o getpart.o strequal.o base64.o mprintf.o
memdebug.o timeval.o
        source='resolve.c' object='resolve.o' libtool=no \
        DEPDIR=.deps depmode=hp2 /bin/sh ../../depcomp \
        cc -DHAVE_CONFIG_H -I. -I../../lib -I../../src -I../../include
-I../../lib -I../../lib +DD64 -c resolve.c
        /bin/sh ../../libtool --tag=CC --mode=link cc +DD64 -o
resolve resolve.o util.o getpart.o strequal.o base64.o mprintf.o
memdebug.o timeval.o
cc +DD64 -o resolve resolve.o util.o getpart.o strequal.o base64.o
mprintf.o memdebug.o timeval.o
        source='tftpd.c' object='tftpd.o' libtool=no \
        DEPDIR=.deps depmode=hp2 /bin/sh ../../depcomp \
        cc -DHAVE_CONFIG_H -I. -I../../lib -I../../src -I../../include
-I../../lib -I../../lib +DD64 -c tftpd.c
        /bin/sh ../../libtool --tag=CC --mode=link cc +DD64 -o
tftpd tftpd.o util.o getpart.o strequal.o base64.o mprintf.o memdebug.o
timeval.o
cc +DD64 -o tftpd tftpd.o util.o getpart.o strequal.o base64.o mprintf.o
memdebug.o timeval.o
ld: Unsatisfied symbol "Curl_hp_recvfrom" in file tftpd.o
1 errors.
*** Error exit code 1

Stop.
*** Error exit code 1

Stop.
*** Error exit code 1

Stop.

Would you want to try making the implementation inline? HP's C compiler
supports it, and I think gcc does as well. (Or they could be made static)

Rich

$ /usr/local/bin/diff -u curl-7.16.2/lib/connect.c
curl-7.16.2.fix2/lib/connect.c
--- curl-7.16.2/lib/connect.c 2007-03-27 14:14:07 -0700
+++ curl-7.16.2.fix2/lib/connect.c 2007-06-12 13:07:17 -0700
@@ -884,3 +884,57 @@
 
   return CURLE_OK;
 }
+
+#if defined(__LP64__) && defined(__hpux) && !
defined(_XOPEN_SOURCE_EXTENDED)
+/* HP-UX has this oddity where it features a few functions that don't
work
+ with socklen_t so we need to convert to ints */
+
+#undef getsockname
+#undef getsockopt
+#undef accept
+#undef recvfrom
+
+int Curl_hp_getsockname(int s, struct sockaddr *name, socklen_t
*namelen)
+{
+ int rc;
+ int len = *namelen;
+ rc = getsockname(s, name, &len);
+
+ // always copy? if(!rc)
+ *namelen = len;
+ return rc;
+}
+int Curl_hp_getsockopt(int s, int level, int optname, void *optval,
+ socklen_t *optlen)
+{
+ int rc;
+ int len = *optlen;
+ rc = getsockopt(s, level, optname, optval, &len);
+ // always copy? if(!rc)
+ *optlen = len;
+ return rc;
+}
+
+int Curl_hp_accept(int sockfd, struct sockaddr *addr, socklen_t
*addrlen)
+{
+ int rc;
+ int len = *addrlen;
+ rc = accept(sockfd, addr, &len);
+ // I think I would always do the copy. if(!rc)
+ *addrlen = len;
+ return rc;
+}
+
+
+ssize_t Curl_hp_recvfrom(int s, void *buf, size_t len, int flags,
+ struct sockaddr *from, socklen_t *fromlen) {
+ ssize_t rc;
+ int fromlen32 = *fromlen;
+ rc = recvfrom(s, buf, len, flags, from, &fromlen32);
+ // I think I would always do the copy. if(!rc)
+ *fromlen = fromlen32;
+ return rc;
+
+}
+
+#endif

$ /usr/local/bin/diff -u curl-7.16.2/lib/setup_once.h
curl-7.16.2.fix2/lib/setup_once.h
--- curl-7.16.2/lib/setup_once.h 2007-04-11 06:10:22 -0700
+++ curl-7.16.2.fix2/lib/setup_once.h 2007-06-12 11:14:40 -0700
@@ -343,6 +343,28 @@
 #define EREMOTE WSAEREMOTE
 #endif
 
+#if defined (__LP64__) && defined(__hpux) && !
defined(_XOPEN_SOURCE_EXTENDED)
+#include <sys/socket.h>
+/* HP-UX has this oddity where it features a few functions that don't
work
+ with socklen_t so we need to convert to ints
+
+ This is due to socklen_t being 64bit under 64bit ABI, but the
+ pre-xopen (default) interfaces requiring an int, which is 32bits.
+
+ Anytime socklen_t is passed by pointer, the function truncates
+ the 64bit socklen_t value by treating it as a 32bit value. */
+#define getsockname(a,b,c) Curl_hp_getsockname((a),(b),(c))
+#define getsockopt(a,b,c,d,e) Curl_hp_getsockopt((a),(b),(c),(d),(e))
+#define accept(a,b,c) Curl_hp_accept((a),(b),(c))
+#define recvfrom(a,b,c,d,e,f) Curl_hp_recvfrom((a),(b),(c),(d),(e),(f))
+int Curl_hp_getsockname(int s, struct sockaddr *name, socklen_t
*namelen);
+int Curl_hp_getsockopt(int s, int level, int optname, void *optval,
+ socklen_t *optlen);
+int Curl_hp_accept(int sockfd, struct sockaddr *addr, socklen_t
*addrlen);
+ssize_t Curl_hp_recvfrom(int s, void *buf, size_t len, int flags,
+ struct sockaddr *from, socklen_t *fromlen);
+#endif
+
 
 #endif /* __SETUP_ONCE_H */
 

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2007-06-12 01:39

Message:
Logged In: YES
user_id=1110
Originator: NO

so all the other uses of socklen_t in libcurl is fine as is, it is just
when we use getsockname() and getsockopt() we need to use an "improved
socklen_t" ?

And yeah, I think I'm in favour of the #define getsockopt()
Curl_hp_getsockopt() approach so that the main source code can remain
without #ifdefs for this case.

Can you check this patch and verify that I got this right, or even fix it
in case I missed some details. I've put the patch here:

http://daniel.haxx.se/curl/hp-socklen.patch

----------------------------------------------------------------------

Comment By: rrauenza (rrauenza)
Date: 2007-06-11 09:32

Message:
Logged In: YES
user_id=1309815
Originator: YES

simply #defining apparently doesn't work.. although I only did it by:

$ CFLAGS="+DD64 -Dsocklen_t=int"

Adding it to config.h doesn't work either since it is included before
system headers..

 cc -DHAVE_CONFIG_H -I../include -I../lib -I../lib +DD64 -Dsocklen_t=int
-c +Maked file.c -DPIC -o .libs/file.o
"/usr/include/sys/socket.h", line 205: error #2084: invalid combination
of
          type specifiers
  typedef size_t socklen_t;
                 ^
Here's what it gets by default:

vega7077-root-># grep socklen lib/config.h
#define GETNAMEINFO_TYPE_ARG2 socklen_t
/* type to use in place of socklen_t if not defined */
/* #undef socklen_t */
vega7077-root->#

The problem isn't that socklen_t doesn't exist, it's that the prototype
(and function) for the xopen version and the libc version are different.

Rich

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2007-06-11 06:51

Message:
Logged In: YES
user_id=1110
Originator: NO

The configure script already checks for an existing socklen_t type and if
it doesn't exist, it provides an alternative. What does your lib/config.h
say about socklen_t when configure has run?

If you make sure it does a '#define socklen_t int' in there, does that
make the build work?

----------------------------------------------------------------------

Comment By: rrauenza (rrauenza)
Date: 2007-06-08 15:41

Message:
Logged In: YES
user_id=1309815
Originator: YES

No, it can't be fixed by just defining _XOPEN_SOURCE_EXTENDED in curl's
configure because some applications linking with curl are compiled without
it. (The one I found this bug with is has this exact case).

If my application uses the BSD interfaces, it would not work with curl if
curl was compiled with _XOPEN_SOURCE_EXTENDED. It gets sticky because some
OTHER 3rd party dependencies may or may not have XOPEN_SOURCE defined and
you have to find the common denominator.. or where it breaks the least.
Best compatibility is to allow both modes of compilations by end user
(later versions of HPUX has a fix for this to allow mixed linkages.. but it
won't be on the earlier OS's you may wish to support.)

This doesn't break on other platforms because socklen is always a 32bit
value. For some reason HPUX typedefs socklen to be a size_t, which is
64bit in 64bit ABI's.

If there is a common header you might do something like..

#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
#define socklen_t int
#endif

but I don't know offhand if that could break anything else... outside of
system include files, of course.

Here's more context of my changes in socket.c -- this is really only a
problem for the socket calls that take a pointer to a socklen_t. The
others are passed by value and the compiler just casts the 64bit value down
to an int.

Interestingly, krb4.c works ok because it uses an int:

  int l = sizeof(conn->local_addr);
  struct SessionHandle *data = conn->data;
  CURLcode result;

  if(getsockname(conn->sock[FIRSTSOCKET],
                 (struct sockaddr *)LOCAL_ADDR, &l) < 0)
    perror("getsockname()");

Diff's:

$ /usr/local/bin/diff -u ftp.c ftp.c.orig
--- ftp.c 2007-06-08 15:34:22 -0700
+++ ftp.c.orig 2007-06-08 15:28:09 -0700
@@ -222,12 +222,7 @@
 #else
       struct sockaddr_in add;
 #endif
-
-#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
- int size = sizeof(add);
-#else
       socklen_t size = (socklen_t) sizeof(add);
-#endif
 
       if(0 == getsockname(sock, (struct sockaddr *) &add, &size)) {
         size = sizeof(add);
@@ -819,11 +814,7 @@
    */
   struct Curl_sockaddr_storage ss;
   struct addrinfo *res, *ai;
-#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
- int sslen;
-#else
   socklen_t sslen;
-#endif
   char hbuf[NI_MAXHOST];
   struct sockaddr *sa=(struct sockaddr *)&ss;
   char tmp[1024];
@@ -1122,11 +1113,7 @@
       if(bind(portsock, (struct sockaddr *)&sa, sslen) == 0) {
         /* we succeeded to bind */
         struct sockaddr_in add;
-#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
- int socksize = sizeof(add);
-#else
         socklen_t socksize = sizeof(add);
-#endif
 
         if(getsockname(portsock, (struct sockaddr *) &add,
                        &socksize)) {
$ /usr/local/bin/diff -u connect.c connect.c.orig
--- connect.c 2007-06-08 15:25:36 -0700
+++ connect.c.orig 2007-06-08 15:25:51 -0700
@@ -365,11 +365,7 @@
     if( bind(sockfd, sock, socksize) >= 0) {
       /* we succeeded to bind */
       struct Curl_sockaddr_storage add;
-#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
- int size;
-#else
       socklen_t size;
-#endif
 
       size = sizeof(add);
       if(getsockname(sockfd, (struct sockaddr *) &add, &size) < 0) {
@@ -409,11 +405,7 @@
   bool rc = TRUE;
 #ifdef SO_ERROR
   int err = 0;
-#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
- int errSize = sizeof(err);
-#else
   socklen_t errSize = sizeof(err);
-#endif
 
 #ifdef WIN32
   /*

Another option could be something like this where hp_getsockname does the
cast/copy from socklen_t to temp int before passing the ptr..

#if defined(__hpux) && ! defined(_XOPEN_SOURCE_EXTENDED)
#define getsockname(a,b,c) hp_getsockname(a,b,c)
#define getsockopt(a,b,c) hp_getopt(a,b,c)
#endif

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2007-06-08 01:10

Message:
Logged In: YES
user_id=1110
Originator: NO

Can you please submit the patch done with diff -u ? I'm having trouble
reading it unless there's some context in it as well.

Is there any particular reason why we shouldn't just try to define
_XOPEN_SOURCE_EXTENDED properly and make your system act/work like other
systems instead? I'd really hate to add such a kludgey #ifdef mess in at
least three different source files. It'll also just beg for this to happen
the next time we add such a function call or modify one of the existing.

I would really prefer a way that cures the problem without relying on
#ifdefs where getsockopt() is used. You think that is possible?

----------------------------------------------------------------------

Comment By: rrauenza (rrauenza)
Date: 2007-06-07 16:58

Message:
Logged In: YES
user_id=1309815
Originator: YES

Looks like ftp.c and tftp.c also need to be fixed. Anywhere those calls
are made.

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1733119&group_id=976
Received on 2007-06-13