Re: [PATCH v8 03/15] bugreport: add tool to generate debugging info

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> +	argc = parse_options(argc, argv, "", bugreport_options,
> +			     bugreport_usage, 0);

Which one between an empty string and NULL is more appropriate to be
passed as "prefix" here?  I assume that this is *not* really a git
program, and no repository/working-tree discovery is involved, and
you won't be using OPT_FILENAME, so it would probably be OK.

> +
> +	if (option_output) {
> +		strbuf_addstr(&report_path, option_output);
> +		strbuf_complete(&report_path, '/');
> +	}
> +
> +
> +	strbuf_addstr(&report_path, "git-bugreport-");
> +	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
> +	strbuf_addstr(&report_path, ".txt");
> +
> +	if (!stat(report_path.buf, &statbuf))
> +		die("'%s' already exists", report_path.buf);

Missed i18n/l10n, but I do not think it is a useful thing for this
check to be here in the first place.

> +	switch (safe_create_leading_directories(report_path.buf)) {
> +	case SCLD_OK:
> +	case SCLD_EXISTS:
> +		break;
> +	default:
> +		die(_("could not create leading directories for '%s'"),
> +		    report_path.buf);
> +	}
> +
> +	get_bug_template(&buffer);
> +
> +	report = fopen_for_writing(report_path.buf);

fopen_for_writing() does not give good semantics for what you seem
to want to do here (i.e. to make sure you do not overwrite an
existing one).  It even has "if we cannot open it, retry after
unlinking" logic in it, which screams "this function is designed for
those who want to overwrite the file", and if you look at its
callers, they are _all_ about opening a file for writing with a well
known name like FETCH_HEAD, COMMIT_EDITMSG, etc.

Besides, a stat() that dies when a file exists would not
help---since that check, you've spent non-zero time, creating
leading directories and preparing boilerplate in the buffer,
and there is no guarantee somebody else used the same filename
in the meantime.

If you want to avoid overwriting an existing file (which I think is
a good idea---I just do not think the earlier "if (!stat()) die()"
is a good way to do so), the only sensible way to do so is to ask
your open/creat to fail if there already is a file---that is how
you'd avoid racing with another process that may be creating such a
file.  Grep for O_EXCL to find where the flag is used with O_CREAT
to see how it is done.

> +	if (report == NULL) {

Style. "if (!report)"

> +		strbuf_release(&report_path);
> +		die("couldn't open '%s' for writing", report_path.buf);

Do we see a use-after-free here?  Also missed i18n/l10n.

It is embarrassing on the reviewers' side that this (which is not
new in this round at all and hasn't changed since v6) hasn't been
caught for a few rounds X-<.

Thanks.