BUG: Git command causes crash

BUG: Git command causes crash

[Ryan O'Hara]

On Git for Windows (MinGW), at least, this command causes git to crash:

git commit -a --no-message --dry-run

Re: BUG: Git command causes crash

[Jeff King]

On Sat, Dec 17, 2011 at 07:13:53PM -0800, Ryan O'Hara wrote:

> On Git for Windows (MinGW), at least, this command causes git to crash:
> 
> git commit -a --no-message --dry-run

On Linux, too, using just "git commit --no-message" (whether there is
something to commit or not). This fixes it for me.

-- >8 --
Subject: [PATCH] commit: initialize static strbuf

Strbufs cannot rely on static all-zero initialization;
instead, they must use STRBUF_INIT to point to the
"slopbuf".

Without this patch, "git commit --no-message" segfaults
reliably.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d0f27f9..336faff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -104,7 +104,7 @@
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status;
 static const char *only_include_assumed;
-static struct strbuf message;
+static struct strbuf message = STRBUF_INIT;
 
 static int null_termination;
 static enum {
-- 
1.7.8.rc3.14.gd2470

Re: BUG: Git command causes crash

[Jeff King]

On Sun, Dec 18, 2011 at 12:03:22AM -0500, Jeff King wrote:

> Subject: [PATCH] commit: initialize static strbuf
> 
> Strbufs cannot rely on static all-zero initialization;
> instead, they must use STRBUF_INIT to point to the
> "slopbuf".
> 
> Without this patch, "git commit --no-message" segfaults
> reliably.

This one, too:

diff --git a/builtin/merge.c b/builtin/merge.c
index 2457940..28a3a7e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,7 @@ struct strategy {
 static int fast_forward_only, option_edit;
 static int allow_trivial = 1, have_message;
 static int overwrite_ignore = 1;
-static struct strbuf merge_msg;
+static struct strbuf merge_msg = STRBUF_INIT;
 static struct commit_list *remoteheads;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;

I'm not sure if you can actually trigger a segfault, but it clearly
violates the strbuf API.

-Peff

Re: BUG: Git command causes crash

[Jeff King]

On Sun, Dec 18, 2011 at 12:07:28AM -0500, Jeff King wrote:

> -static struct strbuf merge_msg;
> +static struct strbuf merge_msg = STRBUF_INIT;
> [...]
> 
> I'm not sure if you can actually trigger a segfault, but it clearly
> violates the strbuf API.

One note on this: that line by itself doesn't violate the strbuf API.
You can always initialize it by hand later.  But it's easy to look
through the code in this case and see that we don't.

A grep of "static struct strbuf" doesn't show any other similar cases. I
also did a grep of "struct strbuf [a-z]*;" to find stack and struct
member variables. Most of the stack ones get initialized right away
(which is not too surprising, as they would contain random garbage and
not work at all). A few of the ones in structs are hard to track down,
and could be problematic (I expected to maybe find a memset-to-zero
initialization), but I followed all of them to an actual strbuf_init.

Which isn't to say my looking was exhaustive, as there might be other
code paths I missed. But at least these two are the low-hanging fruit.

Here's a commit which does them both together (they are really the same
bug).

-- >8 --
Subject: [PATCH] initialize static strbufs

Strbufs cannot rely on static all-zero initialization;
instead, they must use STRBUF_INIT to point to the
"slopbuf". This failure can go undetected in most code
paths, as only some of the strbuf functions assume the
slopbuf is in place (e.g., "strbuf_add" works). In
particular, calling "strbuf_setlen" will cause a segfault.

Without this patch, "git commit --no-message" and "git merge
--no-message" segfault reliably.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c |    2 +-
 builtin/merge.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d0f27f9..336faff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -104,7 +104,7 @@
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status;
 static const char *only_include_assumed;
-static struct strbuf message;
+static struct strbuf message = STRBUF_INIT;
 
 static int null_termination;
 static enum {
diff --git a/builtin/merge.c b/builtin/merge.c
index 2457940..28a3a7e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,7 @@ struct strategy {
 static int fast_forward_only, option_edit;
 static int allow_trivial = 1, have_message;
 static int overwrite_ignore = 1;
-static struct strbuf merge_msg;
+static struct strbuf merge_msg = STRBUF_INIT;
 static struct commit_list *remoteheads;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
-- 
1.7.8.rc3.14.gd2470