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

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

[Jim Meyering]

Looking at tabled's code, I see a few places where unchecked strdup
can lead to NULL deref, whereas the majority are checked carefully.
Patches for the first two I found are below -- haven't completed the job.

In most cases, I see that care is taken to detect failure and propagate
that to higher levels.  In others, due to the use of glib functions,
OOM leads to immediate (and possibly-unclean?) exit, because glib simply
calls exit on OOM.  Or perhaps tabled tells glib not to handle OOM that
way -- assuming that's possible.

This is server-style (and some of it library-grade) code, so I'm surprised
to see it relying on glib, which due to its handling of OOM errors
is often deemed unsuitable for applications that need to die
gracefully.

I'm not 100% sure that each of these strdup
failures will lead inevitably to a NULL deref, but a quick
manual trace suggested it's possible, so...

From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Thu, 23 Sep 2010 10:11:44 +0200
Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 server/config.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/server/config.c b/server/config.c
index f94886e..a58a0e6 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
@@ -436,7 +436,10 @@ void read_config(void)

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

-	tabled_srv.port = strdup("8080");
+	if (!(tabled_srv.port = strdup("8080"))) {
+		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",
--
1.7.3.234.g7bba3


From 725ff27469746639f0da098feab6c3d0a28e7b30 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Thu, 23 Sep 2010 10:25:15 +0200
Subject: [PATCH tabled 2/2] server/object.c: don't deref NULL on OOM


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 server/object.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

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.
 	 */
--
1.7.3.234.g7bba3

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

[Jeff Garzik]

On 09/23/2010 04:43 AM, Jim Meyering wrote:
>
> Looking at tabled's code, I see a few places where unchecked strdup
> can lead to NULL deref, whereas the majority are checked carefully.
> Patches for the first two I found are below -- haven't completed the job.
>
> In most cases, I see that care is taken to detect failure and propagate
> that to higher levels.  In others, due to the use of glib functions,
> OOM leads to immediate (and possibly-unclean?) exit, because glib simply
> calls exit on OOM.  Or perhaps tabled tells glib not to handle OOM that
> way -- assuming that's possible.
>
> This is server-style (and some of it library-grade) code, so I'm surprised
> to see it relying on glib, which due to its handling of OOM errors
> is often deemed unsuitable for applications that need to die
> gracefully.

It's a holdover from kernel coding that I (and Pete?) obsessively check 
return values, even from memory allocation functions.  Occasionally I 
wonder if I'll ever receive an email, from an odd duck somewhere, 
thanking me for writing a server that works even VM overcommit support 
disabled.

On GLib:  it was just so darned convenient, and portable too.  It gave 
quick access to non-Linux OS's, and a bunch of convenience functions.

GLib's OOM death behavior itself is configurable via g_log*fatal*, but 
you're absolutely right that the code itself makes a standard assumption 
that memory allocation functions always succeed.

I wouldn't complain if the GLib dependency went away, but that's quite a 
project for someone...

(now, on to sending a separate email regarding the specific changes 
you've submitted here...)

	Jeff

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

[Jeff Garzik]

On 09/23/2010 04:43 AM, Jim Meyering wrote:
>> From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
> From: Jim Meyering<meyering@redhat.com>
> Date: Thu, 23 Sep 2010 10:11:44 +0200
> Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

(see other email for general response to these changes, comments on 
GLib, OOM, etc.)

First off, I ACK (accept) all these changes.  Technically they appear 
correct, and I am interested in merging them.

But I request a few minor style and workflow adjustments, and a 
resubmission. Specific comments:

[style]

1) the functional style of sizeof keyword, with parens, is preferred:

-			snprintf(s, 64, "get user '%s'", user);
+			snprintf(s, sizeof s, "get user '%s'", user);


2) it is preferred to omit optional braces for singleton test+stmt style 
statements:

+	if (!pass) {
+		goto err_cmp;
+	}


[patch submission administrivia]

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.

In your case, while the patch descriptions and diffs themselves are 
correct, you seem to be sending one-mbox-per-email, while I'm expecting 
one-patch-per-email.  If you could tweak your process to make that 
change, that would reduce the manual labor on my part.


4) While total number of patches is not really a problem, I would 
request sweeping most of the one-and-two-liners in this series into a 
single patch, leaving perhaps only the bucket.c and status.c changes as 
standalone patches.

It's more an art and style preferences, than science, when deciding how 
to separate out changes into patches. Trying to take my cues from the 
kernel, it is preferred, for example, that bug fixes be separate from 
new features, or whitespace and cosmetic changes separate from 
functional changes.  But it is also encouraged to group similar changes 
together, if, for example, you're making a similar change across a large 
number of files.

Mailing list review-ability, useful 'git bisect' boundaries, and a 
coherent 'git shortlog' summary tend to be my guides when deciding patch 
boundaries.

Thanks!

	Jeff

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

[Jeff Garzik]

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.

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

[Jim Meyering]

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(-)

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..a58a0e6 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
@@ -436,7 +436,10 @@ void read_config(void)

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

-	tabled_srv.port = strdup("8080");
+	if (!(tabled_srv.port = strdup("8080"))) {
+		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;
 }

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

[Jim Meyering]

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;
 }

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

[Jeff Garzik]

On 09/24/2010 07:32 AM, Jim Meyering wrote:
> You can pull from the "oom" branch here:
>    git://git.infradead.org/users/meyering/tabled.git


Got nearly everything perfect.  Need one more minor yet important 
change.  As described in doc/contributions.txt, every changeset MUST 
have a Signed-off-by line at the end of a changeset's description.

I was able to pull and build just fine, so your git repo setup and push 
appears correct.

Also, in your pull request, please put the branch immediately following 
the repo URL on the same line, for easier cut-n-paste.  Here's how Linus 
requests his pull-requests to look:

---------------------------SNIP-----------------------------
Please pull from 'upstream-linus' branch of
git://git.kernel.org/pub/scm/git/jgarzik/libata-dev.git upstream-linus

to receive the following updates:

  drivers/ata/ahci.c        |  193 
+++++++++++++++++++++++++++++++--------------
  drivers/ata/libata-acpi.c |   40 +++++-----
  drivers/ata/libata-core.c |    3 +
  drivers/ata/libata.h      |    2 +
  drivers/ata/pata_ali.c    |    2 +-
  include/linux/ata.h       |    9 ++-
  include/linux/libata.h    |   12 +++
  7 files changed, 178 insertions(+), 83 deletions(-)

Dirk Hohndel (1):
       pata_ali: trivial fix of a very frequent spelling mistake

Robert Hancock (1):
       ahci: display all AHCI 1.3 HBA capability flags (v2)

Tejun Heo (5):
       ahci: disable 64bit DMA by default on SB600s
       libata: cosmetic updates
       libata: implement more acpi filtering options
       libata: make gtf_filter per-dev
       ahci: filter FPDMA non-zero offset enable for Aspire 3810T

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index acd1162..4edca6e 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
[COMBINED PATCH FOLLOWS...]

---------------------------SNIP-----------------------------

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

[Jim Meyering]

Jeff Garzik wrote:
> On 09/24/2010 07:32 AM, Jim Meyering wrote:
>> You can pull from the "oom" branch here:
>>    git://git.infradead.org/users/meyering/tabled.git
>
> Got nearly everything perfect.  Need one more minor yet important
> change.  As described in doc/contributions.txt, every changeset MUST
> have a Signed-off-by line at the end of a changeset's description.
>
> I was able to pull and build just fine, so your git repo setup and
> push appears correct.
>
> Also, in your pull request, please put the branch immediately
> following the repo URL on the same line, for easier cut-n-paste.
> Here's how Linus requests his pull-requests to look:

Ok.  I've added those pesky S.O.B lines with this:

  git filter-branch --msg-filter \
    'cat && printf "\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n"' \
    HEAD~4..HEAD

and pushed the result.

Please pull from the 'oom' branch of
git://git.infradead.org/users/meyering/tabled.git

I presume there's no need to re-post the diffstat or diffs.

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

[Jeff Garzik]

On 09/24/2010 01:43 PM, Jim Meyering wrote:
> Jeff Garzik wrote:
>> On 09/24/2010 07:32 AM, Jim Meyering wrote:
>>> You can pull from the "oom" branch here:
>>>     git://git.infradead.org/users/meyering/tabled.git
>>
>> Got nearly everything perfect.  Need one more minor yet important
>> change.  As described in doc/contributions.txt, every changeset MUST
>> have a Signed-off-by line at the end of a changeset's description.
>>
>> I was able to pull and build just fine, so your git repo setup and
>> push appears correct.
>>
>> Also, in your pull request, please put the branch immediately
>> following the repo URL on the same line, for easier cut-n-paste.
>> Here's how Linus requests his pull-requests to look:
>
> Ok.  I've added those pesky S.O.B lines with this:
>
>    git filter-branch --msg-filter \
>      'cat&&  printf "\nSigned-off-by: Jim Meyering<meyering@redhat.com>\n"' \
>      HEAD~4..HEAD
>
> and pushed the result.
>
> Please pull from the 'oom' branch of
> git://git.infradead.org/users/meyering/tabled.git

pulled from you & pushed upstream, thanks!

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

[Jim Meyering]

Jeff Garzik wrote:
> On 09/23/2010 04:43 AM, Jim Meyering wrote:
>>> From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
>> From: Jim Meyering<meyering@redhat.com>
>> Date: Thu, 23 Sep 2010 10:11:44 +0200
>> Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
>
> (see other email for general response to these changes, comments on
> GLib, OOM, etc.)
>
> First off, I ACK (accept) all these changes.  Technically they appear
> correct, and I am interested in merging them.
>
> But I request a few minor style and workflow adjustments, and a
> resubmission. Specific comments:
>
> [style]
>
> 1) the functional style of sizeof keyword, with parens, is preferred:
>
> -			snprintf(s, 64, "get user '%s'", user);
> +			snprintf(s, sizeof s, "get user '%s'", user);

Sure.  Adjusted.

> 2) it is preferred to omit optional braces for singleton test+stmt
> style statements:
>
> +	if (!pass) {
> +		goto err_cmp;
> +	}

Gladly.  That's what I would have done in code I own, but there is a
braced single-line "else" block just above, so I presumed that the
style was "always use braces".  (I think we have the same preferences,
since I too would use braces around the single-line "else" in that case,
though not if the "then" block had also been a one-liner.

> [patch submission administrivia]
>
> 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.
>
> In your case, while the patch descriptions and diffs themselves are
> correct, you seem to be sending one-mbox-per-email, while I'm
> expecting one-patch-per-email.  If you could tweak your process to
> make that change, that would reduce the manual labor on my part.

No problem.

> 4) While total number of patches is not really a problem, I would
> request sweeping most of the one-and-two-liners in this series into a
> single patch, leaving perhaps only the bucket.c and status.c changes
> as standalone patches.

Will do.
You can tell that I'm too accustomed to posting FYI-patches
that I will shortly push -- or that I'll push upon review.

> It's more an art and style preferences, than science, when deciding
> how to separate out changes into patches. Trying to take my cues from
> the kernel, it is preferred, for example, that bug fixes be separate
> from new features, or whitespace and cosmetic changes separate from
> functional changes.  But it is also encouraged to group similar
> changes together, if, for example, you're making a similar change
> across a large number of files.
>
> Mailing list review-ability, useful 'git bisect' boundaries, and a
> coherent 'git shortlog' summary tend to be my guides when deciding
> patch boundaries.

Preaching to the choir ;-)

Thanks for spelling out your guidelines.