[PATCH 3/3] index-helper: take extra care with readlink

[PATCH 3/3] index-helper: take extra care with readlink

[Ramsay Jones]

It took me a few minutes to convince myself that, if index-helper is
the only writer for the symlink, that the call to readlink would
result in a properly NULL terminated string. This relies on the
initialization of the address variable setting each byte of the
'struct sockaddr_un' to zero and the contents of the symlink being
no more than 'sizeof(address.sun_path) - 1' bytes long.

In order to make the call easier to reason about, introduce a wrapper
function, read_link(), which copes with overlong symlinks and ensures
correct NULL termination.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 read-cache.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index c7053d8..39330a1 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1722,12 +1722,23 @@ static void post_read_index_from(struct index_state *istate)
 	tweak_untracked_cache(istate);
 }
 
+static int read_link(const char *path, char *buf, size_t bufsize)
+{
+	int len;
+
+	len = readlink(path, buf, bufsize);
+	if (len < 0 || len >= bufsize)
+		return -1;
+	buf[len] = '\0';
+	return 0;
+}
+
 int connect_to_index_helper(void)
 {
 	struct sockaddr_un address = {0};
 	int fd;
 
-	if (readlink(git_path("index-helper.path"), address.sun_path,
+	if (read_link(git_path("index-helper.path"), address.sun_path,
 		     sizeof(address.sun_path)) < 0)
 		return -1;
 
-- 
2.8.0

Re: [PATCH 3/3] index-helper: take extra care with readlink

[Torsten Bögershausen]

On 04/11/2016 01:03 AM, Ramsay Jones wrote:
> It took me a few minutes to convince myself that, if index-helper is
> the only writer for the symlink, that the call to readlink would
> result in a properly NULL terminated string. This relies on the
>
Minor nit:
s/NULL/NUL/

Re: [PATCH 3/3] index-helper: take extra care with readlink

[David Turner]

On Mon, 2016-04-11 at 00:03 +0100, Ramsay Jones wrote:
> It took me a few minutes to convince myself that, if index-helper is
> the only writer for the symlink, that the call to readlink would
> result in a properly NULL terminated string. This relies on the
> initialization of the address variable setting each byte of the
> 'struct sockaddr_un' to zero and the contents of the symlink being
> no more than 'sizeof(address.sun_path) - 1' bytes long.
> 
> In order to make the call easier to reason about, introduce a wrapper
> function, read_link(), which copes with overlong symlinks and ensures
> correct NULL termination.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  read-cache.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index c7053d8..39330a1 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1722,12 +1722,23 @@ static void post_read_index_from(struct
> index_state *istate)
>  	tweak_untracked_cache(istate);
>  }
>  
> +static int read_link(const char *path, char *buf, size_t bufsize)
> +{
> +	int len;
> +
> +	len = readlink(path, buf, bufsize);
> +	if (len < 0 || len >= bufsize)
> +		return -1;
> +	buf[len] = '\0';
> +	return 0;
> +}
> +
>  int connect_to_index_helper(void)
>  {
>  	struct sockaddr_un address = {0};
>  	int fd;
>  
> -	if (readlink(git_path("index-helper.path"),
> address.sun_path,
> +	if (read_link(git_path("index-helper.path"),
> address.sun_path,
>  		     sizeof(address.sun_path)) < 0)
>  		return -1;
>  


Will squash.