[PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[Rickard Strandqvist]

I have revamped the code so it becomes both more effective and far more clear.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/scsi/scsi_devinfo.c |   31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 49014a1..f6752cc 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -282,27 +282,18 @@ static struct scsi_dev_info_list_table *scsi_devinfo_lookup_by_key(int key)
 static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
 				char *from, int compatible)
 {
-	size_t from_length;
-
-	from_length = strlen(from);
-	strncpy(to, from, min(to_length, from_length));
-	if (from_length < to_length) {
-		if (compatible) {
-			/*
-			 * NUL terminate the string if it is short.
-			 */
-			to[from_length] = '\0';
-		} else {
-			/* 
-			 * space pad the string if it is short. 
-			 */
-			strncpy(&to[from_length], spaces,
-				to_length - from_length);
-		}
-	}
-	if (from_length > to_length)
-		 printk(KERN_WARNING "%s: %s string '%s' is too long\n",
+	strncpy(to, from, to_length);
+	if (to[to_length - 1] != '\0') {
+		to[to_length - 1] = '\0';
+		printk(KERN_WARNING "%s: %s string '%s' is too long\n",
 			__func__, name, from);
+	}
+	if (!compatible) {
+		/*
+		 * space pad the string if it is short.
+		 */
+		strlcat(to, spaces, to_length);
+	}
 }
 
 /**
-- 
1.7.10.4

RE: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Rickard Strandqvist
...
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
...
>  static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
>  				char *from, int compatible)
>  {
> -	size_t from_length;
> -
> -	from_length = strlen(from);
> -	strncpy(to, from, min(to_length, from_length));
> -	if (from_length < to_length) {
> -		if (compatible) {
> -			/*
> -			 * NUL terminate the string if it is short.
> -			 */
> -			to[from_length] = '\0';
> -		} else {
> -			/*
> -			 * space pad the string if it is short.
> -			 */
> -			strncpy(&to[from_length], spaces,
> -				to_length - from_length);
> -		}
> -	}
> -	if (from_length > to_length)
> -		 printk(KERN_WARNING "%s: %s string '%s' is too long\n",
> +	strncpy(to, from, to_length);
> +	if (to[to_length - 1] != '\0') {
> +		to[to_length - 1] = '\0';
> +		printk(KERN_WARNING "%s: %s string '%s' is too long\n",
>  			__func__, name, from);
> +	}

The caller of this function, scsi_dev_info_list_add_keyed, created
the "to" destination buffer, devinfo, with kmalloc, so it's not
guaranteed to be full of zeros.

If from_length is shorter than to_length, then this code will
be inspecting an uninitialized character that strncpy didn't
touch.

---
Rob Elliott    HP Server Storage

Re: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[Rickard Strandqvist]

2014-09-14 23:34 GMT+02:00 Elliott, Robert (Server Storage) <Elliott@hp.com>:
>
>
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Rickard Strandqvist
> ...
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> ...
>>  static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
>>                               char *from, int compatible)
>>  {
>> -     size_t from_length;
>> -
>> -     from_length = strlen(from);
>> -     strncpy(to, from, min(to_length, from_length));
>> -     if (from_length < to_length) {
>> -             if (compatible) {
>> -                     /*
>> -                      * NUL terminate the string if it is short.
>> -                      */
>> -                     to[from_length] = '\0';
>> -             } else {
>> -                     /*
>> -                      * space pad the string if it is short.
>> -                      */
>> -                     strncpy(&to[from_length], spaces,
>> -                             to_length - from_length);
>> -             }
>> -     }
>> -     if (from_length > to_length)
>> -              printk(KERN_WARNING "%s: %s string '%s' is too long\n",
>> +     strncpy(to, from, to_length);
>> +     if (to[to_length - 1] != '\0') {
>> +             to[to_length - 1] = '\0';
>> +             printk(KERN_WARNING "%s: %s string '%s' is too long\n",
>>                       __func__, name, from);
>> +     }
>
> The caller of this function, scsi_dev_info_list_add_keyed, created
> the "to" destination buffer, devinfo, with kmalloc, so it's not
> guaranteed to be full of zeros.
>
> If from_length is shorter than to_length, then this code will
> be inspecting an uninitialized character that strncpy didn't
> touch.
>
> ---
> Rob Elliott    HP Server Storage
>

Hi Elliott

How do you mean?

strncpy zeroes throughout the remainder of the string "from" until the
length off to_length, or otherwise guaranteed trailing zero characters
and a warning is printed.

Is not it exactly the functionality that is desired?

Kind regards
Rickard Strandqvist

RE: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[Elliott, Robert (Server Storage)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 726 bytes --]



> -----Original Message-----
> From: Rickard Strandqvist [mailto:rickard_strandqvist@spectrumdigital.se]
> How do you mean?
> 
> strncpy zeroes throughout the remainder of the string "from" until the
> length off to_length, or otherwise guaranteed trailing zero characters
> and a warning is printed.
> 
> Is not it exactly the functionality that is desired?

Ah, I see that in man 3 strcpy: 
    "If the length of src is less than n, strncpy() pads the 
     remainder of dest with null bytes."

I agree that should work.

---
Rob Elliott    HP Server Storage



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy

[Rickard Strandqvist]

2014-09-15 0:38 GMT+02:00 Elliott, Robert (Server Storage) <Elliott@hp.com>:
>
>
>> -----Original Message-----
>> From: Rickard Strandqvist [mailto:rickard_strandqvist@spectrumdigital.se]
>> How do you mean?
>>
>> strncpy zeroes throughout the remainder of the string "from" until the
>> length off to_length, or otherwise guaranteed trailing zero characters
>> and a warning is printed.
>>
>> Is not it exactly the functionality that is desired?
>
> Ah, I see that in man 3 strcpy:
>     "If the length of src is less than n, strncpy() pads the
>      remainder of dest with null bytes."
>
> I agree that should work.
>
> ---
> Rob Elliott    HP Server Storage


Hi

Okay, good.
Suspected that there was some misunderstanding :)

Kind regards
Rickard Strandqvist