Hello,

Please review the code change. The diff is against the master branch of git://git.openldap.org/openldap.git.

Five files are modified and one is added (see output of 'git status -uno' command).

It was written and tested for Linux/Unix operating system. Windows, VxWorks, etc. are not covered/affected. In os-ip.c, the ldap_socket_bind_addr() function is only called to effectively bind LDAP client socket to a local IP address if HAVE_GETADDRINFO and HAVE_INET_NTOP are defined.

The functionality is made compatible with Microsoft implementation of the LDAP_OPT_SOCKET_BIND_ADDRESSES option wrt "You should provide both IPv4 and IPv6 local addresses, if available, because both IPv4 and IPv6 server addresses can be used for socket connect. Socket bind will fail if there is an address family mismatch. (Re. https://msdn.microsoft.com/en-us/library/aa367019(v=vs.85).aspx). This code change handles space-separated and comma- separated addresses.

libldap$git status -uno On branch master Your branch is up-to-date with 'origin/master'. Changes to be committed: (use "git reset HEAD <file>..." to unstage)

new file: addr.c

Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory)

modified: ../../include/ldap.h modified: Makefile.in modified: ldap-int.h modified: options.c modified: os-ip.c

include$git diff ldap.h diff --git a/include/ldap.h b/include/ldap.h index 588e906..0268f0e 100644 --- a/include/ldap.h +++ b/include/ldap.h @@ -109,6 +109,8 @@ LDAP_BEGIN_DECL #define LDAP_OPT_DIAGNOSTIC_MESSAGE 0x0032 #define LDAP_OPT_ERROR_STRING LDAP_OPT_DIAGNOSTIC_MESSAGE #define LDAP_OPT_MATCHED_DN 0x0033 +/* same option code as Microsoft LDAP_OPT_SOCKET_BIND_ADDRESSES */ +#define LDAP_OPT_BIND_ADDRESSES 0x0044 /* 0x0034 - 0x3fff not defined */ /* 0x0091 used by Microsoft for LDAP_OPT_AUTO_RECONNECT */ #define LDAP_OPT_SSPI_FLAGS 0x0092 @@ -815,6 +817,15 @@ typedef struct ldap_url_desc { #define LDAP_URL_ERR_BADEXTS 0x0a /* bad or missing extensions */

/* + * data type for ldap socket bind addresses + */ +typedef struct ldap_bind_addr { + struct ldap_bind_addr *lba_next; + char *lba_address; + int lba_family; +} LDAPBindAddr; + +/* * LDAP sync (RFC4533) API */

libldap$git diff ldap-int.h diff --git a/libraries/libldap/ldap-int.h b/libraries/libldap/ldap-int.h index bcd6118..1cb466d 100644 --- a/libraries/libldap/ldap-int.h +++ b/libraries/libldap/ldap-int.h @@ -206,6 +206,9 @@ struct ldapoptions { char* ldo_defbase; char* ldo_defbinddn; /* bind dn */

+ /* socket bind addresses */ + LDAPBindAddr *ldo_bind_addr; + /* * Per connection tcp-keepalive settings (Linux only, * ignored where unsupported) @@ -804,6 +807,19 @@ LDAP_F (char *) ldap_url_list2hosts LDAP_P(( LDAPURLDesc *ludlist ));

/* + * in addr.c + */ +LDAP_F (void) ldap_free_bind_addr LDAP_P(( + LDAPBindAddr *lba_ptr )); + +LDAP_F (int) ldap_parse_bind_addr LDAP_P(( + LDAPBindAddr **lba_list, + const char *addresses )); + +LDAP_F (char *) ldap_list_bind_addr LDAP_P(( + LDAPBindAddr *lba_list )); + +/* * in cyrus.c */

libldap$git diff options.c diff --git a/libraries/libldap/options.c b/libraries/libldap/options.c index 1705bd9..e3a9077 100644 --- a/libraries/libldap/options.c +++ b/libraries/libldap/options.c @@ -245,6 +245,11 @@ ldap_get_option( rc = LDAP_OPT_SUCCESS; break;

+ case LDAP_OPT_BIND_ADDRESSES: + * (char **) outvalue = ldap_list_bind_addr(lo->ldo_bind_addr); + rc = LDAP_OPT_SUCCESS; + break; + case LDAP_OPT_HOST_NAME: * (char **) outvalue = ldap_url_list2hosts(lo->ldo_defludp); rc = LDAP_OPT_SUCCESS; @@ -541,6 +546,23 @@ ldap_set_option( rc = LDAP_OPT_SUCCESS; break;

+ case LDAP_OPT_BIND_ADDRESSES: { + const char *addr = (const char *) invalue; + LDAPBindAddr *lba_list = NULL; + rc = LDAP_OPT_SUCCESS; + + if(addr != NULL) { + rc = ldap_parse_bind_addr(&lba_list, addr); + } + + if (rc == LDAP_OPT_SUCCESS) { + if (lo->ldo_bind_addr != NULL) { + ldap_free_bind_addr(lo->ldo_bind_addr); + } + lo->ldo_bind_addr = lba_list; + } + break; + }

case LDAP_OPT_HOST_NAME: { const char *host = (const char *) invalue;

libldap$git diff os-ip.c diff --git a/libraries/libldap/os-ip.c b/libraries/libldap/os-ip.c index c7cee92..fbfb8a9 100644 --- a/libraries/libldap/os-ip.c +++ b/libraries/libldap/os-ip.c @@ -209,6 +209,50 @@ ldap_int_prepare_socket(LDAP *ld, int s, int proto ) return 0; }

+static int +ldap_socket_bind_addr(LDAP *ld, int s, int sf, int st) +{ + LDAPBindAddr **ba; + LDAPBindAddr *bap; + struct addrinfo hints, *bai; + int err; + int matched = 0; + + for ( ba = &ld->ld_options.ldo_bind_addr; *ba != NULL; ba = &(*ba)->lba_next ) { + bap = *ba; + if ( bap->lba_family == sf ) { + matched = 1; + break; + } + } + + if ( !matched ) { + osip_debug(ld, "ldap_socket_bind_addr: no match\n", 0, 0, 0); + return -1; + } + + memset( &hints, 0, sizeof(hints) ); + hints.ai_flags = AI_ADDRCONFIG; + hints.ai_family = sf; + hints.ai_socktype = st; + + err = getaddrinfo( bap->lba_address, NULL, &hints, &bai ); + if ( err != 0 ) { + osip_debug( ld, "ldap_socket_bind_addr: %s getaddrinfo error %s\n", + bap->lba_address, AC_GAI_STRERROR(err), 0 ); + return -1; + } + + if ( bind( s, bai->ai_addr, bai->ai_addrlen )) { + osip_debug( ld, "ldap_socket_bind_addr: %s bind error %s\n", + bap->lba_address, strerror(errno), 0 ); + return -1; + } + + osip_debug( ld, "ldap_socket_bind_addr: %s bind success\n", bap->lba_address, 0, 0 ); + return 0; +} + #ifndef HAVE_WINSOCK

#undef TRACE @@ -655,6 +699,8 @@ ldap_connect_to_host(LDAP *ld, Sockbuf *sb, } break; }

+ ldap_socket_bind_addr( ld, s, sai->ai_family, socktype); + rc = ldap_pvt_connect( ld, s, sai->ai_addr, sai->ai_addrlen, async ); if ( rc == 0 || rc == -2 ) {

libldap$cat addr.c /* LIBLDAP addr.c -- Utility functions for socket bind addresses */ /* $OpenLDAP$ */ /* This work is part of OpenLDAP Software http://www.openldap.org/. * * Copyright 1998-2017 The OpenLDAP Foundation. * All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted only as authorized by the OpenLDAP * Public License. * * A copy of this license is available in the file LICENSE in the * top-level directory of the distribution or, alternatively, at * http://www.OpenLDAP.org/license.html. */ /* Portions Copyright (c) 1996 Regents of the University of Michigan. * All rights reserved. */

#include "portable.h" #include <ac/socket.h> #include "ldap-int.h"

static void ldap_free_bind_addr_list( LDAPBindAddr *lba_list ) { LDAPBindAddr *lba_ptr, *next;

for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = next) { next = lba_ptr->lba_next; ldap_free_bind_addr(lba_ptr); } }

void ldap_free_bind_addr( LDAPBindAddr *lba_ptr ) { if (lba_ptr == NULL) { return; }

if (lba_ptr->lba_address != NULL) { LDAP_FREE( lba_ptr->lba_address ); }

LDAP_FREE(lba_ptr); }

int ldap_parse_bind_addr( LDAPBindAddr **lba_list, const char *addresses ) { int i; char **specs; LDAPBindAddr *lba_ptr;

assert(lba_list != NULL); assert(addresses != NULL);

*lba_list = NULL;

specs = ldap_str2charray(addresses, ", "); if (specs == NULL) { return LDAP_NO_MEMORY; }

/* Count the number of local addresses */ for (i = 0; specs[i] != NULL; i++) /* NO-OP */;

/* Put the addresses in the list backward */ while (--i >= 0) { lba_ptr = LDAP_CALLOC(1, sizeof(LDAPBindAddr)); if (lba_ptr == NULL) { ldap_charray_free(specs); ldap_free_bind_addr_list(*lba_list); *lba_list = NULL; return LDAP_NO_MEMORY; }

lba_ptr->lba_address = specs[i]; specs[i] = NULL;

if (strchr(lba_ptr->lba_address, ':') != NULL) { lba_ptr->lba_family = AF_INET6; } else { lba_ptr->lba_family = AF_INET; }

lba_ptr->lba_next = *lba_list; *lba_list = lba_ptr; }

/* This should be an array of NULLs now */ ldap_charray_free(specs);

return LDAP_SUCCESS; }

char * ldap_list_bind_addr ( LDAPBindAddr *lba_list ) { LDAPBindAddr *lba_ptr; int size; char *s, *p;

if (lba_list == NULL) { return NULL; }

/* Figure out how big the string of bind addresses is */ size = 1; /* nul-terminated string */ for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) { if (lba_ptr->lba_address == NULL) { continue; } size += strlen(lba_ptr->lba_address) + 1; /* address and space or comma */ }

s = LDAP_MALLOC(size); if (s == NULL) { return NULL; }

p = s; for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) { if (lba_ptr->lba_address == NULL) { continue; } strcpy(p, lba_ptr->lba_address); p += strlen(lba_ptr->lba_address); *p++ = ' '; }

if (p != s) p--; /* nuke that extra space or comma */

*p = '\0'; return s; }

libldap$git diff Makefile.in diff --git a/libraries/libldap/Makefile.in b/libraries/libldap/Makefile.in index e72c14d..679ecdc 100644 --- a/libraries/libldap/Makefile.in +++ b/libraries/libldap/Makefile.in @@ -19,7 +19,7 @@ PROGRAMS = apitest dntest ftest ltest urltest

SRCS = bind.c open.c result.c error.c compare.c search.c \ controls.c messages.c references.c extended.c cyrus.c \ - modify.c add.c modrdn.c delete.c abandon.c \ + modify.c add.c addr.c modrdn.c delete.c abandon.c \ sasl.c gssapi.c sbind.c unbind.c cancel.c \ filter.c free.c sort.c passwd.c whoami.c vc.c \ getdn.c getentry.c getattr.c getvalues.c addentry.c \ @@ -32,7 +32,7 @@ SRCS = bind.c open.c result.c error.c compare.c search.c \

OBJS = bind.lo open.lo result.lo error.lo compare.lo search.lo \ controls.lo messages.lo references.lo extended.lo cyrus.lo \ - modify.lo add.lo modrdn.lo delete.lo abandon.lo \ + modify.lo add.lo addr.lo modrdn.lo delete.lo abandon.lo \ sasl.lo gssapi.lo sbind.lo unbind.lo cancel.lo \ filter.lo free.lo sort.lo passwd.lo whoami.lo vc.lo \ getdn.lo getentry.lo getattr.lo getvalues.lo addentry.lo \

/Daniel

-----Original Message----- From: Quanah Gibson-Mount [mailto:quanah@symas.com] Sent: Monday, June 12, 2017 4:07 PM To: Daniel Le daniel.le@exfo.com; 'openldap-devel@openldap.org' openldap-devel@openldap.org Subject: RE: ITS#8654 - Option for LDAP client to bind to a local address

Hi Daniel,

I would suggest sending in a patch against master to the -devel list for review.

For final inclusion if it is approved, see:

http://www.openldap.org/devel/contributing.html

Regards, Quanah

--On Monday, June 12, 2017 8:34 PM +0000 Daniel Le daniel.le@exfo.com wrote:

I've got a chance to write (and test) the code to add API support for socket binding addresses.

Should I send the code diff to this openldap-devel email list for review? How to submit a patch request?

Daniel

-----Original Message----- From: Daniel Le Sent: Tuesday, May 16, 2017 6:02 PM To: 'openldap-devel@openldap.org' openldap-devel@openldap.org Subject: ITS#8654 - Option for LDAP client to bind to a local address

Hello,

In reference to the enhancement request ITS#865, please comment on the following to add support for binding a local IP address to client socket. This is just an outline of changes for one local address. I am not sure whether a list of local addresses is necessary. If it is, then a new function, similarly to ldap_url_parsehosts, may be written to parse the list of local addresses and store them into a linked list. In my use case, only one IPv4 or IPv6 local address is used for binding.

• Modify ldap.h and ldap_set_option to handle the new option

LDAP_OPT_LOCAL_ADDRESS. Should it be named LDAP_OPT_CLIENT_ADDRESS, LDAP_OPT_SOCKET_BIND_ADDRESS...?

• Modify struct ldapoptions in ldap-int.h to add element "char

*ldo_local_address" to hold client local address when ldap_set_option(LDAP_OPT_LOCAL_ADDRESS...) is executed. This can char pointer can point to an IPv4 address or IPv6 address.

• ldap_connect_to_host() in os-ip.c After the connection socket is created (ldap_int_socket) and before it

is connected (ldap_pvt_connect), extract the local IP address. If local address family (AF_INET/ AF_INET6) matches the one of the host, bind socket to the local address.

Regards, Daniel

--

Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: http://www.symas.com

On Mon, Jun 12, 2017 at 10:15:56PM +0000, Daniel Le wrote:

Please review the code change. The diff is against the master branch of git://git.openldap.org/openldap.git.

I'm not able to apply the patch from this email. The whitespace in the context has been mangled - your mail only contains spaces, where the original files contain tabs. My review is therefore only based on reading the code, not testing it. Please attach the actual git commit (use git-format-patch(1)), or link to a branch in a git repository.

I don't know a lot of this code very well, so please forgive any stupid questions and try to answer patiently. :)

In os-ip.c, the ldap_socket_bind_addr() function is only called to effectively bind LDAP client socket to a local IP address if HAVE_GETADDRINFO and HAVE_INET_NTOP are defined.

OK. Does the code still compile and behave reasonably if they aren't? I'm wondering if it still makes sense to compile any of the contents of addr.c or even ldap_socket_bind_addr itself, for example, if getaddrinfo is missing. And I wonder also about the expected return value of ldap_set_option() when it's not actually possible to respect the requested setting.

Changes to be committed: (use "git reset HEAD <file>..." to unstage)

   new file:   addr.c

Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory)

   modified:   ../../include/ldap.h
   modified:   Makefile.in
   modified:   ldap-int.h
   modified:   options.c
   modified:   os-ip.c

Please also update the ldap_get_option.3 man page. (And hopefully I don't need to say this, but I expect Microsoft's documentation is copyrighted and simply cloning or paraphrasing it would be a bad idea.)

include$git diff ldap.h diff --git a/include/ldap.h b/include/ldap.h index 588e906..0268f0e 100644 --- a/include/ldap.h +++ b/include/ldap.h @@ -109,6 +109,8 @@ LDAP_BEGIN_DECL #define LDAP_OPT_DIAGNOSTIC_MESSAGE 0x0032 #define LDAP_OPT_ERROR_STRING LDAP_OPT_DIAGNOSTIC_MESSAGE #define LDAP_OPT_MATCHED_DN 0x0033 +/* same option code as Microsoft LDAP_OPT_SOCKET_BIND_ADDRESSES */ +#define LDAP_OPT_BIND_ADDRESSES 0x0044

Wouldn't it be more compatible to name the option the same?

/* 0x0034 - 0x3fff not defined */ /* 0x0091 used by Microsoft for LDAP_OPT_AUTO_RECONNECT */ #define LDAP_OPT_SSPI_FLAGS 0x0092 @@ -815,6 +817,15 @@ typedef struct ldap_url_desc { #define LDAP_URL_ERR_BADEXTS 0x0a /* bad or missing extensions */

/*

• • data type for ldap socket bind addresses
• */

+typedef struct ldap_bind_addr {

•    struct ldap_bind_addr *lba_next;
  

•    char   *lba_address;
  

•    int    lba_family;
  

+} LDAPBindAddr;

Does this definition need to be exposed to the world? Could it be in ldap-int.h?

<snip ...>

libldap$git diff os-ip.c diff --git a/libraries/libldap/os-ip.c b/libraries/libldap/os-ip.c index c7cee92..fbfb8a9 100644 --- a/libraries/libldap/os-ip.c +++ b/libraries/libldap/os-ip.c @@ -209,6 +209,50 @@ ldap_int_prepare_socket(LDAP *ld, int s, int proto ) return 0; }

+static int +ldap_socket_bind_addr(LDAP *ld, int s, int sf, int st) +{

•   LDAPBindAddr **ba;
  

•   LDAPBindAddr *bap;
  

Hmm. The double-pointer is the one with fewer "p"s in its name? :)

•   struct addrinfo hints, *bai;
  

•   int err;
  

•   int matched = 0;
  

•   for ( ba = &ld->ld_options.ldo_bind_addr; *ba != NULL; ba = &(*ba)->lba_next ) {
  

What's the use of the double pointer when we're only reading the list? I know this idiom is helpful when we want to modify the list, but that's not the case here, right? The addr.c functions later on also only use a single pointer.

•           bap = *ba;
  

•           if ( bap->lba_family == sf ) {
  

•                   matched = 1;
  

•                   break;
  

•           }
  

•   }
  

•   if ( !matched ) {
  

This looks like it could perhaps just be if (*ba == NULL), i.e. we fell off the end of the list?

•           osip_debug(ld, "ldap_socket_bind_addr: no match\n", 0, 0, 0);
  

•           return -1;
  

•   }
  

•   memset( &hints, 0, sizeof(hints) );
  

•   hints.ai_flags = AI_ADDRCONFIG;
  

Not AI_NUMERICHOST?

(I'm not necessarily looking for one answer or the other here; just a rationale, and the chosen behaviour to be documented.)

•   hints.ai_family = sf;
  

•   hints.ai_socktype = st;
  

Did you also test this with a UDP (cldap://) socket? I can't think why it wouldn't work, but I'd like to be certain. :)

•   err = getaddrinfo( bap->lba_address, NULL, &hints, &bai );
  

Assuming we're _not_ using AI_NUMERICHOST, then does this need to be holding the ldap_int_resolv_mutex?

•   if ( err != 0 ) {
  

•           osip_debug( ld, "ldap_socket_bind_addr: %s getaddrinfo error %s\n",
  

•                       bap->lba_address, AC_GAI_STRERROR(err), 0 );
  

•           return -1;
  

•   }
  

•   if ( bind( s, bai->ai_addr, bai->ai_addrlen )) {
  

•           osip_debug( ld, "ldap_socket_bind_addr: %s bind error %s\n",
  

•                       bap->lba_address, strerror(errno), 0 );
  

Should probably be sock_errstr( sock_errno() )?

Bearing in mind that osip_debug is only TRACE level: - should we ldap_pvt_set_errno() if bind() fails? - can we do anything sensible with the getaddrinfo() result?

•           return -1;
  

•   }
  

•   osip_debug( ld, "ldap_socket_bind_addr: %s bind success\n", bap->lba_address, 0, 0 );
  

•   return 0;
  

+}

#ifndef HAVE_WINSOCK

#undef TRACE @@ -655,6 +699,8 @@ ldap_connect_to_host(LDAP *ld, Sockbuf *sb, } break; }

•           ldap_socket_bind_addr( ld, s, sai->ai_family, socktype);
  

•          rc = ldap_pvt_connect( ld, s,
                   sai->ai_addr, sai->ai_addrlen, async );
           if ( rc == 0 || rc == -2 ) {
  
  

libldap$cat addr.c /* LIBLDAP addr.c -- Utility functions for socket bind addresses */

Maybe "bindaddr.c"? "addr.c" sounds more generic - I would expect it to contain utilities for working with addresses in general, while this code is for a pretty specific purpose.

I'd drop "LIBLDAP"; seems to be a url.c'ism, not a pattern.

/* $OpenLDAP$ */ /* This work is part of OpenLDAP Software http://www.openldap.org/.

• Copyright 1998-2017 The OpenLDAP Foundation.

"1998-2017" seems slightly odd for new code, as does the "Portions Copyright (c) 1996" below. I admit though that I'm not really sure what standard practice is here for new code derived from older.

• All rights reserved.
• Redistribution and use in source and binary forms, with or without
• modification, are permitted only as authorized by the OpenLDAP
• Public License.
• A copy of this license is available in the file LICENSE in the
• top-level directory of the distribution or, alternatively, at
• http://www.OpenLDAP.org/license.html.

*/ /* Portions Copyright (c) 1996 Regents of the University of Michigan.

• All rights reserved.

*/

<snip ...>

int ldap_parse_bind_addr( LDAPBindAddr **lba_list, const char *addresses ) { int i; char **specs; LDAPBindAddr *lba_ptr;

   assert(lba_list != NULL);
   assert(addresses != NULL);

   *lba_list = NULL;

   specs = ldap_str2charray(addresses, ", ");
   if (specs == NULL) {
           return LDAP_NO_MEMORY;
   }

   /* Count the number of local addresses */
   for (i = 0; specs[i] != NULL; i++) /* NO-OP */;

   /* Put the addresses in the list backward */
   while (--i >= 0) {

Any reason for that, besides reusing the existing 'i'?

           lba_ptr = LDAP_CALLOC(1, sizeof(LDAPBindAddr));
           if (lba_ptr == NULL) {
                   ldap_charray_free(specs);
                   ldap_free_bind_addr_list(*lba_list);
                   *lba_list = NULL;
                   return LDAP_NO_MEMORY;
           }

           lba_ptr->lba_address = specs[i];
           specs[i] = NULL;

           if (strchr(lba_ptr->lba_address, ':') != NULL) {
                   lba_ptr->lba_family = AF_INET6;

Does this need to be hidden behind LDAP_PF_INET6?

Do you need actually need lba_family? os-ip.c has:

#if defined( HAVE_GETADDRINFO ) && defined( HAVE_INET_NTOP ) # ifdef LDAP_PF_INET6 int ldap_int_inet4or6 = AF_UNSPEC; # else int ldap_int_inet4or6 = AF_INET; # endif #endif

I think I sort of see how you arrived at this: storing the string lets ldap_list_bind_addr return the same one again later; and if you store the string and not a family, then the loop in ldap_socket_bind_addr suddenly starts making a lot more getaddrinfo() calls. But the strchr() is bugging me; I'd rather see getaddrinfo() responsible for the parsing if possible.

           } else {
                   lba_ptr->lba_family = AF_INET;
           }

           lba_ptr->lba_next = *lba_list;
           *lba_list = lba_ptr;
   }

   /* This should be an array of NULLs now */
   ldap_charray_free(specs);

   return LDAP_SUCCESS;

}

char * ldap_list_bind_addr ( LDAPBindAddr *lba_list ) { LDAPBindAddr *lba_ptr; int size; char *s, *p;

   if (lba_list == NULL) {
           return NULL;
   }

   /* Figure out how big the string of bind addresses is */
   size = 1; /* nul-terminated string */

If there are any items, you already have a place for the nul: overwriting the final separator. Therefore I wonder if this (size=1) could just be a special case when there are 0 elements?

   for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) {
           if (lba_ptr->lba_address == NULL) {
                   continue;
           }
           size += strlen(lba_ptr->lba_address) + 1;  /* address and space or comma */

Seems most likely to be a space in this implementation, but yes ;)

   }

   s = LDAP_MALLOC(size);
   if (s == NULL) {
           return NULL;
   }

   p = s;
   for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) {
           if (lba_ptr->lba_address == NULL) {
                   continue;
           }
           strcpy(p, lba_ptr->lba_address);
           p += strlen(lba_ptr->lba_address);
           *p++ = ' ';
   }

   if (p != s)
           p--;  /* nuke that extra space or comma */

   *p = '\0';
   return s;

}

The overall design makes sense to me. These comments were largely nit-picks.

Hope this helps,

Ryan