Re: [PATCH 2/2] dir: remove PATH_MAX limitation
Duy Nguyen pclo...@gmail.com writes: [01/20] dir.c: coding style fix [02/20] dir.h: move struct exclude declaration to top level [03/20] prep_exclude: remove the artificial PATH_MAX limit ...perhaps with s/if (!dir-basebuf.alloc)/if (!dir-basebuf.buf)/ @Duy any reason for not signing off that series? That series still need a lot more work, but for those first three, if you want to fast track, you have my sign-offs. If it is not too much trouble, can you send only the relevant patches (these three) with sign-off again? I'd prefer to give patches a new chance to be eyeballed by people, and they will tend to have a better chance by being a smaller and an isolated topic. Thanks. -- 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 2/2] dir: remove PATH_MAX limitation
Am 05.07.2014 12:48, schrieb Duy Nguyen: On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees karsten.bl...@gmail.com wrote: 'git status' segfaults if a directory is longer than PATH_MAX, because processing .gitignore files in prep_exclude() writes past the end of a PATH_MAX-bounded buffer. Remove the limitation by using strbuf instead. Note: this fix just 'abuses' strbuf as string allocator, len is always 0. prep_exclude() can probably be simplified using more strbuf APIs. FYI I had a similar patch [1] that attempted to lazily strbuf_init() instead so that strbuf_ API could be used. [1] http://article.gmane.org/gmane.comp.version-control.git/248310 Sorry, I missed that one. In my version, strbuf_grow() does the lazy initialization (in fact, many strbuf_* functions can handle memset(0) strbufs just fine). I was simply too lazy to understand (again) how prep_exclude works exactly, and as string calculations like that have the tendency to be just 1 char off, I went for the obviously correct solution (i.e. s/dir-basebuf/dir-base.buf/ plus strbuf_grow() before we write the buffer). But IMO your version is much cleaner already. However, api-strbuf.txt says that buf != NULL is invariant after init, and alloc is somehow private :-) , so perhaps you should - if (!dir-basebuf.alloc) + if (!dir-basebuf.buf) strbuf_init(dir-basebuf, PATH_MAX); -- 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 2/2] dir: remove PATH_MAX limitation
Am 09.07.2014 18:33, schrieb Junio C Hamano: Karsten Blees karsten.bl...@gmail.com writes: 'git status' segfaults if a directory is longer than PATH_MAX, because processing .gitignore files in prep_exclude() writes past the end of a PATH_MAX-bounded buffer. Remove the limitation by using strbuf instead. Note: this fix just 'abuses' strbuf as string allocator, len is always 0. prep_exclude() can probably be simplified using more strbuf APIs. In addition to what Jeff already said, I am afraid that this may make things a lot worse for normal case. By sizing the strbuf to absolute minimum as you dig deeper at each level, wouldn't you copy and reallocate the buffer a lot more, compared to the case where everything fits in the original buffer? strbuf uses ALLOC_GROW, which resizes in (x+16)*1.5 steps (i.e. 24, 60, 114, 195, 316...). Max path len in git is 85, and linux and WebKit are 170ish. I don't think three or four extra reallocs per directory traversal will be noticeable. Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. http://article.gmane.org/gmane.comp.version-control.git/248310 -- 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 2/2] dir: remove PATH_MAX limitation
Karsten Blees karsten.bl...@gmail.com writes: Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. http://article.gmane.org/gmane.comp.version-control.git/248310 Thanks; I've already reverted it from 'next'. Is Duy's patch still viable? -- 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 2/2] dir: remove PATH_MAX limitation
Am 12.07.2014 00:29, schrieb Junio C Hamano: Karsten Blees karsten.bl...@gmail.com writes: Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. http://article.gmane.org/gmane.comp.version-control.git/248310 Thanks; I've already reverted it from 'next'. Is Duy's patch still viable? I think so. It fixes the segfault with long paths on Windows as well (Tested-by: me), uses strbuf APIs as Peff suggested, and initializes the strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ). AFAICT the first two patches of that series are also completely unrelated to the untracked-cache, so we may want to fast-track these? [01/20] dir.c: coding style fix [02/20] dir.h: move struct exclude declaration to top level [03/20] prep_exclude: remove the artificial PATH_MAX limit ...perhaps with s/if (!dir-basebuf.alloc)/if (!dir-basebuf.buf)/ @Duy any reason for not signing off that series? -- 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 2/2] dir: remove PATH_MAX limitation
Karsten Blees karsten.bl...@gmail.com writes:
'git status' segfaults if a directory is longer than PATH_MAX, because
processing .gitignore files in prep_exclude() writes past the end of a
PATH_MAX-bounded buffer.
Remove the limitation by using strbuf instead.
Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
prep_exclude() can probably be simplified using more strbuf APIs.
In addition to what Jeff already said, I am afraid that this may
make things a lot worse for normal case. By sizing the strbuf to
absolute minimum as you dig deeper at each level, wouldn't you copy
and reallocate the buffer a lot more, compared to the case where
everything fits in the original buffer?
Signed-off-by: Karsten Blees bl...@dcon.de
---
dir.c | 35 +++
dir.h | 4 ++--
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/dir.c b/dir.c
index e65888d..8d4d83c 100644
--- a/dir.c
+++ b/dir.c
@@ -798,7 +798,7 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
* path being checked. */
while ((stk = dir-exclude_stack) != NULL) {
if (stk-baselen = baselen
- !strncmp(dir-basebuf, base, stk-baselen))
+ !strncmp(dir-base.buf, base, stk-baselen))
break;
el = group-el[dir-exclude_stack-exclude_ix];
dir-exclude_stack = stk-prev;
@@ -833,48 +833,50 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
stk-baselen = cp - base;
stk-exclude_ix = group-nr;
el = add_exclude_list(dir, EXC_DIRS, NULL);
- memcpy(dir-basebuf + current, base + current,
+ strbuf_grow(dir-base, stk-baselen);
+ memcpy(dir-base.buf + current, base + current,
stk-baselen - current);
/* Abort if the directory is excluded */
if (stk-baselen) {
int dt = DT_DIR;
- dir-basebuf[stk-baselen - 1] = 0;
+ dir-base.buf[stk-baselen - 1] = 0;
dir-exclude = last_exclude_matching_from_lists(dir,
- dir-basebuf, stk-baselen - 1,
- dir-basebuf + current, dt);
- dir-basebuf[stk-baselen - 1] = '/';
+ dir-base.buf, stk-baselen - 1,
+ dir-base.buf + current, dt);
+ dir-base.buf[stk-baselen - 1] = '/';
if (dir-exclude
dir-exclude-flags EXC_FLAG_NEGATIVE)
dir-exclude = NULL;
if (dir-exclude) {
- dir-basebuf[stk-baselen] = 0;
+ dir-base.buf[stk-baselen] = 0;
dir-exclude_stack = stk;
return;
}
}
- /* Try to read per-directory file unless path is too long */
- if (dir-exclude_per_dir
- stk-baselen + strlen(dir-exclude_per_dir) PATH_MAX) {
- strcpy(dir-basebuf + stk-baselen,
+ /* Try to read per-directory file */
+ if (dir-exclude_per_dir) {
+ strbuf_grow(dir-base, stk-baselen +
+ strlen(dir-exclude_per_dir));
+ strcpy(dir-base.buf + stk-baselen,
dir-exclude_per_dir);
/*
- * dir-basebuf gets reused by the traversal, but we
+ * dir-base gets reused by the traversal, but we
* need fname to remain unchanged to ensure the src
* member of each struct exclude correctly
* back-references its source file. Other invocations
* of add_exclude_list provide stable strings, so we
* strdup() and free() here in the caller.
*/
- el-src = strdup(dir-basebuf);
- add_excludes_from_file_to_list(dir-basebuf,
- dir-basebuf, stk-baselen, el, 1);
+ el-src = strdup(dir-base.buf);
+ add_excludes_from_file_to_list(dir-base.buf,
+ dir-base.buf, stk-baselen, el, 1);
}
dir-exclude_stack = stk;
current = stk-baselen;
}
- dir-basebuf[baselen] = '\0';
+ dir-base.buf[baselen] = '\0';
}
/*
@@ -1671,4 +1673,5 @@ void clear_directory(struct dir_struct *dir)
free(stk);
stk = prev;
}
+ strbuf_release(dir-base);
}
diff --git a/dir.h b/dir.h
index
Re: [PATCH 2/2] dir: remove PATH_MAX limitation
On Sat, Jul 05, 2014 at 12:42:29AM +0200, Karsten Blees wrote: Note: this fix just 'abuses' strbuf as string allocator, len is always 0. prep_exclude() can probably be simplified using more strbuf APIs. Hrm. It looks like you grow it and add some data, but really don't want the length to expand (because the caller depends on it). In other directory-traversal code we follow a pattern like: size_t prefix_len = dir-base.len; strbuf_add(dir-base, cp, stk-baselen - current); /* use full path in dir-base, then pop */ strbuf_setlen(dir-base, stk-baselen); That makes it a little more obvious that the memcpy matches the strbuf_grow (because it all happens inside strbuf_add). Is it possible to do something like that here? -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 2/2] dir: remove PATH_MAX limitation
On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees karsten.bl...@gmail.com wrote: 'git status' segfaults if a directory is longer than PATH_MAX, because processing .gitignore files in prep_exclude() writes past the end of a PATH_MAX-bounded buffer. Remove the limitation by using strbuf instead. Note: this fix just 'abuses' strbuf as string allocator, len is always 0. prep_exclude() can probably be simplified using more strbuf APIs. FYI I had a similar patch [1] that attempted to lazily strbuf_init() instead so that strbuf_ API could be used. [1] http://article.gmane.org/gmane.comp.version-control.git/248310 -- Duy -- 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 2/2] dir: remove PATH_MAX limitation
'git status' segfaults if a directory is longer than PATH_MAX, because
processing .gitignore files in prep_exclude() writes past the end of a
PATH_MAX-bounded buffer.
Remove the limitation by using strbuf instead.
Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
prep_exclude() can probably be simplified using more strbuf APIs.
Signed-off-by: Karsten Blees bl...@dcon.de
---
dir.c | 35 +++
dir.h | 4 ++--
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/dir.c b/dir.c
index e65888d..8d4d83c 100644
--- a/dir.c
+++ b/dir.c
@@ -798,7 +798,7 @@ static void prep_exclude(struct dir_struct *dir, const char
*base, int baselen)
* path being checked. */
while ((stk = dir-exclude_stack) != NULL) {
if (stk-baselen = baselen
- !strncmp(dir-basebuf, base, stk-baselen))
+ !strncmp(dir-base.buf, base, stk-baselen))
break;
el = group-el[dir-exclude_stack-exclude_ix];
dir-exclude_stack = stk-prev;
@@ -833,48 +833,50 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
stk-baselen = cp - base;
stk-exclude_ix = group-nr;
el = add_exclude_list(dir, EXC_DIRS, NULL);
- memcpy(dir-basebuf + current, base + current,
+ strbuf_grow(dir-base, stk-baselen);
+ memcpy(dir-base.buf + current, base + current,
stk-baselen - current);
/* Abort if the directory is excluded */
if (stk-baselen) {
int dt = DT_DIR;
- dir-basebuf[stk-baselen - 1] = 0;
+ dir-base.buf[stk-baselen - 1] = 0;
dir-exclude = last_exclude_matching_from_lists(dir,
- dir-basebuf, stk-baselen - 1,
- dir-basebuf + current, dt);
- dir-basebuf[stk-baselen - 1] = '/';
+ dir-base.buf, stk-baselen - 1,
+ dir-base.buf + current, dt);
+ dir-base.buf[stk-baselen - 1] = '/';
if (dir-exclude
dir-exclude-flags EXC_FLAG_NEGATIVE)
dir-exclude = NULL;
if (dir-exclude) {
- dir-basebuf[stk-baselen] = 0;
+ dir-base.buf[stk-baselen] = 0;
dir-exclude_stack = stk;
return;
}
}
- /* Try to read per-directory file unless path is too long */
- if (dir-exclude_per_dir
- stk-baselen + strlen(dir-exclude_per_dir) PATH_MAX) {
- strcpy(dir-basebuf + stk-baselen,
+ /* Try to read per-directory file */
+ if (dir-exclude_per_dir) {
+ strbuf_grow(dir-base, stk-baselen +
+ strlen(dir-exclude_per_dir));
+ strcpy(dir-base.buf + stk-baselen,
dir-exclude_per_dir);
/*
-* dir-basebuf gets reused by the traversal, but we
+* dir-base gets reused by the traversal, but we
* need fname to remain unchanged to ensure the src
* member of each struct exclude correctly
* back-references its source file. Other invocations
* of add_exclude_list provide stable strings, so we
* strdup() and free() here in the caller.
*/
- el-src = strdup(dir-basebuf);
- add_excludes_from_file_to_list(dir-basebuf,
- dir-basebuf, stk-baselen, el, 1);
+ el-src = strdup(dir-base.buf);
+ add_excludes_from_file_to_list(dir-base.buf,
+ dir-base.buf, stk-baselen, el, 1);
}
dir-exclude_stack = stk;
current = stk-baselen;
}
- dir-basebuf[baselen] = '\0';
+ dir-base.buf[baselen] = '\0';
}
/*
@@ -1671,4 +1673,5 @@ void clear_directory(struct dir_struct *dir)
free(stk);
stk = prev;
}
+ strbuf_release(dir-base);
}
diff --git a/dir.h b/dir.h
index 55e5345..e870fb6 100644
--- a/dir.h
+++ b/dir.h
@@ -111,13 +111,13 @@ struct dir_struct {
* per-directory exclude lists.
*
* exclude_stack points to the top of the exclude_stack, and
-* basebuf contains the full path to the current
+* base contains the full path to the
