Re: [PATCH] archive-tar: fix sanity check in config parsing
On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:
> > Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> > at least a comment?
> [...]
> We could also potentially encapsulate it in a function. I think the diff
> code has a very similar block.
Here's a series that does that, with a few other cleanups on top. The
diffstat actually ends up a few lines longer, but that is mostly because
of comments and function declarations. More importantly, though, the
call-sites are much easier to read.
Having written this series, though, I can't help but wonder if the world
would be a better place if config_fn_t looked more like:
typedef int (*config_fn_t)(const char *full_var,
const char *section,
const char *subsection,
const char *key,
const char *value,
void *data);
It's just as easy for the config parser to do this ahead of time, and by
handing off real C-strings (instead of ending up with a ptr/len pair for
the subsection), it makes the lives of the callbacks much easier (e.g.,
the final patch below contorts a bit to use string_list with the
subsection).
I can look into that, but here is the less invasive cleanup:
[1/6]: config: add helper function for parsing key names
[2/6]: archive-tar: use match_config_key when parsing config
[3/6]: convert some config callbacks to match_config_key
[4/6]: userdiff: drop parse_driver function
[5/6]: submodule: use match_config_key when parsing config
[6/6]: submodule: simplify memory handling in config parsing
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: fix sanity check in config parsing
On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:
> >For the curious, the original version of the patch[1] read:
> >
> >+ if (prefixcmp(var, "tarfilter."))
> >+ return 0;
> >+ dot = strrchr(var, '.');
> >+ if (dot == var + 9)
> >+ return 0;
> >
> >and when I shortened the config section to "tar" in a re-roll of the
> >series, I missed the corresponding change to the offset.
>
> Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> at least a comment?
Then you are relying on the two strings being the same, rather than the
string and the length being the same. If you wanted to DRY it up, it
would look like:
diff --git a/archive-tar.c b/archive-tar.c
index d1cce46..a7c0690 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -332,15 +332,17 @@ static int tar_filter_config(const char *var, const char
*value, void *data)
const char *type;
int namelen;
- if (prefixcmp(var, "tar."))
+#define SECTION "tar"
+ if (prefixcmp(var, SECTION "."))
return 0;
dot = strrchr(var, '.');
- if (dot == var + 9)
+ if (dot == var + strlen(SECTION))
return 0;
- name = var + 4;
+ name = var + strlen(SECTION) + 1;
namelen = dot - name;
type = dot + 1;
+#undef SECTION
ar = find_tar_filter(name, namelen);
if (!ar) {
(of course there are other variants where you do not use a macro, but
then you need to manually check for the "." after the prefixcmp call).
I dunno. It is technically more robust in that the offsets are computed,
but I think it is a little harder to read. Of course, I wrote the
original so I am probably not a good judge.
We could also potentially encapsulate it in a function. I think the diff
code has a very similar block.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: fix sanity check in config parsing
Jeff King wrote:
On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:
When parsing these config variable names, we currently check that
the second dot is found nine characters into the name, disallowing
filter names with a length of five characters. Additionally,
git archive crashes when the second dot is omitted:
$ ./git -c tar.foo=bar archive HEAD >/dev/null
fatal: Data too large to fit into virtual memory space.
Instead we should check if the second dot exists at all, or if
we only found the first one.
Eek. Thanks for finding it. Your fix is obviously correct.
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -335,7 +335,7 @@ static int tar_filter_config(const char *var,
const char *value, void *data) if (prefixcmp(var, "tar."))
return 0;
dot = strrchr(var, '.');
- if (dot == var + 9)
+ if (dot == var + 3)
return 0;
For the curious, the original version of the patch[1] read:
+ if (prefixcmp(var, "tarfilter."))
+ return 0;
+ dot = strrchr(var, '.');
+ if (dot == var + 9)
+ return 0;
and when I shortened the config section to "tar" in a re-roll of the
series, I missed the corresponding change to the offset.
Wouldn't it then be better ti use strlen("tar") rather than a 3? Or at least
a comment?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: fix sanity check in config parsing
On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote: > When parsing these config variable names, we currently check that > the second dot is found nine characters into the name, disallowing > filter names with a length of five characters. Additionally, > git archive crashes when the second dot is omitted: > > $ ./git -c tar.foo=bar archive HEAD >/dev/null > fatal: Data too large to fit into virtual memory space. > > Instead we should check if the second dot exists at all, or if > we only found the first one. Eek. Thanks for finding it. Your fix is obviously correct. > --- a/archive-tar.c > +++ b/archive-tar.c > @@ -335,7 +335,7 @@ static int tar_filter_config(const char *var, const char > *value, void *data) > if (prefixcmp(var, "tar.")) > return 0; > dot = strrchr(var, '.'); > - if (dot == var + 9) > + if (dot == var + 3) > return 0; For the curious, the original version of the patch[1] read: + if (prefixcmp(var, "tarfilter.")) + return 0; + dot = strrchr(var, '.'); + if (dot == var + 9) + return 0; and when I shortened the config section to "tar" in a re-roll of the series, I missed the corresponding change to the offset. -Peff [1] http://thread.gmane.org/gmane.comp.version-control.git/175785/focus=175858 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
