Re: [PATCH 12/15] use get_commit_buffer everywhere
On Tue, Jun 10, 2014 at 09:06:35AM -0700, Junio C Hamano wrote: > > So any call to strbuf_detach on the result would be disastrous. > > You are right. Where did this original crap come from X-<... I do not know if that face means you actually looked at the history, but in case you did not... It was added by Duy's 13f8b72 (Convert commit_tree() to take strbuf as message, 2011-12-15). However that was v2 of his patch. If you read the original thread, you can see that v1 passed a separate pointer/length pair, and was only changed after a reviewer-who-shall-not-be-named asked him to change it. ;) Of course there were many people participating in the review, and none of us noticed it. I think it is simply a subtle bug. > > I feel like the most elegant solution is for create_notes_commit to take > > a buf/len pair rather than a strbuf, but it unfortunately is just > > feeding that to commit_tree. Adjusting that code path would affect quite > > a few other spots. > > > > The other obvious option is actually populating the strbuf, but it feels > > ugly to have to make a copy just to satisfy the function interface. > > > > Maybe a cast and a warning comment are the least evil thing, as below? I > > dunno, it feels pretty wrong. > > Yeah, it does feel wrong wrong wrong. Perhaps this big comment > would serve as a marker for a low-hanging fruit for somebody else to > fix it, e.g. by using strbuf-add to make a copy, which would be the > easiest and safest workaround? I really think commit_tree is the culprit here. It doesn't actually want a strbuf at all, but takes one to make passing the pointer/len pair simpler. Fixing it turned out to be not _too_ disruptive, and it showed that there is another dubious use of strbuf_attach from a different caller. I'll post my re-rolled series with those fixes 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 12/15] use get_commit_buffer everywhere
Jeff King writes: > I agree it's not right, though. I think the original is questionable, > too. It takes a pointer into the middle of partial_commit->buffer and > attaches it to a strbuf. That's wrong because: > > 1. It's pointing into the middle of an allocated buffer, not the > beginning. > > 2. We do not own partial_commit->buffer in the first place. > > So any call to strbuf_detach on the result would be disastrous. You are right. Where did this original crap come from X-<... > ... and it > doesn't cause a bug in practice because the only use of the strbuf is to > pass it as a const to create_notes_commit. > > I feel like the most elegant solution is for create_notes_commit to take > a buf/len pair rather than a strbuf, but it unfortunately is just > feeding that to commit_tree. Adjusting that code path would affect quite > a few other spots. > > The other obvious option is actually populating the strbuf, but it feels > ugly to have to make a copy just to satisfy the function interface. > > Maybe a cast and a warning comment are the least evil thing, as below? I > dunno, it feels pretty wrong. Yeah, it does feel wrong wrong wrong. Perhaps this big comment would serve as a marker for a low-hanging fruit for somebody else to fix it, e.g. by using strbuf-add to make a copy, which would be the easiest and safest workaround? > diff --git a/notes-merge.c b/notes-merge.c > index 94a1a8a..1f3b309 100644 > --- a/notes-merge.c > +++ b/notes-merge.c > @@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o, > DIR *dir; > struct dirent *e; > struct strbuf path = STRBUF_INIT; > - char *msg = strstr(partial_commit->buffer, "\n\n"); > + const char *msg = strstr(partial_commit->buffer, "\n\n"); > struct strbuf sb_msg = STRBUF_INIT; > int baselen; > > @@ -719,7 +719,15 @@ int notes_merge_commit(struct notes_merge_options *o, > strbuf_setlen(&path, baselen); > } > > - strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1); > + /* > + * This is a bit tricky. We should not be attaching msg, which > + * is not owned by us and is not even the start of a heap buffer, to a > + * strbuf. But the create_notes_commit interface really wants > + * a strbuf, even though it will only ever use it as a buf/len pair and > + * never modify it. So this is tentatively safe as long as nobody ever > + * modifies, detaches, or releases the strbuf. > + */ > + strbuf_attach(&sb_msg, (char *)msg, strlen(msg), strlen(msg) + 1); > create_notes_commit(partial_tree, partial_commit->parents, &sb_msg, > result_sha1); > if (o->verbosity >= 4) > > I'm still confused and disturbed that my gcc is not noticing this > obvious const violation. Hmm, shutting off ccache seems to make it work. > Doubly disturbing. > > -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 12/15] use get_commit_buffer everywhere
On Mon, Jun 09, 2014 at 08:02:24PM -0400, Jeff King wrote: > I'm still confused and disturbed that my gcc is not noticing this > obvious const violation. Hmm, shutting off ccache seems to make it work. > Doubly disturbing. Ah, mystery solved. It's a gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60014 I get: $ gcc -c -Wall -Werror -DSHA1_HEADER='"block-sha1/sha1.h"' notes-merge.c notes-merge.c: In function ‘notes_merge_commit’: notes-merge.c:723:2: error: passing argument 2 of ‘strbuf_attach’ discards ‘const’ qualifier from pointer target type [-Werror] ...etc... $ gcc -E -Wall -Werror -DSHA1_HEADER='"block-sha1/sha1.h"' notes-merge.c >foo.c $ gcc -c -Wall -Werror -DSHA1_HEADER='"block-sha1/sha1.h"' foo.c [no warnings from either] ccache uses the latter technique. -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 12/15] use get_commit_buffer everywhere
On Mon, Jun 09, 2014 at 03:40:57PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > diff --git a/notes-merge.c b/notes-merge.c > > index 94a1a8a..7885ab2 100644 > > --- a/notes-merge.c > > +++ b/notes-merge.c > > @@ -671,7 +671,8 @@ int notes_merge_commit(struct notes_merge_options *o, > > DIR *dir; > > struct dirent *e; > > struct strbuf path = STRBUF_INIT; > > - char *msg = strstr(partial_commit->buffer, "\n\n"); > > + const char *buffer = get_commit_buffer(partial_commit); > > + const char *msg = strstr(buffer, "\n\n"); > > This tightening causes... > > > struct strbuf sb_msg = STRBUF_INIT; > > int baselen; > > > > @@ -720,6 +721,7 @@ int notes_merge_commit(struct notes_merge_options *o, > > } > > > > strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1); > > ...a new error here: > > notes-merge.c:723:2: error: passing argument 2 of 'strbuf_attach' > discards 'const' qualifier from pointer target type [-Werror] > strbuf.h:19:13: note: expected 'void *' but argument is of type > 'const char *' That's weird. I compile with -Wall -Werror, and my gcc doesn't complain. Hmph. I agree it's not right, though. I think the original is questionable, too. It takes a pointer into the middle of partial_commit->buffer and attaches it to a strbuf. That's wrong because: 1. It's pointing into the middle of an allocated buffer, not the beginning. 2. We do not own partial_commit->buffer in the first place. So any call to strbuf_detach on the result would be disastrous. The compiler doesn't notice because of the const leak in strstr, and it doesn't cause a bug in practice because the only use of the strbuf is to pass it as a const to create_notes_commit. I feel like the most elegant solution is for create_notes_commit to take a buf/len pair rather than a strbuf, but it unfortunately is just feeding that to commit_tree. Adjusting that code path would affect quite a few other spots. The other obvious option is actually populating the strbuf, but it feels ugly to have to make a copy just to satisfy the function interface. Maybe a cast and a warning comment are the least evil thing, as below? I dunno, it feels pretty wrong. diff --git a/notes-merge.c b/notes-merge.c index 94a1a8a..1f3b309 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o, DIR *dir; struct dirent *e; struct strbuf path = STRBUF_INIT; - char *msg = strstr(partial_commit->buffer, "\n\n"); + const char *msg = strstr(partial_commit->buffer, "\n\n"); struct strbuf sb_msg = STRBUF_INIT; int baselen; @@ -719,7 +719,15 @@ int notes_merge_commit(struct notes_merge_options *o, strbuf_setlen(&path, baselen); } - strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1); + /* +* This is a bit tricky. We should not be attaching msg, which +* is not owned by us and is not even the start of a heap buffer, to a +* strbuf. But the create_notes_commit interface really wants +* a strbuf, even though it will only ever use it as a buf/len pair and +* never modify it. So this is tentatively safe as long as nobody ever +* modifies, detaches, or releases the strbuf. +*/ + strbuf_attach(&sb_msg, (char *)msg, strlen(msg), strlen(msg) + 1); create_notes_commit(partial_tree, partial_commit->parents, &sb_msg, result_sha1); if (o->verbosity >= 4) I'm still confused and disturbed that my gcc is not noticing this obvious const violation. Hmm, shutting off ccache seems to make it work. Doubly disturbing. -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 12/15] use get_commit_buffer everywhere
Jeff King writes: > diff --git a/notes-merge.c b/notes-merge.c > index 94a1a8a..7885ab2 100644 > --- a/notes-merge.c > +++ b/notes-merge.c > @@ -671,7 +671,8 @@ int notes_merge_commit(struct notes_merge_options *o, > DIR *dir; > struct dirent *e; > struct strbuf path = STRBUF_INIT; > - char *msg = strstr(partial_commit->buffer, "\n\n"); > + const char *buffer = get_commit_buffer(partial_commit); > + const char *msg = strstr(buffer, "\n\n"); This tightening causes... > struct strbuf sb_msg = STRBUF_INIT; > int baselen; > > @@ -720,6 +721,7 @@ int notes_merge_commit(struct notes_merge_options *o, > } > > strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1); ...a new error here: notes-merge.c:723:2: error: passing argument 2 of 'strbuf_attach' discards 'const' qualifier from pointer target type [-Werror] strbuf.h:19:13: note: expected 'void *' but argument is of type 'const char *' -- 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
