Here are my comments on release 4 of the patch. I went through it more
thoroughly this time, but I'll admit I don't have OS/400 experience.

+/* No OS/400 header file defines u_int32_t. */
+#define u_int32_t unsigned long

Making this a typedef would be safer.

+#pragma enum(int)

This sounds like a dangerous pragma to use, as it (presumably) could
change the size of some of the structs defined in the remaining header files.
If this isn't used consistently, both within curl and in user apps, this
could cause inconsistent structs that would create havoc.

+#ifndef __OS400__
+#define ICONV_OPEN_ERROR(t) ((t) == (iconv_t) -1)
+#else
+#define ICONV_OPEN_ERROR(t) ((t).return_value == -1)
+#endif

This file is compiled for OS/400, so why is the #ifndef necessary?

+ for (i = 12; i > 7; i--) {
+ d = ccsid / 10;
+ buf[i] = '0' + (ccsid - 10 * d);
+ ccsid = d;
+ }

This can be replaced with the one liner:

  curl_msprintf(buf+8, "%05d", ccsid);

+ cp = realloc(cp, l);

This form of realloc is dangerous. If the system runs out of memory and
fails the realloc, then the original pointer is not freed. However, the
original pointer has been replaced by NULL and is therefore leaked.

+DEBUG=*ALL # Debug level
+OUTPUT=*NONE # Compilation output option.

If a file named THATSALL or HAVENONE exists, then this will do globbing
and expand to those names; they need to be quoted.

+# Create and compile the identification source file.
+
+echo '#pragma comment(user, "libcurl version '"${LIBCURL_VERSION}"'")' > os400.c
+echo '#pragma comment(date)' >> os400.c
+echo '#pragma comment(copyright, "Copyright (C) 1998-2007 Daniel Stenberg et al.")' >> os400.c

You forgot to add your name :^)

+static int
+convert(char * d, size_t dlen, int dccsid,
+ const char * s, int slen, int sccsid)
+
+{
+ int i;
+ iconv_t cd;
+ size_t lslen;
+
+ /**
+ *** Convert `sccsid'-coded `slen'-data bytes at `s' into `dccsid'-coded
+ *** data stored in the `dlen'-byte buffer at `d'.
+ *** If `slen' < 0, source string is null-terminated.
+ *** CCSID 65535 (no conversion) is replaced by the ASCII CCSID.
+ *** Return the destination length, or -1 if error.
+ **/
+
+ if (sccsid == 65535)
+ sccsid = ASCII_CCSID;
+
+ if (dccsid == 65535)
+ dccsid = ASCII_CCSID;
+
+ if (sccsid == dccsid) {
+ if (slen < 0)
+ lslen = strlen(s) + 1;
+
+ i = lslen < dlen? lslen: dlen;

If slen is >=0, then lslen here will be used when undefined.

+static char *
+dynconvert(int dccsid, const char * s, int slen, int sccsid)
+
+{
+ int cstr;
+ char * d;
+ size_t dlen;
+ int l;
+
+ /* Like convert, but the destination is allocated and returned. */
+
+ dlen = (size_t) (slen < 0? strlen(s): slen);
+ dlen *= 6; /* Allow some expansion. */

Why the magic number 6? Could the converted string really be 6 times longer?
A worst-case UTF-8 expansion is only 4 times longer, for example. The
number 6 is used fairly often in this file, and should probably be given a
macro to self-document it.

+ d = malloc(dlen + 1);
+
+ if (!d)
+ return (char *) NULL;
+
+ l = convert(d, dlen, dccsid, s, slen, sccsid);

Shouldn't there be a ++dlen somewhere above?

+ if (l < 0) {
+ free(d);
+ return (char *) NULL;
+ }
+
+ d[l++] = '\0';
+
+ if (l < ++dlen)
+ d = realloc(d, l);
+
+ return d;
+}
+
+
+char *
+curl_version_ccsid(unsigned int ccsid)
+
+{
+ int i;
+ char * aversion;
+ char * eversion;
+
+ aversion = curl_version();
+
+ if (!aversion)
+ return aversion;
+
+ i = strlen(aversion) + 1;
+
+ if (!(eversion = Curl_thread_buffer(LK_CURL_VERSION, i + 1)))

Is the +1 really necessary here?

+ return (char *) NULL;
+
+ if (convert(eversion, i, ccsid, aversion, -1, ASCII_CCSID) < 0)
+ return (char *) NULL;

+struct curl_slist *
+curl_slist_append_ccsid(struct curl_slist * list,
+ const char * data, unsigned int ccsid)
+
+{
+ char * s;
+
+ s = (char *) NULL;
+
+ if (!data)
+ return curl_slist_append(list, data);
+
+ s = dynconvert(ASCII_CCSID, s, -1, ccsid);

Shouldn't this be:
   s = dynconvert(ASCII_CCSID, data, -1, ccsid);

+time_t
+curl_getdate_ccsid(const char * p, const time_t * unused, unsigned int ccsid)
+
+{
+ char * s;
+ time_t t;
+
+ if (!p)
+ return curl_getdate(p, unused);
+
+ s = dynconvert(ASCII_CCSID, s, -1, ccsid);

And this:

   s = dynconvert(ASCII_CCSID, p, -1, ccsid);

+curl_version_info_data *
+curl_version_info_ccsid(CURLversion stamp, unsigned int ccsid)
+
+{
+ curl_version_info_data * p;

You may want to put an assert in here on the sizeof(curl_version_info_data)
so if that structure is ever expanded without this function being modified,
it will be caught.

+ if (id->version) {
+ l = convert(cp, n, ccsid, id->version, -1, ASCII_CCSID);
+
+ if (l <= 0)
+ return (curl_version_info_data *) NULL;
+
+ id->version = cp;
+ cp += l;
+ n -= l;
+ }

This sequence of code occurs seven times in a row. It would be a good
candidate for turning into a helper function or a macro.

+Curl_is_formadd_string(CURLformoption option)

Probably a good idea to put an assert on CURLFORM_LASTENTRY in this
function to catch the case where a new CURLformoption is added without
modifying this function.

+
+CURLcode
+curl_easy_getinfo_ccsid(CURL * curl, CURLINFO info, ...)
+
+{
+ va_list arg;
+ void * paramp;
+ CURLcode ret;
+ unsigned int ccsid;
+ char * * cpp;
+ char * s;
+ char * d;
+ struct SessionHandle * data;
+
+ /* WARNING: unlike curl_easy_get_info(), the strings returned by this
+ procedure have to be free'ed. */
+
+ data = (struct SessionHandle *) curl;
+ va_start(arg, info);
+ paramp = va_arg(arg, void *);
+ ret = Curl_getinfo(data, info, paramp);
+
+ if (ret != CURLE_OK || ((int) info & CURLINFO_TYPEMASK) != CURLINFO_STRING ||
+ info == CURLINFO_STRING) {

I don't see how (info == CURLINFO_STRING) could ever be true.

+CURLFORMcode
+curl_formadd_ccsid(struct curl_httppost * * httppost,
+ struct curl_httppost * * last_post, ...)

This function is > 240 lines long and could (should) be split into several
more for readability and easier maintenance. Long function length is
one aspect of curl coding style you don't need to follow :^)

+
+{
+ va_list arg;
+ CURLformoption option;
+ CURLFORMcode result;
+ struct curl_forms * forms;
+ struct curl_forms * lforms;
+ unsigned int ccsid;
+ const char * cp;
+ int nargs;
+ int namex;
+ int namelengthx;
+ int contentx;
+ int lengthx;
+ unsigned int contentccsid;
+ unsigned int nameccsid;
+
+ /* Count the arguments. */

Rather than having a separate loop to count the arguments, can you just make
one pass and realloc the buffer on each iteration? A reasonable memory
allocator should handle all those reallocs without much penalty, and
eliminating a loop would make the code smaller and less likely to break
with maintenance.

+
+ va_start(arg, last_post);
+
+ for (nargs = 0;
+ (option = va_arg(arg, CURLformoption)) != CURLFORM_END; nargs++) {
+ if (Curl_is_formadd_string(option)) {
+ (void) va_arg(arg, char *); /* Eat string pointer. */
+ (void) va_arg(arg, long); /* Eat CCSID. */
+ continue;
+ }
+
+ switch (option) {
+
+ case CURLFORM_ARRAY:
+ for (forms = va_arg(arg, struct curl_forms *);
+ forms->option != CURLFORM_END; forms++) {
+ if (forms->option == CURLFORM_ARRAY)
+ return CURL_FORMADD_ILLEGAL_ARRAY;

va_end is missing here (although it may be optional on OS/400).

+
+ if (Curl_is_formadd_string(forms->option))
+ forms++; /* Eat CCSID. */

Is this right? Don't you need a va_arg here instead to be correct?

+
+ nargs++;
+ }
+
+ nargs--; /* Fix for loop increment. */
+ break;
+
+ case CURLFORM_PTRCONTENTS:
+ case CURLFORM_BUFFERPTR:
+ (void) va_arg(arg, char *); /* Not a convertible string. */
+ break;
+
+ case CURLFORM_NAMELENGTH:
+ case CURLFORM_BUFFERLENGTH:
+ case CURLFORM_CONTENTSLENGTH:
+ (void) va_arg(arg, long);
+ break;
+
+ case CURLFORM_CONTENTHEADER:
+ (void) va_arg(arg, struct curl_slist *);
+ break;
+
+ default:
+ return CURL_FORMADD_UNKNOWN_OPTION;

va_end is missing here, too, plus eight more locations in this function
I won't bother denoting.

+ }
+ }
+
+ nargs++; /* Count CURLFORM_END. */
+ va_end(arg);
+
+ /* Allocate the local curl_forms array. */
+
+ lforms = (struct curl_forms *) malloc(nargs * sizeof * lforms);
+
+ if (!lforms)
+ return CURL_FORMADD_MEMORY;
+
+ /* Fill local curl_forms array, converting strings as needed. */
+
+ va_start(arg, last_post);
+ nargs = 0;
+ contentx = -1;
+ lengthx = -1;
+ namex = -1;
+ namelengthx = -1;
+
+ for (;;) {
+ option = va_arg(arg, CURLformoption);
+
+ if (option == CURLFORM_PTRNAME)
+ option = CURLFORM_COPYNAME;
+
+ lforms[nargs].option = option;
+
+ if (option == CURLFORM_END)
+ break;
+
+ if (option == CURLFORM_CONTENTTYPE) {
+ if (Curl_formadd_convert(lforms, contentx, lengthx, contentccsid) < 0) {

I don't see why CURLFORM_CONTENTTYPE is special cased like this. What
exactly is going on here?

+ Curl_formadd_release_local(lforms, nargs, contentx);
+ return CURL_FORMADD_MEMORY;
+ }
+
+ contentx = -1;
+ lengthx = -1;
+ }
+
+ if (Curl_is_formadd_string(option)) {
+ lforms[nargs].value = va_arg(arg, char *);
+ ccsid = (unsigned int) va_arg(arg, long);
+
+ if (option == CURLFORM_COPYCONTENTS) {
+ if (contentx >= 0) {
+ Curl_formadd_release_local(lforms, nargs, contentx);
+ return CURL_FORMADD_OPTION_TWICE;
+ }
+
+ contentx = nargs;
+ contentccsid = ccsid;
+ }
+ else if (option == CURLFORM_COPYNAME) {
+ if (namex >= 0) {
+ Curl_formadd_release_local(lforms, nargs, contentx);
+ return CURL_FORMADD_OPTION_TWICE;
+ }
+
+ namex = nargs;
+ nameccsid = ccsid;
+ }
+ else if (Curl_formadd_convert(lforms, nargs, -1, ccsid) < 0) {
+ Curl_formadd_release_local(lforms, nargs, contentx);
+ return CURL_FORMADD_MEMORY;
+ }
+
+ nargs++;
+ continue;
+ }
+
+ switch (option) {
+
+ case CURLFORM_ARRAY:
+ for (forms = va_arg(arg, struct curl_forms *);
+ forms->option != CURLFORM_END; forms++) {

The code in this loop looks a lot like the code above, at the top of the
previous for loop. Some refactoring here may be in order.

+CURLcode
+curl_easy_setopt_ccsid(CURL * curl, CURLoption tag, ...)
+
+{
+ CURLcode result;
+ va_list arg;
+ struct SessionHandle * data;
+ char * s;
+ unsigned int ccsid;
+
+ data = (struct SessionHandle *) curl;
+ va_start(arg, tag);
+
+ switch (tag) {
+
+ case CURLOPT_CAINFO:
+ case CURLOPT_CAPATH:

Probably a good idea to put an assert on CURLOPT_LASTENTRY on entry to
this function to catch the case where a new option is added without
modifying this function accordingly.

+++ packages/OS400/os400sys.c 2007-08-15 17:08:41.309766400 +0200

Some of the functions in this file look like they could be useful to
other ports and are have no dependencies on curl whatsoever (e.g.
Curl_getnameinfo_a, Curl_getaddrinfo_a). Would it make sense to move
those into another source file so they could be more easily used by other
OS/400 software porters?

+static char *
+get_buffer(buffer_t * buf, long size)
+
+{
+ char * cp;
+
+ /* If `size' >= 0, make sure buffer at `buf' is at least `size'-byte long.
+ Return the buffer address. */
+
+ if (size < 0)
+ return buf->buf;
+
+ if (!buf->buf) {
+ if ((buf->buf = malloc(size)))
+ buf->size = size;
+
+ return buf->buf;
+ }
+
+ if (size <= buf->size)
+ return buf->buf;

This will be wasting space by returning too large a buffer. It might
be better to realloc the buffer in this case, too.

+
+ /* Must enlarge the buffer. */
+
+ if ((cp = realloc(buf->buf, size))) {
+ buf->buf = cp;
+ buf->size = size;
+ }
+
+ return cp;
+}

+int
+Curl_getnameinfo_a(const struct sockaddr * sa, socklen_t salen,
+ char * nodename, socklen_t nodenamelen,
+ char * servname, socklen_t servnamelen,
+ int flags)
+
+{
+ char * enodename;
+ char * eservname;
+ int status;
+ int i;
+
+ enodename = (char *) NULL;
+ eservname = (char *) NULL;
+
+ if (nodename && nodenamelen)
+ if (!(enodename = malloc(nodenamelen)))
+ return EAI_MEMORY;
+
+ if (servname && servnamelen)
+ if (!(eservname = malloc(servnamelen))) {
+ if (enodename)
+ free(enodename);
+
+ return EAI_MEMORY;
+ }
+
+ status = getnameinfo(sa, salen, enodename, nodenamelen,
+ eservname, servnamelen, flags);
+
+ if (!status) {
+ if (enodename) {
+ i = strlen(enodename) + 1;
+ QadrtConvertE2A(nodename, enodename, nodenamelen, i);
+ nodename[i] = '\0';

This looks like an off-by-one error to me. To NUL-terminate the string,
it should be writing to nodename[strlen(enodename)], not that +1.

+ }
+
+ if (eservname) {
+ i = strlen(eservname) + 1;
+ QadrtConvertE2A(servname, eservname, servnamelen, i);
+ servname[i] = '\0';

Same here.

+ }
+ }
+
+ if (enodename)
+ free(enodename);
+
+ if (eservname)
+ free(eservname);
+
+ return status;
+}
+
+
+int
+Curl_getaddrinfo_a(const char * nodename, const char * servname,
+ const struct addrinfo * hints,
+ struct addrinfo * * res)
+
+{
+ char * enodename;
+ char * eservname;
+ int status;
+ int i;
+
+ enodename = (char *) NULL;
+ eservname = (char *) NULL;
+
+ if (nodename) {
+ i = strlen(nodename) + 1;
+
+ if (!(enodename = malloc(i)))
+ return EAI_MEMORY;
+
+ QadrtConvertA2E(enodename, nodename, i, i);
+ enodename[i] = '\0';

Here it causes a guaranteed buffer overwrite.

+ }
+
+ if (servname) {
+ i = strlen(servname) + 1;
+
+ if (!(eservname = malloc(i))) {
+ if (enodename)
+ free(enodename);
+
+ return EAI_MEMORY;
+ }
+
+ QadrtConvertA2E(eservname, servname, i, i);
+ eservname[i] = '\0';

And here.

+ }
+
+ status = getaddrinfo(enodename, eservname, hints, res);
+
+ if (enodename)
+ free(enodename);
+
+ if (eservname)
+ free(eservname);
+
+ return status;
+}
+

+OM_uint32
+Curl_gss_init_sec_context_a(OM_uint32 * minor_status, gss_cred_id_t cred_handle,
+ gss_ctx_id_t * context_handle,
+ gss_name_t target_name, gss_OID mech_type,
+ gss_flags_t req_flags, OM_uint32 time_req,
+ gss_channel_bindings_t input_chan_bindings,
+ gss_buffer_t input_token,
+ gss_OID * actual_mech_type,
+ gss_buffer_t output_token, gss_flags_t * ret_flags,
+ OM_uint32 * time_rec)
+
+{
+ int rc;
+ unsigned int i;
+ gss_buffer_desc in;
+ gss_buffer_t inp;
+ char * t;
+
+ in.value = NULL;
+
+ if ((inp = input_token))
+ if (inp->length && inp->value) {
+ i = inp->length;
+
+ if (!(in.value = malloc(i + 1))) {
+ if (minor_status)
+ *minor_status = ENOMEM;
+
+ return GSS_S_FAILURE;
+ }
+
+ QadrtConvertA2E(in.value, input_token->value, i, i);
+ ((char *) in.value)[i] = '\0';
+ in.length = i;
+ inp = &in;
+ }
+
+ rc = gss_init_sec_context(minor_status, cred_handle, context_handle,
+ target_name, mech_type, req_flags, time_req,
+ input_chan_bindings, inp, actual_mech_type,
+ output_token, ret_flags, time_rec);
+
+ if (in.value)
+ free(in.value);
+
+ if (rc != GSS_S_COMPLETE || !output_token ||
+ !output_token->length || !output_token->value)
+ return rc;
+
+ i = output_token->length;
+
+ if (!(t = malloc(i + 1))) {
+ gss_release_buffer(minor_status, output_token);
+
+ if (minor_status)
+ *minor_status = ENOMEM;
+
+ return GSS_S_FAILURE;
+ }
+
+ QadrtConvertE2A(t, output_token->value, i, i);
+ t[i] = '\0';
+ gss_release_buffer(minor_status, output_token);

I have no idea how the GSS library works, but the way this code works
seems dangerous. It makes assumptions about the internals of
gss_release_buffer and what it releases. It seems safer to me to copy the
converted string into the original buffer returned by gss_init_sec_context
instead of calling gss_release_buffer and doing our own allocation (as is
done in Curl_ldap_get_dn_a). This goes for the other GSS wrappers, too.

+ output_token->value = t;
+ output_token->length = i;
+ return rc;
+}
+

+/* ASCII wrappers for the LDAP procedures. */
+
+void *
+Curl_ldap_init_a(char * host, int port)
+
+{
+ unsigned int i;
+ char * ehost;
+ void * result;
+
+ if (!host)
+ return (void *) ldap_init(host, port);
+
+ i = strlen(host) + 1;
+
+ if (!(ehost = malloc(i)))
+ return (void *) NULL;
+
+ QadrtConvertA2E(ehost, host, i, i);
+ ehost[i] = '\0';

Write past end of buffer.

+ result = (void *) ldap_init(ehost, port);
+ free(ehost);
+ return result;
+}
+

+
+int
+Curl_ldap_simple_bind_s_a(void * ld, char * dn, char * passwd)
+
+{
+ int i;
+ char * edn;
+ char * epasswd;
+
+ edn = (char *) NULL;
+ epasswd = (char *) NULL;
+
+ if (dn) {
+ i = strlen(dn) + 1;
+
+ if (!(edn = malloc(i)))
+ return LDAP_NO_MEMORY;
+
+ QadrtConvertA2E(edn, dn, i, i);
+ edn[i] = '\0';

Write past end of buffer.

+ }
+
+ if (passwd) {
+ i = strlen(passwd) + 1;
+
+ if (!(epasswd = malloc(i))) {
+ if (edn)
+ free(edn);
+
+ return LDAP_NO_MEMORY;
+ }
+
+ QadrtConvertA2E(epasswd, passwd, i, i);
+ epasswd[i] = '\0';

Write past end of buffer.

+ }
+
+ i = ldap_simple_bind_s(ld, edn, epasswd);
+
+ if (epasswd)
+ free(epasswd);
+
+ if (edn)
+ free(edn);
+
+ return i;
+}
+
+

+int
+Curl_ldap_search_s_a(void * ld, char * base, int scope, char * filter,
+ char * * attrs, int attrsonly, LDAPMessage * * res)
+
+{
+ int i;
+ int j;
+ char * ebase;
+ char * efilter;
+ char * * eattrs;
+ int status;
+
+ ebase = (char *) NULL;
+ efilter = (char *) NULL;
+ eattrs = (char * *) NULL;
+ status = LDAP_SUCCESS;
+
+ if (base) {
+ i = strlen(base) + 1;
+
+ if (!(ebase = malloc(i)))
+ status = LDAP_NO_MEMORY;
+ else {
+ QadrtConvertA2E(ebase, base, i, i);
+ ebase[i] = '\0';

Write past end of buffer.

+ }
+ }
+
+ if (filter && status == LDAP_SUCCESS) {
+ i = strlen(filter) + 1;
+
+ if (!(efilter = malloc(i)))
+ status = LDAP_NO_MEMORY;
+ else {
+ QadrtConvertA2E(efilter, filter, i, i);
+ efilter[i] = '\0';

Write past end of buffer.

+ }
+ }
+
+ if (attrs && status == LDAP_SUCCESS) {
+ for (i = 0; attrs[i++];)
+ ;
+
+ if (!(eattrs = (char * *) malloc(i * sizeof *eattrs)))
+ status = LDAP_NO_MEMORY;
+ else {
+ memset((char *) eattrs, 0, i * sizeof *eattrs);

Could use a calloc above and avoid this memset.

+
+ for (j = 0; attrs[j]; j++) {
+ i = strlen(attrs[j]) + 1;
+
+ if (!(eattrs[j] = malloc(i))) {
+ status = LDAP_NO_MEMORY;
+ break;
+ }
+
+ QadrtConvertA2E(eattrs[j], attrs[j], i, i);
+ eattrs[j][i] = '\0';

Write past end of buffer.

+ }
+ }
+ }
+
+ if (status == LDAP_SUCCESS)
+ status = ldap_search_s(ld, ebase? ebase: "", scope,
+ efilter? efilter: "(objectclass=*)",
+ eattrs, attrsonly, res);
+
+ if (eattrs) {
+ for (j = 0; eattrs[j]; j++)
+ free(eattrs[j]);
+
+ free(eattrs);
+ }
+
+ if (efilter)
+ free(efilter);
+
+ if (ebase)
+ free(ebase);
+
+ return status;
+}
+
+

+struct berval * *
+Curl_ldap_get_values_len_a(void * ld, LDAPMessage * entry, const char * attr)
+
+{
+ int i;
+ char * cp;
+ struct berval * * result;
+
+ cp = (char *) NULL;
+
+ if (attr) {
+ i = strlen(attr) + 1;
+
+ if (!(cp = malloc(i))) {
+ ldap_set_lderrno(ld, LDAP_NO_MEMORY, NULL,
+ ldap_err2string(LDAP_NO_MEMORY));
+ return (struct berval * *) NULL;
+ }
+
+ QadrtConvertA2E(cp, attr, i, i);
+ cp[i] = '\0';

Write past end of buffer.

+ }
+
+ result = ldap_get_values_len(ld, entry, cp);
+
+ if (cp)
+ free(cp);
+
+ /* Result data are binary in nature, so they haven't been converted to EBCDIC.
+ Therefore do not convert. */
+
+ return result;
+}

>>> Dan

-- 
http://www.MoveAnnouncer.com              The web change of address service
          Let webmasters know that your web site has moved