Hi Daniel, >> following the comment at the beginning of security.c I gave a try at >> rewriting the file. The diff against trunk is attached along with the new >> implementation. It passes all the tests on my machine but the compilation >> has not been tested outside a Linux/32bit machine. The diff is pretty big >> and can be split upon demand to ease review. > > Whoa, very cool work! Sorry for being slow at responding, it slipped between > somehow.
Thanks, I have been pretty busy too and haven't pinged about that too. > I fear that security.c is mostly used for kerberos4 and possibly some gssapi > stuff and I must admit that I have _never_ used any of those since the days > we first introduced krb4 (when I was given a krb account to borrow for a few > days) so if we go this route I think we just need to trust that the tests > are decent enough to at least not break everything completely and then wish > and hope that someone who actually uses krb4 or gss will try it. I don't have a kerberos account and I just went ahead with the tests and trying to match the old code to keep the behaviour close enough. > The original code that the copyright covers was first modified quite a lot > by Martin Hedenfalk to adapt it to curl, and then I did my share at > curlifying it. That was even before the file first appeared in the directory > as the first version we can find with git (September 2000). From there, the > file has been further modified. Now you've modified it a lot on top of all > this. Is there any traces left that would warrant a copyright and thus a say > in which license to use? The code is inspired in its structure and some of its algorithm from the previous one. Also looking at the original code, it looks like some of it is still around. > In all fairness, however much I'd like to just get out of that annoying > announce license, I can spot similarities in the patched code and the > original code we imported into curl. They are close enough to be seen by a > human eye looking for it. Are they big enough to warrant copyright? I don't > know, but in this case I rather leave the copyright in just to be safe and > play nice. I think as long as you integrate one line of a previous change, it may be considered as a derivative work and thus be submitted to the same license. It may be difficult to prove that a one-liner is included though so usually the threshold is higher (function, algorithm). > But, given that you've worked a lot on this and fixed a bunch of issues and > quirks in the code, I think we should proceed and merge your patch even if > it doesn't (yet) remove the Original BSD license from the file. > > What do you think? Am I wrong? It seems like a good middle ground and the wisest decision considering our lack of better legal knowledge. I was hoping to get some insights from fellow hackers on this license issue (and I have thanks!). My understanding of a rewrite was that it needed to be done in a block. If we don't go this way, I would rather split the huge patch into different components so that they can get better reviews. It will also be easier to spot regressions that may arise. Please find attached the first split. I will do the rest over the week-end. Regards, Julien
From ccc3e1121dee39a9d796baf700b2a2b86d9c9489 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Thu, 9 Sep 2010 23:52:49 -0700 Subject: [PATCH 1/5] Security.c: Fix headers guard to match the rest of the code. --- lib/security.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/security.c b/lib/security.c index eceb013..4f5393d 100644 --- a/lib/security.c +++ b/lib/security.c @@ -92,10 +92,10 @@ name_to_level(const char *name) } static const struct Curl_sec_client_mech * const mechs[] = { -#ifdef HAVE_GSSAPI +#if defined(HAVE_GSSAPI) &Curl_krb5_client_mech, #endif -#ifdef HAVE_KRB4 +#if defined(HAVE_KRB4) &Curl_krb4_client_mech, #endif NULL @@ -496,5 +496,6 @@ Curl_sec_end(struct connectdata *conn) conn->mech=NULL; } -#endif /* HAVE_KRB4 */ +#endif /* HAVE_KRB4 || HAVE_GSSAPI */ + #endif /* CURL_DISABLE_FTP */ -- 1.7.0.4
From c301d33dc7a5de122f87814edeb823ab6bf06bc4 Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Fri, 10 Sep 2010 00:07:09 -0700 Subject: [PATCH 2/5] security.c: Made block_read and sec_get_data return CURLcode. To do so, made block_read call Curl_read_plain instead of read. While changing them renamed block_read to socket_read and sec_get_data to read_data to better match their function. Also fixed a potential memory leak in block_read. --- lib/security.c | 75 +++++++++++++++++++++++++++++-------------------------- 1 files changed, 40 insertions(+), 35 deletions(-) diff --git a/lib/security.c b/lib/security.c index 4f5393d..9523642 100644 --- a/lib/security.c +++ b/lib/security.c @@ -101,24 +101,30 @@ static const struct Curl_sec_client_mech * const mechs[] = { NULL }; -static int -block_read(int fd, void *buf, size_t len) + +/* Read |len| from the socket |fd| and store it in |to|. Return a + CURLcode saying whether an error occured or CURLE_OK if |len| was read. */ +static CURLcode +socket_read(curl_socket_t fd, void *to, size_t len) { - unsigned char *p = buf; - int b; - while(len) { - b = read(fd, p, len); - if(b == 0) - return 0; - else if(b < 0 && (errno == EINTR || errno == EAGAIN)) - /* TODO: this will busy loop in the EAGAIN case */ - continue; - else if(b < 0) - return -1; - len -= b; - p += b; + char *to_p = to; + CURLcode code; + ssize_t nread; + + while(len > 0) { + code = Curl_read_plain(fd, to_p, len, &nread); + if(code == CURLE_OK) { + len -= nread; + to_p += nread; + } + else { + /* FIXME: We are doing a busy wait */ + if(code == CURLE_AGAIN) + continue; + return code; + } } - return p - (unsigned char*)buf; + return CURLE_OK; } static int @@ -138,31 +144,30 @@ block_write(int fd, const void *buf, size_t len) return p - (unsigned char*)buf; } -static int -sec_get_data(struct connectdata *conn, - int fd, struct krb4buffer *buf) +static CURLcode read_data(struct connectdata *conn, + curl_socket_t fd, + struct krb4buffer *buf) { int len; - int b; + void* tmp; + CURLcode ret; + + ret = socket_read(fd, &len, sizeof(len)); + if (ret != CURLE_OK) + return ret; - b = block_read(fd, &len, sizeof(len)); - if(b == 0) - return 0; - else if(b < 0) - return -1; len = ntohl(len); - /* TODO: This realloc will cause a memory leak in an out of memory - * condition */ - buf->data = realloc(buf->data, len); - b = buf->data ? block_read(fd, buf->data, len) : -1; - if(b == 0) - return 0; - else if(b < 0) - return -1; + tmp = realloc(buf->data, len); + if (tmp == NULL) + return CURLE_OUT_OF_MEMORY; + + ret = socket_read(fd, buf->data, len); + if (ret != CURLE_OK) + return ret; buf->size = (conn->mech->decode)(conn->app_data, buf->data, len, conn->data_prot, conn); buf->index = 0; - return 0; + return CURLE_OK; } static size_t @@ -195,7 +200,7 @@ static ssize_t sec_read(struct connectdata *conn, int num, buffer = (char*)buffer + len; while(length) { - if(sec_get_data(conn, fd, &conn->in_buffer) < 0) + if(read_data(conn, fd, &conn->in_buffer) != CURLE_OK) return -1; if(conn->in_buffer.size == 0) { if(rx) -- 1.7.0.4
From 134f19f11b1d510f9795f9313a629135255261cd Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Fri, 10 Sep 2010 00:17:17 -0700 Subject: [PATCH 3/5] security.c: Made block_write return a CURLcode. While doing so, renamed it to socket_write to better match its function. --- lib/security.c | 47 ++++++++++++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 19 deletions(-) diff --git a/lib/security.c b/lib/security.c index 9523642..e9f8ea0 100644 --- a/lib/security.c +++ b/lib/security.c @@ -127,21 +127,30 @@ socket_read(curl_socket_t fd, void *to, size_t len) return CURLE_OK; } -static int -block_write(int fd, const void *buf, size_t len) + +/* Write |len| bytes from the buffer |to| to the socket |fd|. Return a + CURLcode saying whether an error occured or CURLE_OK if |len| was written. */ +static CURLcode +socket_write(struct connectdata *conn, curl_socket_t fd, const void *to, size_t len) { - const unsigned char *p = buf; - int b; - while(len) { - b = write(fd, p, len); - if(b < 0 && (errno == EINTR || errno == EAGAIN)) - continue; - else if(b < 0) - return -1; - len -= b; - p += b; + const char *to_p = to; + CURLcode code; + ssize_t written; + + while(len > 0) { + code = Curl_write_plain(conn, fd, to_p, len, &written); + if(code == CURLE_OK) { + len -= written; + to_p += written; + } + else { + /* FIXME: We are doing a busy wait */ + if(code == CURLE_AGAIN) + continue; + return code; + } } - return p - (unsigned char*)buf; + return CURLE_OK; } static CURLcode read_data(struct connectdata *conn, @@ -237,11 +246,11 @@ sec_send(struct connectdata *conn, int fd, const char *from, int length) bytes = Curl_base64_encode(conn->data, (char *)buf, bytes, &cmdbuf); if(bytes > 0) { if(protlevel == prot_private) - block_write(fd, "ENC ", 4); + socket_write(conn, fd, "ENC ", 4); else - block_write(fd, "MIC ", 4); - block_write(fd, cmdbuf, bytes); - block_write(fd, "\r\n", 2); + socket_write(conn, fd, "MIC ", 4); + socket_write(conn, fd, cmdbuf, bytes); + socket_write(conn, fd, "\r\n", 2); Curl_infof(conn->data, "%s %s\n", protlevel == prot_private ? "ENC" : "MIC", cmdbuf); free(cmdbuf); @@ -249,8 +258,8 @@ sec_send(struct connectdata *conn, int fd, const char *from, int length) } else { bytes = htonl(bytes); - block_write(fd, &bytes, sizeof(bytes)); - block_write(fd, buf, ntohl(bytes)); + socket_write(conn, fd, &bytes, sizeof(bytes)); + socket_write(conn, fd, buf, ntohl(bytes)); } free(buf); return length; -- 1.7.0.4
From f2708ab5e1f08a2aa2f525e50b577773e192f6cd Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Fri, 10 Sep 2010 00:22:40 -0700 Subject: [PATCH 4/5] security.c: buffer_read various fixes. Tighten the type of the |data| parameter to avoid a cast. Also made it const as we should not modify it. Added a DEBUGASSERT on the size to be written while changing it. --- lib/security.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/security.c b/lib/security.c index e9f8ea0..c79128a 100644 --- a/lib/security.c +++ b/lib/security.c @@ -180,11 +180,13 @@ static CURLcode read_data(struct connectdata *conn, } static size_t -buffer_read(struct krb4buffer *buf, void *data, size_t len) +buffer_read(struct krb4buffer *buf, const char *data, size_t len) { - if(buf->size - buf->index < len) - len = buf->size - buf->index; - memcpy(data, (char*)buf->data + buf->index, len); + size_t buf_capacity = buf->size - buf->index; + DEBUGASSERT(buf->size > buf->index); + if(buf_capacity < len) + len = buf_capacity; + memcpy(buf, data, len); buf->index += len; return len; } -- 1.7.0.4
From fbf68844ea0c6726901e11290703ed4079e3782c Mon Sep 17 00:00:00 2001 From: Julien Chaffraix <[email protected]> Date: Fri, 10 Sep 2010 00:26:37 -0700 Subject: [PATCH 5/5] security.c: Remove out_buffer as it was never written into. --- lib/security.c | 4 ---- lib/urldata.h | 2 +- 2 files changed, 1 insertions(+), 5 deletions(-) diff --git a/lib/security.c b/lib/security.c index c79128a..3a3f0e0 100644 --- a/lib/security.c +++ b/lib/security.c @@ -291,10 +291,6 @@ int Curl_sec_fflush_fd(struct connectdata *conn, int fd) { if(conn->data_prot != prot_clear) { - if(conn->out_buffer.index > 0){ - sec_write(conn, fd, conn->out_buffer.data, conn->out_buffer.index); - conn->out_buffer.index = 0; - } sec_send(conn, fd, NULL, 0); } return 0; diff --git a/lib/urldata.h b/lib/urldata.h index 9369dd8..4d60591 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -806,7 +806,7 @@ struct connectdata { enum protection_level data_prot; enum protection_level request_data_prot; size_t buffer_size; - struct krb4buffer in_buffer, out_buffer; + struct krb4buffer in_buffer; void *app_data; const struct Curl_sec_client_mech *mech; struct sockaddr_in local_addr; -- 1.7.0.4
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
