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

From: Emily Shaffer <[email-masked]>
Date: Thu, 20 Feb 2020 14:33:23 -0800

On Thu, Feb 20, 2020 at 11:33:26AM -0800, Junio C Hamano wrote:
> Emily Shaffer <[email-masked]> 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.

Good point. Ouch.

> 
> 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.

Sure. Thanks, I reworked it to use open().

By the way, I noticed reading the GNU manual that file descriptors may
not be supported outside of GNU environments; but I also noticed that 1)
Git uses open() lots of places to use O_EXCL, and 2) fopen() doesn't
support exclusive opens (except in glibc, and nobody is using that
particular option in the Git codebase now). So I guess it's safe to use
open()...

> > +	if (report == NULL) {
> Style. "if (!report)"

The type of 'report' changes using open(), so this check changes too.
Now it says "if (report < 0)" - per the open() doc, it returns a
positive fd or -1.

> 
> > +		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.

Hm. I suppose it's OK to UNLEAK() like we do at the successful exit
since the die() will end the process. Added i18n too.

> 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-<.

Well, I'm embarrassed to have written it in the first place.

 - Emily