(ITS#3405) Suggested change for ldap_pvt_inet_aton

[jeff.lewis29@verizon.net]

Full_Name: Jeff Lewis
Version: 2.2.18
OS: HP-UX B.11.11
URL: ftp://ftp.openldap.org/incoming/
Submission from: (NULL) (192.127.94.7)


For most 64 bit architectures, the ldap_pvt_inet_aton function in
libraries/libldap/os-ip.c does not work right.  This not a pressing problem by
any stretch of the imagination but it is one that may need attention at some
point.

Of the platforms I'm using (Solaris, HP-UX, AIX and Linux), all support
inet_aton, but while chasing down a different problem (turned out to be a
cockpit error on my part when I ran configure), I experimentally undef'd
HAVE_INET_ATON and that's when I discovered this problem.

When HAVE_INET_ATON is not defined in the portable.h file, ldap_pvt_inet_aton is
used instead of the real inet_aton.  This function calls inet_addr to obtain the
conversion between the "human readable" IP address and the internal binary form.
 On all 64 bit platforms that I work with, the return from inet_addr is an
unsigned 32 bit value.  The ldap_pvt_inet_aton function receives this value in
an unsigned long.  Since both values are unsigned, no sign extension occurs.

There is a test in ldap_pvt_inet_aton that does not work right.  It is:

  unsigned long u = inet_addr( host );
  if ( u != 0xffffffff || u != (unsigned long) -1 ) {

This test will always succeed on 64 bit platforms (except Windows) because the
value 0xffffffff is returned as an unsigned 32 bit value from inet_addr.  The
first part of the test (u != 0xffffffff) fails as it should, but the second part
succeeds (u != (unsigned long)-1) because (unsigned long)-1 on a 64 bit platform
is 0xffffffffffffffff.  Because the test always succeeds on 64 bit platforms,
ldap_pvt_inet_aton always succeeds with either a properly converted IP address
or an IP address of 255.255.255.255.

Based on what I've observed in my four environments where inet_addr always
returns an unsigned 32 bit value, I believe the code should read:

  unsigned long u = inet_addr( host );
  if ( u != 0xffffffff ) {

I elected to leave the declaration of "u" alone because Windows defines
inet_addr's return to be "unsigned long".  On Windows "long" is always 32 bits,
even in their 64 bit releases.  I'd think "unsigned int" would work just as
well.

I have tested this change without a define for HAVE_INET_ATON using a debugger
to make sure I made it into ldap_pvt_inet_aton.  The change works fine in the
following environments:

  HP-UX B.11.11 on PA-RISC 32 bit
  HP-UX B.11.11 on PA-RISC 64 bit
  HP-UX B.11.23 on Intel ia64

  SunOS 5.8 and 5.9 on SPARC 32 bit
  SunOS 5.8 and 5.9 on SPARC 64 bit

  Red Hat Enterprise Linux AS release 3 (Taroon Update 1) on Intel i386 and
x64_64
  Red Hat Enterprise Linux AS release 3 (Taroon Update 2) on Intel ia64

  AIX version 5: POWER 32 bit
  AIX version 5: POWER 64 bit

  MP-RAS (SVR4 version 3): Intel i386

The only concern I have about my proposed fix is whether or not there are 64 bit
O/S architectures where inet_addr returns an unsigned 64 bit value and singals
failure by returning 0xffffffffffffffff instead of 0xffffffff.  If there are,
then the fix should probably read thusly:

  unsigned long u = inet_addr( host );
  if ( u != 0xffffffff && u != (unsigned long) -1 ) {

This one has been tested on the same platforms and works equally as well.