git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] expand_user_path: do not look at NULL path
@ 2014-01-28  1:36 Jeff King
  2014-01-28  1:37 ` [PATCH 2/2] handle_path_include: don't look at NULL value Jeff King
  0 siblings, 1 reply; 2+ messages in thread
From: Jeff King @ 2014-01-28  1:36 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

We explicitly check for and handle the case that the
incoming "path" variable is NULL, but before doing so we
call strchrnul on it, leading to a potential segfault.

We can fix this simply by moving the strchrnul call down; as
a bonus, we can tighten the scope on the associated
variable.

Signed-off-by: Jeff King <peff@peff.net>
---
Most of the callers already check for NULL, so the only way to trigger
this is via an empty "include.path". That is addressed separately in the
next patch, so technically this is not needed if we apply that one. But
in that case, the "path == NULL" check here is useless, so I think this
makes things safer overall.

 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index 24594c4..f9c5062 100644
--- a/path.c
+++ b/path.c
@@ -265,12 +265,12 @@ static struct passwd *getpw_str(const char *username, size_t len)
 char *expand_user_path(const char *path)
 {
 	struct strbuf user_path = STRBUF_INIT;
-	const char *first_slash = strchrnul(path, '/');
 	const char *to_copy = path;
 
 	if (path == NULL)
 		goto return_null;
 	if (path[0] == '~') {
+		const char *first_slash = strchrnul(path, '/');
 		const char *username = path + 1;
 		size_t username_len = first_slash - username;
 		if (username_len == 0) {
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 2+ messages in thread

* [PATCH 2/2] handle_path_include: don't look at NULL value
  2014-01-28  1:36 [PATCH 1/2] expand_user_path: do not look at NULL path Jeff King
@ 2014-01-28  1:37 ` Jeff King
  0 siblings, 0 replies; 2+ messages in thread
From: Jeff King @ 2014-01-28  1:37 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

When we see config like:

  [include]
  path

the expand_user_path helper notices that the config value is
empty, but we then dereference NULL while printing the error
message (glibc will helpfully print "(null)" for us here,
but we cannot rely on that).

  $ git -c include.path rev-parse
  error: Could not expand include path '(null)'
  fatal: unable to parse command-line config

Instead of tweaking our message, let's actually use
config_error_nonbool to match other config variables that
expect a value:

  $ git -c include.path rev-parse
  error: Missing value for 'include.path'
  fatal: unable to parse command-line config

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index d969a5a..314d8ee 100644
--- a/config.c
+++ b/config.c
@@ -84,8 +84,12 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 {
 	int ret = 0;
 	struct strbuf buf = STRBUF_INIT;
-	char *expanded = expand_user_path(path);
+	char *expanded;
 
+	if (!path)
+		return config_error_nonbool("include.path");
+
+	expanded = expand_user_path(path);
 	if (!expanded)
 		return error("Could not expand include path '%s'", path);
 	path = expanded;
-- 
1.8.5.2.500.g8060133

^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2014-01-28  1:37 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-28  1:36 [PATCH 1/2] expand_user_path: do not look at NULL path Jeff King
2014-01-28  1:37 ` [PATCH 2/2] handle_path_include: don't look at NULL value Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).