tabled: make time2str reentrant

PATCH In review Pete Zaitcev 2 participants 4 messages Jan 3, 2010 Library code

Share & tools

Pete Zaitcev

author

2010-01-03 07:56 UTC #0 lore raw

PATCH 1/3 tabled: make time2str reentrant

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 <[email protected]>

---
 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 <[email protected]>
Date:   Sat Jan 2 23:36:11 2010 -0700

Fix time2str to be reentrant and safer.

include/httputil.h +1 -1

index 7a1f6da..b2e8f30 100644
@@ -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);

lib/httpstor.c +6 -6

index f69a317..a502858 100644
@@ -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);

lib/httputil.c +12 -3

index c953da0..1c56be9 100644
@@ -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;
 }

server/bucket.c +4 -4

index cefd2e2..690da79 100644
@@ -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,

server/object.c +6 -5

index 6bda7a6..103b36e 100644
@@ -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;

server/server.c +8 -6

index fce038d..32bb8a4 100644
@@ -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;

server/tabled.h +0 -1

index 290edaf..59c474a 100644
@@ -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);

Jeff Garzik

2010-01-03 08:27 UTC #1 lore raw

Quoted 85 lines · 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<[email protected]>

---
  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<[email protected]>
Date:   Sat Jan 2 23:36:11 2010 -0700

Fix time2str to be reentrant and safer.

include/httputil.h +1 -1

index 7a1f6da..b2e8f30 100644
@@ -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);

lib/httpstor.c +5 -5

index f69a317..a502858 100644
@@ -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);

lib/httpstor.c struct httpstor_keylist *httpstor_keys(struct httpstor_client *httpstor, const c · line 743

Quoted 5 lines

lib/httpstor.c +1 -1

@@ -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

Pete Zaitcev

author

2010-01-03 08:34 UTC #2 lore raw

Quoted 6 lines · On Sun, 03 Jan 2010 03:27:59 -0500 Jeff Garzik <[email protected]> wrote:

Review
Quoted 2 lines
(quoted diff) +1 -1
-	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

Jeff Garzik

2010-01-03 21:27 UTC #3 lore raw

Quoted 11 lines · Pete Zaitcev

Quoted 6 lines · On Sun, 03 Jan 2010 03:27:59 -0500 Jeff Garzik<[email protected]> wrote:
Review
Quoted 2 lines
(quoted diff) +1 -1
-	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