Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]

On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano [off-list ref] wrote:

David Sharp [off-list ref] writes:

quoted

Without this patch, git-rev-parse --prefix, --default, or
--resolve-git-dir, without a value argument, would result in a segfault.
Instead, die() with a message.

When I sent the review message, I actually was on the fence between
checking i vs argc and checking the nullness myself.  I realize,
after seeing the actual patch below, that we are protecting against
picking up a NULL and blindly passing it on in the codepaths that
follow, so the updated code does look a lot better, at least to me.

It's a matter of perspective, I guess. When checking the index, to me,
I saw that it is protecting against reading past the end of an array,
which is at least as bad as sending NULL to codepaths that don't
accept NULL. I do think that everybody knows that reading past the end
of an array is bad, while not necessarily everybody knows that the
argv array is null-terminated.

Thanks.

quoted

Signed-off-by: David Sharp <redacted>
---
 builtin/rev-parse.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..45901df 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c

@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                              continue;
                      }
                      if (!strcmp(arg, "--default")) {
-                             def = argv[i+1];
-                             i++;
+                             def = argv[++i];
+                             if (!def)
+                                     die("--default requires an argument");
                              continue;
                      }
                      if (!strcmp(arg, "--prefix")) {
-                             prefix = argv[i+1];
+                             prefix = argv[++i];
+                             if (!prefix)
+                                     die("--prefix requires an argument");
                              startup_info->prefix = prefix;
                              output_prefix = 1;
-                             i++;
                              continue;
                      }
                      if (!strcmp(arg, "--revs-only")) {

@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
                              continue;
                      }
                      if (!strcmp(arg, "--resolve-git-dir")) {
-                             const char *gitdir = resolve_gitdir(argv[i+1]);
+                             const char *gitdir = argv[++i];
                              if (!gitdir)
-                                     die("not a gitdir '%s'", argv[i+1]);
+                                     die("--resolve-git-dir requires an argument");
+                             gitdir = resolve_gitdir(gitdir);
+                             if (!gitdir)
+                                     die("not a gitdir '%s'", argv[i]);
                              puts(gitdir);
                              continue;
                      }