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

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

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