[PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[Giel van Schijndel]

Especially since one very strange piece of code seems to be written in
such a way that a NUL needs to be placed where a NUL is present already.
The author probably meant to fill the last byte of the buffer with a NUL
instead. But regardless of that: that isn't necessary since snprintf()
already guarantees NUL termination for buffers sizes > 0 and <= INT_MAX.
---
 drivers/scsi/ipr.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index df4e27c..b49fe45 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -3990,7 +3990,6 @@ static ssize_t ipr_store_update_fw(struct device *dev,
 		return -EACCES;
 
 	len = snprintf(fname, 99, "%s", buf);
-	fname[len-1] = '\0';
 
 	if (request_firmware(&fw_entry, fname, &ioa_cfg->pdev->dev)) {
 		dev_err(&ioa_cfg->pdev->dev, "Firmware file %s not found\n", fname);
@@ -9358,13 +9357,12 @@ static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
 
 static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
 {
-	int vec_idx, n = sizeof(ioa_cfg->vectors_info[0].desc) - 1;
+	int vec_idx;
 
 	for (vec_idx = 0; vec_idx < ioa_cfg->nvectors; vec_idx++) {
-		snprintf(ioa_cfg->vectors_info[vec_idx].desc, n,
+		snprintf(ioa_cfg->vectors_info[vec_idx].desc,
+			 sizeof(ioa_cfg->vectors_info[vec_idx].desc),
 			 "host%d-%d", ioa_cfg->host->host_no, vec_idx);
-		ioa_cfg->vectors_info[vec_idx].
-			desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;
 	}
 }
 
-- 
2.1.4

Re: [PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[Linus Torvalds]

On Sun, Jan 4, 2015 at 10:05 AM, Giel van Schijndel <me@mortis.eu> wrote:
> Especially since one very strange piece of code seems to be written in
> such a way that a NUL needs to be placed where a NUL is present already.

Actually, it's worse than that. This:

>         len = snprintf(fname, 99, "%s", buf);
> -       fname[len-1] = '\0';

is complete garbage, since the return value of snprintf() is not the
length of the result, but length of what the result *would* have been.

So if the string doesn't fit in 99 bytes, it will actively corrupt
some random memory after the string. It's not writing zero to what was
already zero, it's corrupting memory.

Anyway, from a quick glance your patches look fine, but you need to
sign off on them. See Documentation/SubmittingPatches.

                       Linus

Re: [PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Sun, Jan 04, 2015 at 11:34:43 -0800, Linus Torvalds wrote:
> On Sun, Jan 4, 2015 at 10:05 AM, Giel van Schijndel <me@mortis.eu> wrote:
>> Especially since one very strange piece of code seems to be written in
>> such a way that a NUL needs to be placed where a NUL is present already.
> 
> Actually, it's worse than that. This:
> 
>>         len = snprintf(fname, 99, "%s", buf);
>> -       fname[len-1] = '\0';
> 
> is complete garbage, since the return value of snprintf() is not the
> length of the result, but length of what the result *would* have been.
> 
> So if the string doesn't fit in 99 bytes, it will actively corrupt
> some random memory after the string. It's not writing zero to what was
> already zero, it's corrupting memory.

Ah yes, I didn't even notice that nasty side effect. I just deleted that
"really, really" NUL-termination line because it was based on a
misunderstanding of snprintf()'s postcondition. Even if
len==sizeof(fname) this still would have given the wrong example for
others to follow.

> Anyway, from a quick glance your patches look fine, but you need to
> sign off on them. See Documentation/SubmittingPatches.

Ah yes, forgot that. Would it be sufficient if I sent a reply to all
those patch mails with next line tacked on, or would it require a
resubmission?
> Signed-off-by: Giel van Schijndel <me@mortis.eu>

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Always code as if the guy who ends up maintaining your code will be a
 violent psychopath who knows where you live."
  -- Rick Osborne

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Sun, Jan 04, 2015 at 19:05:58 +0100, Giel van Schijndel wrote:
> Especially since one very strange piece of code seems to be written in
> such a way that a NUL needs to be placed where a NUL is present already.
> The author probably meant to fill the last byte of the buffer with a NUL
> instead. But regardless of that: that isn't necessary since snprintf()
> already guarantees NUL termination for buffers sizes > 0 and <= INT_MAX.
> ---

Forgot to:
Signed-off-by: Giel van Schijndel <me@mortis.eu>

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
Of course I talk to myself. Sometimes I need expert advice.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH RESEND] scsi: cleanup: snprintf() always NUL-terminates: depend on it

[Giel van Schijndel]

Especially since one very strange piece of code seems to be written in
such a way that a NUL needs to be placed where a NUL is present already.
The author probably meant to fill the last byte of the buffer with a NUL
instead. But regardless of that: that isn't necessary since snprintf()
already guarantees NUL termination for buffers sizes > 0 and <= INT_MAX.

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Reported-at: http://www.viva64.com/en/b/0299/
---
 drivers/scsi/ipr.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index df4e27c..b49fe45 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -3990,7 +3990,6 @@ static ssize_t ipr_store_update_fw(struct device *dev,
 		return -EACCES;
 
 	len = snprintf(fname, 99, "%s", buf);
-	fname[len-1] = '\0';
 
 	if (request_firmware(&fw_entry, fname, &ioa_cfg->pdev->dev)) {
 		dev_err(&ioa_cfg->pdev->dev, "Firmware file %s not found\n", fname);
@@ -9358,13 +9357,12 @@ static int ipr_enable_msi(struct ipr_ioa_cfg *ioa_cfg)
 
 static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
 {
-	int vec_idx, n = sizeof(ioa_cfg->vectors_info[0].desc) - 1;
+	int vec_idx;
 
 	for (vec_idx = 0; vec_idx < ioa_cfg->nvectors; vec_idx++) {
-		snprintf(ioa_cfg->vectors_info[vec_idx].desc, n,
+		snprintf(ioa_cfg->vectors_info[vec_idx].desc,
+			 sizeof(ioa_cfg->vectors_info[vec_idx].desc),
 			 "host%d-%d", ioa_cfg->host->host_no, vec_idx);
-		ioa_cfg->vectors_info[vec_idx].
-			desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;
 	}
 }
 
-- 
2.1.4

Re: [PATCH] Cleanup: snprintf() always NUL-terminates: depend on it

[Aleksandr P]

>   len = snprintf(fname, 99, "%s", buf);
> - fname[len-1] = '\0';

> I just deleted that "really, really" NUL-termination line because
> it was based on a misunderstanding of snprintf()'s postcondition.

Are you sure this code can be simple deleted? It does not only
terminate the string but deletes the last character. According to man:
" Upon successful return, these functions return the number of
characters printed (EXCLUDING the null  byte  used  to end output to
strings)."
So, if buf was "abc" len would be 3 and fname would become "ab".

Best regards,
Alexandr