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
[PATCH] archive-tar: fix sanity check in config parsing
git archive supports passing generated tar archives through filter commands like gzip. Additional filters can be set up using the configuration variables tar..command and tar..remote. 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. Signed-off-by: Rene Scharfe --- archive-tar.c | 2 +- t/t5000-tar-tree.sh | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/archive-tar.c b/archive-tar.c index d1cce46..093d10e 100644 --- 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; name = var + 4; diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh index e7c240f..3fbd366 100755 --- a/t/t5000-tar-tree.sh +++ b/t/t5000-tar-tree.sh @@ -212,7 +212,8 @@ test_expect_success 'git-archive --prefix=olde-' ' test_expect_success 'setup tar filters' ' git config tar.tar.foo.command "tr ab ba" && git config tar.bar.command "tr ab ba" && - git config tar.bar.remote true + git config tar.bar.remote true && + git config tar.invalid baz ' test_expect_success 'archive --list mentions user filter' ' -- 1.8.0 -- 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