[PATCH] nul-terminate readlink result

[PATCH] nul-terminate readlink result

[Tobias Stoeckmann]

readlink by itself does not guarantee that its result is properly
nul-terminated. Setting the last byte of the buffer to '\0' fixes
this issue.
---
 libkms/linux.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libkms/linux.c b/libkms/linux.c
index 4d47148..667d37c 100644
--- a/libkms/linux.c
+++ b/libkms/linux.c
@@ -82,6 +82,7 @@ linux_name_from_sysfs(int fd, char **out)
 
 	if (readlink(path, link, PATH_SIZE) < 0)
 		return -EINVAL;
+	link[PATH_SIZE] = '\0';
 
 	/* link looks something like this: ../../../bus/pci/drivers/intel */
 	slash_name = strrchr(link, '/');
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] nul-terminate readlink result

[Tobias Stoeckmann]

On Sat, Jul 11, 2015 at 11:02:57AM +0200, Tobias Stoeckmann wrote:
>  	if (readlink(path, link, PATH_SIZE) < 0)
>  		return -EINVAL;
> +	link[PATH_SIZE] = '\0';

Sorry, this patch is wrong. It has to be terminated at the correct
position. Please see this updated version:


From a1840916dd370d5c84ebf23df91f1c13d564d212 Mon Sep 17 00:00:00 2001
From: Tobias Stoeckmann <tobias@stoeckmann.org>
Date: Sun, 12 Jul 2015 12:10:59 +0200
Subject: [PATCH] nul-terminate readlink result

readlink by itself does not terminate its result. The caller has
to terminate the string at the proper position.
---
 libkms/linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libkms/linux.c b/libkms/linux.c
index 4d47148..5e2431f 100644
--- a/libkms/linux.c
+++ b/libkms/linux.c
@@ -55,6 +55,7 @@ linux_name_from_sysfs(int fd, char **out)
 	unsigned maj, min;
 	char* slash_name;
 	int ret;
+	ssize_t len;
 
 	/* 
 	 * Inside the sysfs directory for the device there is a symlink
@@ -80,8 +81,9 @@ linux_name_from_sysfs(int fd, char **out)
 
 	snprintf(path, PATH_SIZE, "/sys/dev/char/%d:%d/device/driver", maj, min);
 
-	if (readlink(path, link, PATH_SIZE) < 0)
+	if ((len = readlink(path, link, PATH_SIZE)) < 0)
 		return -EINVAL;
+	link[len] = '\0';
 
 	/* link looks something like this: ../../../bus/pci/drivers/intel */
 	slash_name = strrchr(link, '/');
-- 
2.4.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] nul-terminate readlink result

[Emil Velikov]

Hi Tobias,

On 12 July 2015 at 11:14, Tobias Stoeckmann <tobias@stoeckmann.org> wrote:
> On Sat, Jul 11, 2015 at 11:02:57AM +0200, Tobias Stoeckmann wrote:
>>       if (readlink(path, link, PATH_SIZE) < 0)
>>               return -EINVAL;
>> +     link[PATH_SIZE] = '\0';
>
> Sorry, this patch is wrong. It has to be terminated at the correct
> position. Please see this updated version:
>
Thanks for the update, there is a pick I'm afraid:

>
> From a1840916dd370d5c84ebf23df91f1c13d564d212 Mon Sep 17 00:00:00 2001
> From: Tobias Stoeckmann <tobias@stoeckmann.org>
> Date: Sun, 12 Jul 2015 12:10:59 +0200
> Subject: [PATCH] nul-terminate readlink result
>
> readlink by itself does not terminate its result. The caller has
> to terminate the string at the proper position.
> ---
>  libkms/linux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libkms/linux.c b/libkms/linux.c
> index 4d47148..5e2431f 100644
> --- a/libkms/linux.c
> +++ b/libkms/linux.c
> @@ -55,6 +55,7 @@ linux_name_from_sysfs(int fd, char **out)
>         unsigned maj, min;
>         char* slash_name;
>         int ret;
> +       ssize_t len;
>
>         /*
>          * Inside the sysfs directory for the device there is a symlink
> @@ -80,8 +81,9 @@ linux_name_from_sysfs(int fd, char **out)
>
>         snprintf(path, PATH_SIZE, "/sys/dev/char/%d:%d/device/driver", maj, min);
>
> -       if (readlink(path, link, PATH_SIZE) < 0)
> +       if ((len = readlink(path, link, PATH_SIZE)) < 0)
>                 return -EINVAL;
> +       link[len] = '\0';
The man page does not say it -1 + errno is returned when the buffer is
too small.

There is ENAMETOOLONG which reads "A pathname, or a component of a
pathname, was too long.". At the same time the example given
explicitly checks for len >= PATH_SIZE.

It uses lstat + malloc over a static storage. Not sure how much this
matters here though.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel