Re: [PATCH 3/6] pretty: prepare notes message at a centralized place
On Thu, Oct 18, 2012 at 02:17:01AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > It's possible that switching it would create bugs elsewhere (there are > > over 100 uses of strbuf_detach, so maybe somebody really does want this > > NULL behavior), but I tend to think it is just as likely to be fixing > > undiscovered bugs. > > Yeah, I tend to agree. > > This "format-patch --notes" is obviously a post 1.8.0 topic, and so > is the strbuf_detach() clean-up. Let me bookmark this thread in > case it hasn't been resolved when I came back from my vacation, so > that I won't forget ;-). Actually, I have found a few segfaults, one of them remotely triggerable in http-backend. I think it can probably wait until post-1.8.0 as it does not have any security implications, though. Details in a moment. -Peff -- 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 3/6] pretty: prepare notes message at a centralized place
Jeff King writes: > It's possible that switching it would create bugs elsewhere (there are > over 100 uses of strbuf_detach, so maybe somebody really does want this > NULL behavior), but I tend to think it is just as likely to be fixing > undiscovered bugs. Yeah, I tend to agree. This "format-patch --notes" is obviously a post 1.8.0 topic, and so is the strbuf_detach() clean-up. Let me bookmark this thread in case it hasn't been resolved when I came back from my vacation, so that I won't forget ;-). -- 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 3/6] pretty: prepare notes message at a centralized place
On Wed, Oct 17, 2012 at 10:45:25PM -0700, Junio C Hamano wrote:
> + if (opt->show_notes) {
> + int raw;
> + struct strbuf notebuf = STRBUF_INIT;
> +
> + raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
> + format_display_notes(commit->object.sha1, ¬ebuf,
> + get_log_output_encoding(), raw);
> + ctx.notes_message = notebuf.len
> + ? strbuf_detach(¬ebuf, NULL)
> + : xcalloc(1, 1);
> + }
This last line seems like it is caused by a bug in the strbuf API.
Detaching an empty string will sometimes get you NULL and sometimes not.
For example, this:
struct strbuf foo = STRBUF_INIT;
strbuf_detach(&foo, NULL);
will return NULL. But this:
struct strbuf foo = STRBUF_INIT;
strbuf_addstr(&foo, "bar");
strbuf_reset(&foo);
strbuf_detach(&foo, NULL);
will get you a zero-length string. Which just seems insane to me. The
whole point of strbuf_detach is that you do not have to care about the
internal representation. It should probably always return a newly
allocated zero-length string.
Looking through a few uses of strbuf_detach, it looks like callers
assume they will always get a pointer from strbuf_detach, and we are
saved by implementation details. For example, sha1_file_to_archive might
have an empty file, but the fact that strbuf_attach always allocates a
byte means that the detach will never return NULL. Similarly,
argv_array_pushf would never want to turn an empty string into an
accidental NULL; it is saved by the fact that strbuf_vaddf will always
preemptively allocate 64 bytes.
It's possible that switching it would create bugs elsewhere (there are
over 100 uses of strbuf_detach, so maybe somebody really does want this
NULL behavior), but I tend to think it is just as likely to be fixing
undiscovered bugs.
-Peff
--
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
