[Patch 1/3] tabled: make time2str reentrant

[Patch 1/3] tabled: make time2str reentrant

[Pete Zaitcev]

The main point here is to kill gmtime, but since we're at it, may as well
fix the API and add safety (observe, that not all timestr arguments were
64 bytes long in the original code).

Signed-Off-By: Pete Zaitcev <zaitcev@redhat.com>

---
 include/httputil.h |    2 +-
 lib/httpstor.c     |   12 ++++++------
 lib/httputil.c     |   15 ++++++++++++---
 server/bucket.c    |    8 ++++----
 server/object.c    |   11 ++++++-----
 server/server.c    |   14 ++++++++------
 server/tabled.h    |    1 -
 7 files changed, 37 insertions(+), 26 deletions(-)

commit d180799fbd1eedc32eabc4e2335950714bd26b65
Author: Master <zaitcev@niphredil.zaitcev.lan>
Date:   Sat Jan 2 23:36:11 2010 -0700

    Fix time2str to be reentrant and safer.

diff --git a/include/httputil.h b/include/httputil.h
index 7a1f6da..b2e8f30 100644
--- a/include/httputil.h
+++ b/include/httputil.h
@@ -85,7 +85,7 @@ enum ReqACLC {
 };
 
 /* httputil.c */
-extern char *time2str(char *strbuf, time_t time);
+extern char *time2str(char *buf, int len, time_t time);
 extern time_t str2time(const char *timestr);
 extern int req_hdr_push(struct http_req *req, char *key, char *val);
 extern char *req_hdr(struct http_req *req, const char *key);
diff --git a/lib/httpstor.c b/lib/httpstor.c
index f69a317..a502858 100644
--- a/lib/httpstor.c
+++ b/lib/httpstor.c
@@ -196,7 +196,7 @@ struct httpstor_blist *httpstor_list_buckets(struct httpstor_client *httpstor)
 	req.method = "GET";
 	req.orig_path = "/";
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
@@ -315,7 +315,7 @@ static bool __httpstor_ad_bucket(struct httpstor_client *httpstor, const char *n
 	req.method = delete ? "DELETE" : "PUT";
 	req.orig_path = orig_path;
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
@@ -374,7 +374,7 @@ bool httpstor_get(struct httpstor_client *httpstor, const char *bucket, const ch
 	req.method = "GET";
 	req.orig_path = orig_path;
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
@@ -453,7 +453,7 @@ bool httpstor_put(struct httpstor_client *httpstor, const char *bucket, const ch
 	req.method = "PUT";
 	req.orig_path = orig_path;
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
@@ -544,7 +544,7 @@ bool httpstor_del(struct httpstor_client *httpstor, const char *bucket, const ch
 	req.method = "DELETE";
 	req.orig_path = orig_path;
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
@@ -740,7 +740,7 @@ struct httpstor_keylist *httpstor_keys(struct httpstor_client *httpstor, const c
 	req.method = "GET";
 	req.orig_path = orig_path;
 
-	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
+	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
 
 	req_hdr_push(&req, "Date", timestr);
 
diff --git a/lib/httputil.c b/lib/httputil.c
index c953da0..1c56be9 100644
--- a/lib/httputil.c
+++ b/lib/httputil.c
@@ -40,10 +40,19 @@ time_t str2time(const char *timestr)
 	return mktime(&tm);
 }
 
-char *time2str(char *strbuf, time_t src_time)
+char *time2str(char *strbuf, int buflen, time_t src_time)
 {
-	struct tm *tm = gmtime(&src_time);
-	strftime(strbuf, 64, "%a, %d %b %Y %H:%M:%S %z", tm);
+	struct tm tm;
+	size_t rc;
+
+	if (buflen <= 0)
+		return NULL;	/* too wrong, better crash right away. */
+	gmtime_r(&src_time, &tm);
+	rc = strftime(strbuf, buflen, "%a, %d %b %Y %H:%M:%S %z", &tm);
+	if (rc >= buflen)
+		strbuf[buflen-1] = 0;
+	else if (rc == 0)
+		strbuf[0] = 0;
 	return strbuf;
 }
 
diff --git a/server/bucket.c b/server/bucket.c
index cefd2e2..690da79 100644
--- a/server/bucket.c
+++ b/server/bucket.c
@@ -265,7 +265,7 @@ bool service_list(struct client *cli, const char *user)
                         "    </Bucket>\r\n",
 
 			     ent->name,
-			     time2str(timestr,
+			     time2str(timestr, sizeof(timestr),
 			     	      GUINT64_FROM_LE(ent->time_create)));
 		if (!s)
 			goto err_out_content;
@@ -547,7 +547,7 @@ bool bucket_add(struct client *cli, const char *user, const char *bucket)
 "\r\n",
 		     cli->req.major,
 		     cli->req.minor,
-		     time2str(timestr, time(NULL)),
+		     time2str(timestr, sizeof(timestr), time(NULL)),
 		     bucket) < 0)
 		return cli_err(cli, InternalError);
 
@@ -700,7 +700,7 @@ bool bucket_del(struct client *cli, const char *user, const char *bucket)
 "\r\n",
 		     cli->req.major,
 		     cli->req.minor,
-		     time2str(timestr, time(NULL))) < 0)
+		     time2str(timestr, sizeof(timestr), time(NULL))) < 0)
 		return cli_err(cli, InternalError);
 
 	rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
@@ -1093,7 +1093,7 @@ static bool bucket_list_keys(struct client *cli, const char *user,
                          "  </Contents>\r\n",
 
 			 vp->key,
-			 time2str(timestr, vp->mtime / 1000000),
+			 time2str(timestr, sizeof(timestr), vp->mtime / 1000000),
 			 vp->md5,
 			 (unsigned long long) vp->size,
 			 user,
diff --git a/server/object.c b/server/object.c
index 6bda7a6..103b36e 100644
--- a/server/object.c
+++ b/server/object.c
@@ -224,7 +224,7 @@ bool object_del(struct client *cli, const char *user,
 "\r\n",
 		     cli->req.major,
 		     cli->req.minor,
-		     time2str(timestr, time(NULL))) < 0)
+		     time2str(timestr, sizeof(timestr), time(NULL))) < 0)
 		return cli_err(cli, InternalError);
 
 	rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
@@ -515,7 +515,7 @@ static bool object_put_end(struct client *cli)
 		     cli->req.major,
 		     cli->req.minor,
 		     md5,
-		     time2str(timestr, time(NULL))) < 0) {
+		     time2str(timestr, sizeof(timestr), time(NULL))) < 0) {
 		/* FIXME: cleanup failure */
 		applog(LOG_ERR, "OOM in object_put_end");
 		return cli_err(cli, InternalError);
@@ -928,7 +928,7 @@ static bool object_put_acls(struct client *cli, const char *user,
 "\r\n",
 		     cli->req.major,
 		     cli->req.minor,
-		     time2str(timestr, time(NULL))) < 0) {
+		     time2str(timestr, sizeof(timestr), time(NULL))) < 0) {
 		/* FIXME: cleanup failure */
 		applog(LOG_ERR, "OOM in object_put_end");
 		return cli_err(cli, InternalError);
@@ -1263,8 +1263,9 @@ bool object_get_body(struct client *cli, const char *user, const char *bucket,
 		     modified ? 200 : 304,
 		     (unsigned long long) GUINT64_FROM_LE(obj->size),
 		     md5,
-		     time2str(timestr, time(NULL)),
-		     time2str(modstr, GUINT64_FROM_LE(obj->mtime) / 1000000),
+		     time2str(timestr, sizeof(timestr), time(NULL)),
+		     time2str(modstr, sizeof(modstr),
+			      GUINT64_FROM_LE(obj->mtime) / 1000000),
 		     extra_hdr->str) < 0)
 		goto err_out_in_end;
 
diff --git a/server/server.c b/server/server.c
index fce038d..32bb8a4 100644
--- a/server/server.c
+++ b/server/server.c
@@ -698,7 +698,7 @@ bool cli_err(struct client *cli, enum errcode code)
 		 * FIXME '*' for URI is bogus. We keep this until we know
 		 * how to test this code path.
 		 */
-		if (asprintf(&hdr,
+		rc = asprintf(&hdr,
 			"HTTP/%d.%d %d x\r\n"
 			"Content-Type: application/xml\r\n"
 			"Content-Length: %zu\r\n"
@@ -711,13 +711,14 @@ bool cli_err(struct client *cli, enum errcode code)
 			     cli->req.minor,
 			     err_info[code].status,
 			     strlen(content),
-			     time2str(timestr, time(NULL)),
-			     "*") < 0) {
+			     time2str(timestr, sizeof(timestr), time(NULL)),
+			     "*");
+		if (rc < 0) {
 			free(content);
 			return false;
 		}
 	} else {
-		if (asprintf(&hdr,
+		rc = asprintf(&hdr,
 			"HTTP/%d.%d %d x\r\n"
 			"Content-Type: application/xml\r\n"
 			"Content-Length: %zu\r\n"
@@ -729,7 +730,8 @@ bool cli_err(struct client *cli, enum errcode code)
 			     cli->req.minor,
 			     err_info[code].status,
 			     strlen(content),
-			     time2str(timestr, time(NULL))) < 0) {
+			     time2str(timestr, sizeof(timestr), time(NULL)));
+		if (rc < 0) {
 			free(content);
 			return false;
 		}
@@ -766,7 +768,7 @@ bool cli_resp_xml(struct client *cli, int http_status,
 		     cli->req.minor,
 		     http_status,
 		     strlist_len(content),
-		     time2str(timestr, time(NULL)),
+		     time2str(timestr, sizeof(timestr), time(NULL)),
 		     cxn_close ? "Connection: close\r\n" : "") < 0) {
 		__strlist_free(content);
 		return false;
diff --git a/server/tabled.h b/server/tabled.h
index 290edaf..59c474a 100644
--- a/server/tabled.h
+++ b/server/tabled.h
@@ -300,7 +300,6 @@ extern void applogerr(const char *prefix);
 extern void strup(char *s);
 extern int write_pid_file(const char *pid_fn);
 extern int fsetflags(const char *prefix, int fd, int or_flags);
-extern char *time2str(char *strbuf, time_t time);
 extern void md5str(const unsigned char *digest, char *outstr);
 extern void req_sign(struct http_req *req, const char *bucket, const char *key,
 	      char *b64hmac_out);

Re: [Patch 1/3] tabled: make time2str reentrant

[Jeff Garzik]

On 01/03/2010 02:56 AM, Pete Zaitcev wrote:
> The main point here is to kill gmtime, but since we're at it, may as well
> fix the API and add safety (observe, that not all timestr arguments were
> 64 bytes long in the original code).
>
> Signed-Off-By: Pete Zaitcev<zaitcev@redhat.com>
>
> ---
>   include/httputil.h |    2 +-
>   lib/httpstor.c     |   12 ++++++------
>   lib/httputil.c     |   15 ++++++++++++---
>   server/bucket.c    |    8 ++++----
>   server/object.c    |   11 ++++++-----
>   server/server.c    |   14 ++++++++------
>   server/tabled.h    |    1 -
>   7 files changed, 37 insertions(+), 26 deletions(-)
>
> commit d180799fbd1eedc32eabc4e2335950714bd26b65
> Author: Master<zaitcev@niphredil.zaitcev.lan>
> Date:   Sat Jan 2 23:36:11 2010 -0700
>
>      Fix time2str to be reentrant and safer.
>
> diff --git a/include/httputil.h b/include/httputil.h
> index 7a1f6da..b2e8f30 100644
> --- a/include/httputil.h
> +++ b/include/httputil.h
> @@ -85,7 +85,7 @@ enum ReqACLC {
>   };
>
>   /* httputil.c */
> -extern char *time2str(char *strbuf, time_t time);
> +extern char *time2str(char *buf, int len, time_t time);
>   extern time_t str2time(const char *timestr);
>   extern int req_hdr_push(struct http_req *req, char *key, char *val);
>   extern char *req_hdr(struct http_req *req, const char *key);
> diff --git a/lib/httpstor.c b/lib/httpstor.c
> index f69a317..a502858 100644
> --- a/lib/httpstor.c
> +++ b/lib/httpstor.c
> @@ -196,7 +196,7 @@ struct httpstor_blist *httpstor_list_buckets(struct httpstor_client *httpstor)
>   	req.method = "GET";
>   	req.orig_path = "/";
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>
>   	req_hdr_push(&req, "Date", timestr);
>
> @@ -315,7 +315,7 @@ static bool __httpstor_ad_bucket(struct httpstor_client *httpstor, const char *n
>   	req.method = delete ? "DELETE" : "PUT";
>   	req.orig_path = orig_path;
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>
>   	req_hdr_push(&req, "Date", timestr);
>
> @@ -374,7 +374,7 @@ bool httpstor_get(struct httpstor_client *httpstor, const char *bucket, const ch
>   	req.method = "GET";
>   	req.orig_path = orig_path;
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>
>   	req_hdr_push(&req, "Date", timestr);
>
> @@ -453,7 +453,7 @@ bool httpstor_put(struct httpstor_client *httpstor, const char *bucket, const ch
>   	req.method = "PUT";
>   	req.orig_path = orig_path;
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>
>   	req_hdr_push(&req, "Date", timestr);
>
> @@ -544,7 +544,7 @@ bool httpstor_del(struct httpstor_client *httpstor, const char *bucket, const ch
>   	req.method = "DELETE";
>   	req.orig_path = orig_path;
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>
>   	req_hdr_push(&req, "Date", timestr);
>
> @@ -740,7 +740,7 @@ struct httpstor_keylist *httpstor_keys(struct httpstor_client *httpstor, const c
>   	req.method = "GET";
>   	req.orig_path = orig_path;
>
> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));

applied 1-3, and then added sizeof() to the above time2str calls...  The 
hardcoded sizes I used for the strings is ugly enough...  let's not add 
additional "naked numbers" on top of that.

	Jeff

Re: [Patch 1/3] tabled: make time2str reentrant

[Pete Zaitcev]

On Sun, 03 Jan 2010 03:27:59 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> > -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
> > +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
> 
> applied 1-3, and then added sizeof() to the above time2str calls...  The 
> hardcoded sizes I used for the strings is ugly enough...  let's not add 
> additional "naked numbers" on top of that.

I thought wrapping was worse. Sure...

-- Pete

Re: [Patch 1/3] tabled: make time2str reentrant

[Jeff Garzik]

On 01/03/2010 03:34 AM, Pete Zaitcev wrote:
> On Sun, 03 Jan 2010 03:27:59 -0500
> Jeff Garzik<jeff@garzik.org>  wrote:
>
>>> -	sprintf(datestr, "Date: %s", time2str(timestr, time(NULL)));
>>> +	sprintf(datestr, "Date: %s", time2str(timestr, 64, time(NULL)));
>>
>> applied 1-3, and then added sizeof() to the above time2str calls...  The
>> hardcoded sizes I used for the strings is ugly enough...  let's not add
>> additional "naked numbers" on top of that.
>
> I thought wrapping was worse. Sure...

Cosmetically annoying, yes.  But hardcoding string length in multiple 
locations increases the chances of an actual bug, so...

	Jeff