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
