[PATCH] xfsprogs: properly terminate string in quota's restore_file()

[PATCH] xfsprogs: properly terminate string in quota's restore_file()

[Eric Sandeen]

This code copies up to the entire size of devbuffer, and then
tries to use "strlen" to null terminate it.

But strlen works by *finding* the null, so it's at best a
no-op, and at worst not properly terminating the string.

Fix this by placing the null at the last byte of the buffer.

Addresses-Coverity-Id: 1297519
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/quota/edit.c b/quota/edit.c
index d226e89..a53a7e6 100644
--- a/quota/edit.c
+++ b/quota/edit.c
@@ -385,7 +385,7 @@ restore_file(
 	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
 		if (strncmp("fs = ", buffer, 5) == 0) {
 			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
-			dev[strlen(dev) - 1] = '\0';
+			dev[sizeof(devbuffer) - 1] = '\0';
 			continue;
 		}
 		rtbsoft = rtbhard = 0;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: properly terminate string in quota's restore_file()

[Brian Foster]

On Tue, Aug 25, 2015 at 11:39:29AM -0500, Eric Sandeen wrote:
> This code copies up to the entire size of devbuffer, and then
> tries to use "strlen" to null terminate it.
> 
> But strlen works by *finding* the null, so it's at best a
> no-op, and at worst not properly terminating the string.
> 
> Fix this by placing the null at the last byte of the buffer.
> 
> Addresses-Coverity-Id: 1297519
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/quota/edit.c b/quota/edit.c
> index d226e89..a53a7e6 100644
> --- a/quota/edit.c
> +++ b/quota/edit.c
> @@ -385,7 +385,7 @@ restore_file(
>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>  		if (strncmp("fs = ", buffer, 5) == 0) {
>  			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
> -			dev[strlen(dev) - 1] = '\0';
> +			dev[sizeof(devbuffer) - 1] = '\0';

According to the man page, fgets() NULL terminates the provided buffer.
Next, we attempt to strncpy() just the device name part of the string
(copying up to 512 bytes from a 512-5 byte buffer). I'm not quite sure,
but it looks like the above line could be trying to replace a newline
with a NULL terminator..? E.g., it expects the last character in an
already NULL terminated line to be a newline.

Brian

>  			continue;
>  		}
>  		rtbsoft = rtbhard = 0;
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: properly terminate string in quota's restore_file()

[Eric Sandeen]

On 8/26/15 6:53 AM, Brian Foster wrote:
> On Tue, Aug 25, 2015 at 11:39:29AM -0500, Eric Sandeen wrote:
>> This code copies up to the entire size of devbuffer, and then
>> tries to use "strlen" to null terminate it.
>>
>> But strlen works by *finding* the null, so it's at best a
>> no-op, and at worst not properly terminating the string.
>>
>> Fix this by placing the null at the last byte of the buffer.
>>
>> Addresses-Coverity-Id: 1297519
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/quota/edit.c b/quota/edit.c
>> index d226e89..a53a7e6 100644
>> --- a/quota/edit.c
>> +++ b/quota/edit.c
>> @@ -385,7 +385,7 @@ restore_file(
>>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>>  		if (strncmp("fs = ", buffer, 5) == 0) {
>>  			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
>> -			dev[strlen(dev) - 1] = '\0';
>> +			dev[sizeof(devbuffer) - 1] = '\0';
> 
> According to the man page, fgets() NULL terminates the provided buffer.

Yep, I should have looked more closely.

It copies at most size-1 (511 bytes), including the newline if it fits,
and terminates with \0.

> Next, we attempt to strncpy() just the device name part of the string
> (copying up to 512 bytes from a 512-5 byte buffer).

In fact since the original buffer (via fgets, 512) will be <= 512 long,
copying again from 5 bytes in will always result in less than 512 bytes
copied.

> I'm not quite sure,
> but it looks like the above line could be trying to replace a newline
> with a NULL terminator..? E.g., it expects the last character in an
> already NULL terminated line to be a newline.

Yes, that's exactly it, thanks.

There is one very corner case where if the last char in the buffer is
part of the device name, the buffer won't contain the newline, and the
'\0' will actually take out one char, rather than the newline.  But
that's one heck of a corner case, the exactly-sized-device-name case ...
probably not worth worrying about, though I might send a patch just to
satisfy my OCD.

Thanks for keeping me straight on this one, sorry for the noise.

-Eric

> Brian
> 
>>  			continue;
>>  		}
>>  		rtbsoft = rtbhard = 0;
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs