Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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\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! -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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 \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. -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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- -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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/ob
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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");
+
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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. -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
On 09/23/2010 04:43 AM, Jim Meyering wrote:
From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
From: Jim Meyering
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
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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 -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
