[slabinfo PATCH] Fix off-by-one after readlink() call

[slabinfo PATCH] Fix off-by-one after readlink() call

[Thomas Jarosch]

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
---
 tools/slub/slabinfo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/slub/slabinfo.c b/tools/slub/slabinfo.c
index 868cc93..cc1a378 100644
--- a/tools/slub/slabinfo.c
+++ b/tools/slub/slabinfo.c
@@ -1145,7 +1145,7 @@ static void read_slab_dir(void)
 		switch (de->d_type) {
 		   case DT_LNK:
 			alias->name = strdup(de->d_name);
-			count = readlink(de->d_name, buffer, sizeof(buffer));
+			count = readlink(de->d_name, buffer, sizeof(buffer)-1);
 
 			if (count < 0)
 				fatal("Cannot read symlink %s\n", de->d_name);
-- 
1.7.6.4

Re: [slabinfo PATCH] Fix off-by-one after readlink() call

[Christoph Lameter]

On Fri, 14 Oct 2011, Thomas Jarosch wrote:

> index 868cc93..cc1a378 100644
> --- a/tools/slub/slabinfo.c
> +++ b/tools/slub/slabinfo.c
> @@ -1145,7 +1145,7 @@ static void read_slab_dir(void)
>  		switch (de->d_type) {
>  		   case DT_LNK:
>  			alias->name = strdup(de->d_name);
> -			count = readlink(de->d_name, buffer, sizeof(buffer));
> +			count = readlink(de->d_name, buffer, sizeof(buffer)-1);
>

readlink required that the buffer size be specified.

READLINK(2)                                              Linux Programmer's Manual                                              READLINK(2)

NAME
       readlink - read value of a symbolic link

SYNOPSIS
       #include <unistd.h>

       ssize_t readlink(const char *path, char *buf, size_t bufsiz);

   Feature Test Macro Requirements for glibc (see feature_test_macros(7)):

       readlink(): _BSD_SOURCE || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L

DESCRIPTION
       readlink()  places  the  contents  of the symbolic link path in the buffer buf, which has size bufsiz.  readlink() does not append a
       null byte to buf.  It will truncate the contents (to a length of bufsiz characters), in case the buffer is too small to hold all  of
       the contents.

Re: [slabinfo PATCH] Fix off-by-one after readlink() call

[Thomas Jarosch]

On 10/14/2011 07:16 PM, Christoph Lameter wrote:
>> index 868cc93..cc1a378 100644
>> --- a/tools/slub/slabinfo.c
>> +++ b/tools/slub/slabinfo.c
>> @@ -1145,7 +1145,7 @@ static void read_slab_dir(void)
>>  		switch (de->d_type) {
>>  		   case DT_LNK:
>>  			alias->name = strdup(de->d_name);
>> -			count = readlink(de->d_name, buffer, sizeof(buffer));
>> +			count = readlink(de->d_name, buffer, sizeof(buffer)-1);
>>
> 
> DESCRIPTION
>        readlink()  places  the  contents  of the symbolic link path in the buffer buf, which has size bufsiz.  readlink() does not append a
>        null byte to buf.  It will truncate the contents (to a length of bufsiz characters), in case the buffer is too small to hold all  of
>        the contents.

The problem is the line after the readlink() call:

buffer[count] = '\0';

The common technique is to reduce the buffer size by one.
Another fix would be to check

"
if (count < 0 || count == sizeof(buffer))
    fatal();
"

Reducing the buffer size by one is easier IMHO.

Cheers,
Thomas

Re: [slabinfo PATCH] Fix off-by-one after readlink() call

[Christoph Lameter]

On Fri, 14 Oct 2011, Thomas Jarosch wrote:

> Reducing the buffer size by one is easier IMHO.

Ok.

Acked-by: Christoph Lameter <cl@gentwo.org>

Re: [slabinfo PATCH] Fix off-by-one after readlink() call

[David Rientjes]

On Fri, 14 Oct 2011, Thomas Jarosch wrote:

> The problem is the line after the readlink() call:
> 
> buffer[count] = '\0';
> 
> The common technique is to reduce the buffer size by one.
> Another fix would be to check
> 
> "
> if (count < 0 || count == sizeof(buffer))
>     fatal();
> "
> 
> Reducing the buffer size by one is easier IMHO.
> 

I think this should be used for changelog.

Acked-by: David Rientjes <rientjes@google.com>

Re: [slabinfo PATCH] Fix off-by-one after readlink() call

[Pekka Enberg]

On Fri, Oct 14, 2011 at 10:53 PM, David Rientjes <rientjes@google.com> wrote:
> On Fri, 14 Oct 2011, Thomas Jarosch wrote:
>
>> The problem is the line after the readlink() call:
>>
>> buffer[count] = '\0';
>>
>> The common technique is to reduce the buffer size by one.
>> Another fix would be to check
>>
>> "
>> if (count < 0 || count == sizeof(buffer))
>>     fatal();
>> "
>>
>> Reducing the buffer size by one is easier IMHO.
>>
>
> I think this should be used for changelog.
>
> Acked-by: David Rientjes <rientjes@google.com>

Thomas, please resend with proper changelog and with David's and
Christoph's ACKs included.

[slabinfo PATCH v2] Fix off-by-one buffer corruption after readlink() call

[Thomas Jarosch]

readlink() never zero terminates the provided buffer.
Therefore we already do

    buffer[count] = 0;

This leads to an off-by-one buffer corruption as readlink()
might return the full size of the buffer.

The common technique is to reduce the buffer size by one.
Another fix would be to check

"
if (count < 0 || count == sizeof(buffer))
    fatal();
"

Reducing the buffer size by one is easier IMHO.

Signed-off-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
Acked-by: David Rientjes <rientjes@google.com>
Acked-by: Christoph Lameter <cl@gentwo.org>
---
 tools/slub/slabinfo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/slub/slabinfo.c b/tools/slub/slabinfo.c
index 868cc93..cc1a378 100644
--- a/tools/slub/slabinfo.c
+++ b/tools/slub/slabinfo.c
@@ -1145,7 +1145,7 @@ static void read_slab_dir(void)
 		switch (de->d_type) {
 		   case DT_LNK:
 			alias->name = strdup(de->d_name);
-			count = readlink(de->d_name, buffer, sizeof(buffer));
+			count = readlink(de->d_name, buffer, sizeof(buffer)-1);
 
 			if (count < 0)
 				fatal("Cannot read symlink %s\n", de->d_name);
-- 
1.7.6.4