git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 03/27] parse_arg(): really test that argument is properly terminated

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Apr 7, 2014, 13:47 UTC
Message-ID
<1396878498-19887-4-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1396878498-19887-1-git-send-email-mhagger@alum.mit.edu>
The old parse_arg(), when fed an argument
    "refs/heads/a"master

parsed 'refs/heads/a' off of the front of the argument and considered itself successful. It was only when parse_next_arg() tried to parse the *next* argument that a problem was noticed. But in fact, the definition of the input format requires arguments to be terminated by SP or NUL, so *this* argument is already erroneous and parse_arg() should diagnose the problem.

So teach parse_arg() to verify that C-quoted arguments are terminated correctly. If not, emit a more specific error message.

There is no corresponding error case of a non-C-quoted argument that is not terminated correctly, because the end of a non-quoted argument is *by definition* a space or NUL, so there is no way to insert other junk between the "end" of the argument and the argument terminator.

Adjust the tests to expect the new error message. Add a docstring to the function, incorporating the comments that were formerly within the function plus some added information.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-ref.c  | 20 +++++++++++++++-----
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..02b5f95 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
 	update->have_old = *oldvalue || line_termination;
 }
 
+/*
+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
+ * and append the result to arg.  Return a pointer to the terminator.
+ * Die if there is an error in how the argument is C-quoted.  This
+ * function is only used if not -z.
+ */
 static const char *parse_arg(const char *next, struct strbuf *arg)
 {
-	/* Parse SP-terminated, possibly C-quoted argument */
-	if (*next != '"')
+	if (*next == '"') {
+		const char *orig = next;
+
+		if (unquote_c_style(arg, next, &next))
+			die("badly quoted argument: %s", orig);
+		if (*next && !isspace(*next))
+			die("unexpected character after quoted argument: %s", orig);
+	} else {
 		while (*next && !isspace(*next))
 			strbuf_addch(arg, *next++);
-	else if (unquote_c_style(arg, next, &next))
-		die("badly quoted argument: %s", next);
+	}
 
-	/* Return position after the argument */
 	return next;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 29391c6..774f8c5 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
 	grep "fatal: badly quoted argument: \\\"master" err
 '
 
-test_expect_success 'stdin fails on arguments not separated by space' '
+test_expect_success 'stdin fails on junk after quoted argument' '
 	echo "create \"$a\"master" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: expected SP but got: master" err
+	grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err
 '
 
 test_expect_success 'stdin fails create with no ref' '

-- 
1.9.1
Previous: Michael HaggertyNext: Michael Haggerty
Message 4 of 34 in “Clean up update-refs --stdin and implement ref_transaction”
  1. 00/27 Clean up update-refs --stdin and implement ref_transactionMichael Haggerty, Apr 7, 2014
  2. 01/27 t1400: fix name and expected result of one testMichael Haggerty, Apr 7, 2014
  3. 02/27 t1400: provide more usual input to the commandMichael Haggerty, Apr 7, 2014
  4. 03/27 parse_arg(): really test that argument is properly terminatedMichael Haggerty, Apr 7, 2014
  5. 04/27 t1400: add some more tests involving quoted argumentsMichael Haggerty, Apr 7, 2014
  6. 05/27 refs.h: rename the action_on_err constantsMichael Haggerty, Apr 7, 2014
  7. 06/27 update_refs(): fix constnessMichael Haggerty, Apr 7, 2014
  8. 07/27 update-ref --stdin: read the whole input at onceMichael Haggerty, Apr 7, 2014
  9. 08/27 parse_cmd_verify(): copy old_sha1 instead of evaluating <oldvalue> twiceMichael Haggerty, Apr 7, 2014
  10. 09/27 update-ref.c: extract a new function, parse_refname()Michael Haggerty, Apr 7, 2014
  11. 10/27 update-ref --stdin: improve error messages for invalid valuesMichael Haggerty, Apr 7, 2014
  12. 11/27 update-ref --stdin: make error messages more consistentMichael Haggerty, Apr 7, 2014
  13. 12/27 update-ref --stdin: simplify error messages for missing oldvaluesMichael Haggerty, Apr 7, 2014
  14. 13/27 t1400: test that stdin -z update treats empty <newvalue> as zerosMichael Haggerty, Apr 7, 2014
  15. 14/27 update-ref.c: extract a new function, parse_next_sha1()Michael Haggerty, Apr 7, 2014
  16. 15/27 update-ref --stdin -z: deprecate interpreting the empty string as zerosMichael Haggerty, Apr 7, 2014
  17. 16/27 t1400: test one mistake at a timeMichael Haggerty, Apr 7, 2014
  18. 17/27 update-ref --stdin: improve the error message for unexpected EOFMichael Haggerty, Apr 7, 2014
  19. 18/27 update-ref --stdin: harmonize error messagesMichael Haggerty, Apr 7, 2014
  20. 19/27 refs: add a concept of a reference transactionMichael Haggerty, Apr 7, 2014
  21. Junio C HamanoApr 7, 2014
  22. Michael HaggertyApr 14, 2014
  23. Junio C HamanoApr 14, 2014
  24. Michael HaggertyApr 15, 2014
  25. Junio C HamanoApr 15, 2014
  26. 20/27 update-ref --stdin: reimplement using reference transactionsMichael Haggerty, Apr 7, 2014
  27. 21/27 refs: remove API function update_refs()Michael Haggerty, Apr 7, 2014
  28. 22/27 struct ref_update: rename field "ref_name" to "refname"Michael Haggerty, Apr 7, 2014
  29. 23/27 struct ref_update: store refname as a FLEX_ARRAYMichael Haggerty, Apr 7, 2014
  30. 24/27 ref_transaction_commit(): simplify code using temporary variablesMichael Haggerty, Apr 7, 2014
  31. 25/27 struct ref_update: add a lock fieldMichael Haggerty, Apr 7, 2014
  32. 26/27 struct ref_update: add a type fieldMichael Haggerty, Apr 7, 2014
  33. 27/27 ref_transaction_commit(): work with transaction->updates in placeMichael Haggerty, Apr 7, 2014
  34. Ronnie SahlbergApr 11, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.