Re: [PATCH v3 02/25] Convert git_snpath() to strbuf_git_path()
Nguyễn Thái Ngọc Duy writes: > @@ -651,14 +653,10 @@ static void update_refs_for_switch(const struct > checkout_opts *opts, > new->name); > } > } > - if (old->path && old->name) { > - char log_file[PATH_MAX], ref_file[PATH_MAX]; > - > - git_snpath(log_file, sizeof(log_file), "logs/%s", > old->path); > - git_snpath(ref_file, sizeof(ref_file), "%s", old->path); > - if (!file_exists(ref_file) && file_exists(log_file)) > - remove_path(log_file); > - } > + if (old->path && old->name && > + !file_exists(git_path("%s", old->path)) && > + file_exists(git_path("logs/%s", old->path))) > + remove_path(git_path("logs/%s", old->path)); Hmph. Is this conversion safe? This adds three uses of the round-robin path buffer; if a caller of this function used two or more path buffers obtained from get_pathname() and expected their contents to remain stable across the call to this, it will silently break. -- 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 v3 02/25] Convert git_snpath() to strbuf_git_path()
On Thu, Feb 20, 2014 at 6:48 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> @@ -651,14 +653,10 @@ static void update_refs_for_switch(const struct >> checkout_opts *opts, >> new->name); >> } >> } >> - if (old->path && old->name) { >> - char log_file[PATH_MAX], ref_file[PATH_MAX]; >> - >> - git_snpath(log_file, sizeof(log_file), "logs/%s", >> old->path); >> - git_snpath(ref_file, sizeof(ref_file), "%s", >> old->path); >> - if (!file_exists(ref_file) && file_exists(log_file)) >> - remove_path(log_file); >> - } >> + if (old->path && old->name && >> + !file_exists(git_path("%s", old->path)) && >> + file_exists(git_path("logs/%s", old->path))) >> + remove_path(git_path("logs/%s", old->path)); > > Hmph. Is this conversion safe? > > This adds three uses of the round-robin path buffer; if a caller of > this function used two or more path buffers obtained from > get_pathname() and expected their contents to remain stable across > the call to this, it will silently break. three round-robin buffers but not all required at the same time, once the first file_exists() returns the first round-robin buffer could be free, and remove_path() calls git_path again, not reusing the result from the second file_exists, so the second buffer is free to go too. -- 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
Re: [PATCH v3 02/25] Convert git_snpath() to strbuf_git_path()
Duy Nguyen writes: >>> - } >>> + if (old->path && old->name && >>> + !file_exists(git_path("%s", old->path)) && >>> + file_exists(git_path("logs/%s", old->path))) >>> + remove_path(git_path("logs/%s", old->path)); >> >> Hmph. Is this conversion safe? >> >> This adds three uses of the round-robin path buffer; if a caller of >> this function used two or more path buffers obtained from >> get_pathname() and expected their contents to remain stable across >> the call to this, it will silently break. > > three round-robin buffers but not all required at the same time, once > the first file_exists() returns the first round-robin buffer could be > free, and remove_path() calls git_path again, not reusing the result > from the second file_exists, so the second buffer is free to go too. I know these three callers to git_path() will not step on each other's toes. But that is not the question I asked. -- 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 v3 02/25] Convert git_snpath() to strbuf_git_path()
On Thu, Feb 20, 2014 at 10:41 AM, Junio C Hamano wrote: > Duy Nguyen writes: > - } + if (old->path && old->name && + !file_exists(git_path("%s", old->path)) && + file_exists(git_path("logs/%s", old->path))) + remove_path(git_path("logs/%s", old->path)); >>> >>> Hmph. Is this conversion safe? >>> >>> This adds three uses of the round-robin path buffer; if a caller of >>> this function used two or more path buffers obtained from >>> get_pathname() and expected their contents to remain stable across >>> the call to this, it will silently break. >> >> three round-robin buffers but not all required at the same time, once >> the first file_exists() returns the first round-robin buffer could be >> free, and remove_path() calls git_path again, not reusing the result >> from the second file_exists, so the second buffer is free to go too. > > I know these three callers to git_path() will not step on each > other's toes. But that is not the question I asked. OK so your question was if there was a git_path() or mkpath() call earlier in update_refs_for_switch() and the result was expected to remain stable till the end of update_refs_for_switch(), then this conversion could ruin it, correct? I didn't think about that, but I have checked and the only mkpath() call in this function is not supposed to last long. If it's about a git_pathname() call outside update_refs_...() that still expects to be stable, we should fix that code instead. -- 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
Re: [PATCH v3 02/25] Convert git_snpath() to strbuf_git_path()
Duy Nguyen writes: > OK so your question was if there was a git_path() or mkpath() call > earlier in update_refs_for_switch() and the result was expected to > remain stable till the end of update_refs_for_switch(), then this > conversion could ruin it, correct? I didn't think about that,... Yeah, I couldn't tell if you thought about it, and that was why I asked. If a (recursively) caller does this: caller () { const char *path1 = git_path(...); const char *path2 = mkpath(...); const char *path3 = git_path_submodule(...); callee(); use(path1, path2, path3); } it was safe back when the callee() did not mess with the round-robin pathname buffer, but it will be broken once callee() does. While looking at the patch I didn't check what the caller was doing hence my question. In general, in order to reduce that kind of hard-to-debug bugs, we should be reducing the uses of these functions when we do not have to (which applies equally to such a caller that expects multiple temporary paths to persist, and to a callee as well). Adding multiple repeated calls to git_path(), especially two of them formatting the same string into two separate round-robin pathname buffer, looked strange in a patch that was supposed to be a preparatory code-cleanup stage. -- 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 v3 02/25] Convert git_snpath() to strbuf_git_path()
Nguyễn Thái Ngọc Duy writes: > @@ -2717,17 +2729,19 @@ static int copy_msg(char *buf, const char *msg) > return cp - buf; > } > > -int log_ref_setup(const char *refname, char *logfile, int bufsize) > +int log_ref_setup(const char *refname, struct strbuf *sb_logfile) > { > int logfd, oflags = O_APPEND | O_WRONLY; > + const char *logfile; > > - git_snpath(logfile, bufsize, "logs/%s", refname); > + strbuf_git_path(sb_logfile, "logs/%s", refname); > + logfile = sb_logfile->buf; > if (log_all_ref_updates && > (starts_with(refname, "refs/heads/") || >starts_with(refname, "refs/remotes/") || >starts_with(refname, "refs/notes/") || >!strcmp(refname, "HEAD"))) { > - if (safe_create_leading_directories(logfile) < 0) > + if (safe_create_leading_directories(sb_logfile->buf) < 0) Other references to logfile in this function are kept as-is, and I think this line can also stay as-it-was. That is the whole point of introducing the local variable logfile and assinging to it upfront as a synonym, isn't it? -- 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