{"thread":{"id":"35745","subject":"[PATCH 1/2] expand_user_path: do not look at NULL path","startedAt":"2014-01-28T01:36:12Z","lastAt":"2014-01-28T01:37:30Z","messageCount":2,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"233858","messageId":"20140128013612.GA29548@sigill.intra.peff.net","threadId":"35745","inReplyTo":null,"subject":"[PATCH 1/2] expand_user_path: do not look at NULL path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-28T01:36:12Z","receivedAt":"2014-01-28T01:36:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We explicitly check for and handle the case that the\nincoming \"path\" variable is NULL, but before doing so we\ncall strchrnul on it, leading to a potential segfault.\n\nWe can fix this simply by moving the strchrnul call down; as\na bonus, we can tighten the scope on the associated\nvariable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMost of the callers already check for NULL, so the only way to trigger\nthis is via an empty \"include.path\". That is addressed separately in the\nnext patch, so technically this is not needed if we apply that one. But\nin that case, the \"path == NULL\" check here is useless, so I think this\nmakes things safer overall.\n\n path.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex 24594c4..f9c5062 100644\n--- a/path.c\n+++ b/path.c\n@@ -265,12 +265,12 @@ static struct passwd *getpw_str(const char *username, size_t len)\n char *expand_user_path(const char *path)\n {\n \tstruct strbuf user_path = STRBUF_INIT;\n-\tconst char *first_slash = strchrnul(path, '/');\n \tconst char *to_copy = path;\n \n \tif (path == NULL)\n \t\tgoto return_null;\n \tif (path[0] == '~') {\n+\t\tconst char *first_slash = strchrnul(path, '/');\n \t\tconst char *username = path + 1;\n \t\tsize_t username_len = first_slash - username;\n \t\tif (username_len == 0) {\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233859","messageId":"20140128013729.GA21120@sigill.intra.peff.net","threadId":"35745","inReplyTo":"20140128013612.GA29548@sigill.intra.peff.net","subject":"[PATCH 2/2] handle_path_include: don't look at NULL value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-28T01:37:30Z","receivedAt":"2014-01-28T01:37:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see config like:\n\n  [include]\n  path\n\nthe expand_user_path helper notices that the config value is\nempty, but we then dereference NULL while printing the error\nmessage (glibc will helpfully print \"(null)\" for us here,\nbut we cannot rely on that).\n\n  $ git -c include.path rev-parse\n  error: Could not expand include path '(null)'\n  fatal: unable to parse command-line config\n\nInstead of tweaking our message, let's actually use\nconfig_error_nonbool to match other config variables that\nexpect a value:\n\n  $ git -c include.path rev-parse\n  error: Missing value for 'include.path'\n  fatal: unable to parse command-line config\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex d969a5a..314d8ee 100644\n--- a/config.c\n+++ b/config.c\n@@ -84,8 +84,12 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n {\n \tint ret = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *expanded = expand_user_path(path);\n+\tchar *expanded;\n \n+\tif (!path)\n+\t\treturn config_error_nonbool(\"include.path\");\n+\n+\texpanded = expand_user_path(path);\n \tif (!expanded)\n \t\treturn error(\"Could not expand include path '%s'\", path);\n \tpath = expanded;\n-- \n1.8.5.2.500.g8060133\n"}]}