Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

[Jim Meyering <jim@meyering.net>]

Jim Meyering wrote:
> Jeff Garzik wrote:
>
>> On 09/23/2010 03:19 PM, Jeff Garzik wrote:
>>> 3) I process patches similar to how Linus and others in the kernel do
>>> it: "git am /path/to/mbox_of_patches" That tends to impose some
>>> restrictions on the contents of each email.
>>
>> FWIW, 'git pull' submissions are welcome.  Standard kernel-style pull
>> submission style applies[1].
>>
>> 	Jeff
>>
>> [1] public git pull URL including branch name, diffstat, shortlog or
>> full log of changeset summaries, and finally, the combined diff of all
>> changes.
>
> Here you go.
> You can pull from the "oom" branch here:
>   git://git.infradead.org/users/meyering/tabled.git
>
> I think I've addressed all of your preferences, merging
> most OOM fixes into one commit, but not the two you mentioned
> that should stay separate.  I also left the sizeof(s) one separate.
>
> However, I did leave the copyright year updates in.
> If they're a problem, let me know and I'll do another round.
> Otherwise, I can send a patch to update all of the
> remaining ones to include 2010 so this won't be an
> issue for 3 more months.
>
> $ git shortlog HEAD ^origin/master
> Jim Meyering (4):
>       server/server.c: use sizeof(s) rather than equivalent "64"
>       don't dereference NULL on OOM
>       server/status.c: don't deref NULL on failed strdup
>       server/bucket.c: don't deref NULL upon failed malloc
>
>  b/server/bucket.c  |   25 ++++++++++++++++---------
>  b/server/config.c  |    7 +++++--
>  b/server/object.c  |    7 ++++++-
>  b/server/replica.c |    7 +++++--
>  b/server/server.c  |    2 +-
>  b/server/status.c  |    8 +++++---
>  server/server.c    |   13 +++++++++----
>  7 files changed, 47 insertions(+), 22 deletions(-)

That wasn't quite right.
Here's an updated patch, including e.g., this:

diff --git a/server/config.c b/server/config.c
index a58a0e6..9539cfd 100644
--- a/server/config.c
+++ b/server/config.c
@@ -436,7 +436,8 @@ void read_config(void)

 	memset(&ctx, 0, sizeof(struct config_context));

-	if (!(tabled_srv.port = strdup("8080"))) {
+	tabled_srv.port = strdup("8080");
+	if (!tabled_srv.port) {
 		applog(LOG_ERR, "no core");
 		exit(1);
 	}

==============================================================
diff --git a/server/bucket.c b/server/bucket.c
index eb03e03..cf42d2d 100644
--- a/server/bucket.c
+++ b/server/bucket.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -788,29 +788,36 @@ static GList *bucket_list_pfx(GList *content, GHashTable *common_pfx,
 	s = malloc(cpfx_len);
 	p = s;

+#define append_const(buf, c) \
+  do { memcpy(buf, c, sizeof(c)-1); (buf) += sizeof(c)-1; } while (0)
+
 	tmpl = pfx_list;
 	while (tmpl) {
 		prefix = (char *) tmpl->data;
 		pfx_len = strlen(prefix);

-		memcpy(p, optag, sizeof(optag)-1);  p += sizeof(optag)-1;
-		memcpy(p, pfoptag, sizeof(pfoptag)-1);  p += sizeof(pfoptag)-1;
-		memcpy(p, prefix, pfx_len);  p += pfx_len;
-		memcpy(p, delim, delim_len);  p += delim_len;
-		memcpy(p, pfedtag, sizeof(pfedtag)-1);  p += sizeof(pfedtag)-1;
-		memcpy(p, edtag, sizeof(edtag)-1);  p += sizeof(edtag)-1;
+		if (p) {
+			append_const(p, optag);
+			append_const(p, pfoptag);
+			memcpy(p, prefix, pfx_len);  p += pfx_len;
+			memcpy(p, delim, delim_len);  p += delim_len;
+			append_const(p, pfedtag);
+			append_const(p, edtag);
+		}

 		free(prefix);

 		tmpl = tmpl->next;
 	}
-	*p = 0;
+	if (p)
+		*p = 0;

 	free(delim);
 	g_list_free(pfx_list);

-	return g_list_append(content, s);
+	return s ? g_list_append(content, s) : content;
 }
+#undef append_const

 struct bucket_list_info {
 	char *prefix;
diff --git a/server/config.c b/server/config.c
index f94886e..dad6579 100644
--- a/server/config.c
+++ b/server/config.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2009 Red Hat, Inc.
+ * Copyright 2009, 2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -437,6 +437,10 @@ void read_config(void)
 	memset(&ctx, 0, sizeof(struct config_context));

 	tabled_srv.port = strdup("8080");
+	if (!tabled_srv.port) {
+		applog(LOG_ERR, "no core");
+		exit(1);
+	}

 	if (!g_file_get_contents(tabled_srv.config, &text, &len, NULL)) {
 		applog(LOG_ERR, "failed to read config file %s",
diff --git a/server/object.c b/server/object.c
index 3801e94..1f2f68f 100644
--- a/server/object.c
+++ b/server/object.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -801,6 +801,11 @@ static bool object_put_body(struct client *cli, const char *user,
 	cli->out_objid = objid;
 	cli->out_user = strdup(user);

+	if (!cli->out_bucket || !cli->out_key || !cli->out_user) {
+		applog(LOG_ERR, "OOM in object_put_body");
+		return cli_err(cli, InternalError);
+	}
+
 	/* handle Expect: 100-continue header, by unconditionally
 	 * requesting that they continue.
 	 */
diff --git a/server/replica.c b/server/replica.c
index 1b5e832..7c31112 100644
--- a/server/replica.c
+++ b/server/replica.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2009 Red Hat, Inc.
+ * Copyright 2009-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -541,7 +541,10 @@ static int rep_scan_parse(struct cursor *cp, struct db_obj_ent *obj)
 	}
 	obj_koff = obj->n_str * sizeof(uint16_t);

-	okey = malloc(64 + obj_klen);
+	if (!(okey = malloc(64 + obj_klen))) {
+		applog(LOG_ERR, "rep_scan_parse: no core %d", 64+obj_klen);
+		return -1;
+	}

 	memcpy(okey->bucket, obj->bucket, 64);
 	memcpy(okey->key, (char *)(obj+1) + obj_koff, obj_klen);
diff --git a/server/server.c b/server/server.c
index 3398026..044ff51 100644
--- a/server/server.c
+++ b/server/server.c
@@ -306,7 +306,8 @@ static char *pathtokey(const char *path)
 		return NULL;
 	klen = end - path;

-	key = malloc(klen + 1);
+	if ((key = malloc(klen + 1)) == NULL)
+		return NULL;
 	memcpy(key, path, klen);
 	key[klen] = 0;

@@ -356,13 +357,16 @@ static int authcheck(struct http_req *req, char *extra_bucket,
 		if (rc != DB_NOTFOUND) {
 			char s[64];

-			snprintf(s, 64, "get user '%s'", user);
+			snprintf(s, sizeof(s), "get user '%s'", user);
 			tdbrep.tdb.passwd->err(tdbrep.tdb.passwd, rc, s);
 		}
 	} else {
 		pass = val.data;
 	}

+	if (!pass)
+		goto err_cmp;
+
 	hreq_sign(req, extra_bucket, pass, b64sig);
 	free(pass);

@@ -996,7 +1000,8 @@ static bool cli_evt_http_req(struct client *cli, unsigned int events)
 	if (debugging)
 		applog(LOG_INFO,
 		       "%s: method %s, path '%s', key '%s', bucket '%s'",
-		       cli->addr_host, method, path, key, bucket);
+		       cli->addr_host, method, path ? path : "<NULL>",
+		       key, bucket);

 	if (auth) {
 		err = authcheck(&cli->req, buck_in_path? NULL: bucket, auth,
diff --git a/server/status.c b/server/status.c
index e9fbb38..7d4cc9a 100644
--- a/server/status.c
+++ b/server/status.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -158,13 +158,14 @@ bool stat_evt_http_req(struct client *cli, unsigned int events)
 	char *path = NULL;
 	// int rc;
 	bool rcb;
+	char *root = (char *) "/";

 	/* grab useful headers */
 	// content_len_str = hreq_hdr(req, "content-length");

 	path = strdup(req->uri.path);
 	if (!path)
-		path = strdup("/");
+		path = root;

 	if (debugging)
 		applog(LOG_INFO, "%s: status method %s, path '%s'",
@@ -195,6 +196,7 @@ bool stat_evt_http_req(struct client *cli, unsigned int events)
 		rcb = stat_err(cli, InvalidArgument);
 	}

-	free(path);
+	if (path != root);
+		free(path);
 	return rcb;
 }