On Thu, Feb 20, 2020 at 11:33:26AM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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
