[PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[Arnd Bergmann]

gcc-8 warns about a code pattern that is used in the newly added
test_rhashtable code:

lib/test_rhashtable.c: In function 'print_ht':
lib/test_rhashtable.c:511:21: error: '
bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
                     ^~~~~~~~~
lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem here is using the same fixed-length buffer as input and output
of snprintf(), which for an unbounded loop has an actual potential to
overflow the buffer. The '512' byte length was apparently chosen to
be "long enough" to prevent that in practice, but without any specific
guarantees of being the smallest safe size.

I can see three possible ways to avoid this warning:

- rewrite the code to use pointer arithmetic to forward the buffer,
  rather than copying the buffer itself. This is a more conventional
  use of sprintf(), and it avoids the warning, but is not any more
  safe than the original code.
- Rewrite the function in a safe way that avoids both the potential
  overflow and the warning.
- Ask the gcc developers to not warn for this pattern if we consider
  the warning to be inappropriate.

This patch implements the first of the above, as an illustration of
the problem, and the simplest workaround.

Fixes: 499ac3b60f65 ("test_rhashtable: add test case for rhltable with duplicate objects")
Cc: Martin Sebor <msebor@gcc.gnu.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
My patch is untested, please try it out before applying.
---
 lib/test_rhashtable.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index f4000c137dbe..a0f4fb03d2de 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -496,6 +496,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 	struct rhashtable *ht;
 	const struct bucket_table *tbl;
 	char buff[512] = "";
+	char *buffp = buff;
 	unsigned int i, cnt = 0;
 
 	ht = &rhlt->ht;
@@ -508,18 +509,18 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 		next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) : NULL;
 
 		if (!rht_is_a_nulls(pos)) {
-			sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
+			buffp += sprintf(buffp, "\nbucket[%d] -> ", i);
 		}
 
 		while (!rht_is_a_nulls(pos)) {
 			struct rhlist_head *list = container_of(pos, struct rhlist_head, rhead);
-			sprintf(buff, "%s[[", buff);
+			buffp += sprintf(buffp, "[[");
 			do {
 				pos = &list->rhead;
 				list = rht_dereference(list->next, ht);
 				p = rht_obj(ht, pos);
 
-				sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
+				buffp += sprintf(buffp, "val %d (tid=%d)%s", p->value.id, p->value.tid,
 					list? ", " : " ");
 				cnt++;
 			} while (list);
@@ -528,7 +529,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 			next = !rht_is_a_nulls(pos) ?
 				rht_dereference(pos->next, ht) : NULL;
 
-			sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? " -> " : "");
+			buffp += sprintf(buffp, "]]%s", !rht_is_a_nulls(pos) ? " -> " : "");
 		}
 	}
 	printk(KERN_ERR "\n---- ht: ----%s\n-------------\n", buff);
-- 
2.9.0

Re: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[Rasmus Villemoes]

On 2018-03-13 14:21, Arnd Bergmann wrote:
> gcc-8 warns about a code pattern that is used in the newly added
> test_rhashtable code:
> 
> lib/test_rhashtable.c: In function 'print_ht':
> lib/test_rhashtable.c:511:21: error: '
> bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>                      ^~~~~~~~~
> lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The problem here is using the same fixed-length buffer as input and output
> of snprintf(), which for an unbounded loop has an actual potential to
> overflow the buffer. The '512' byte length was apparently chosen to
> be "long enough" to prevent that in practice, but without any specific
> guarantees of being the smallest safe size.

well, 1024 would certainly be enough, because the result is anyway
passed to printk() which formats into a buffer of that size, so anything
more would certainly just be thrown away...

> I can see three possible ways to avoid this warning:
> 
> - rewrite the code to use pointer arithmetic to forward the buffer,
>   rather than copying the buffer itself. This is a more conventional
>   use of sprintf(), and it avoids the warning, but is not any more
>   safe than the original code.
> - Rewrite the function in a safe way that avoids both the potential
>   overflow and the warning.
> - Ask the gcc developers to not warn for this pattern if we consider
>   the warning to be inappropriate.
> 
> This patch implements the first of the above, as an illustration of
> the problem, and the simplest workaround.

If you use scnprintf() and forward the printed length you can get rid of
the potential buffer overrun:

len = 0;
...
len += scnprintf(buf + len, sizeof(buf) - len, fmt, args...)

scnprintf has the property that if you pass in a positive value, you get
back something that is strictly less, so with the above pattern, we
might eventually have sizeof(buf)-len==1, so all subsequent scnprintfs
return 0, but we never overflow the buffer. The effect is thus the same
as if you had done all the formatting with a single snprintf() call.

FWIW, I sent an RFC [1] two years ago trying to get rid of all
snprintf(buf, ..., "%s...", buf, ...), because I think it's too fragile
(it obviously breaks horribly if anything precedes the %s with buf as
its argument), but others disagreed and said that the kernel's
vsnprintf() instead should be documented to support that special case of
overlapping src and dst. I don't really recall what happened with the
patches, perhaps some got applied, but if not, maybe gcc-8 will now warn
about those places.

[1]
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1096481.html


> Fixes: 499ac3b60f65 ("test_rhashtable: add test case for rhltable with duplicate objects")
> Cc: Martin Sebor <msebor@gcc.gnu.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> My patch is untested, please try it out before applying.
> ---
>  lib/test_rhashtable.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> index f4000c137dbe..a0f4fb03d2de 100644
> --- a/lib/test_rhashtable.c
> +++ b/lib/test_rhashtable.c
> @@ -496,6 +496,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  	struct rhashtable *ht;
>  	const struct bucket_table *tbl;
>  	char buff[512] = "";
> +	char *buffp = buff;
>  	unsigned int i, cnt = 0;
>  
>  	ht = &rhlt->ht;
> @@ -508,18 +509,18 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  		next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) : NULL;
>  
>  		if (!rht_is_a_nulls(pos)) {
> -			sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
> +			buffp += sprintf(buffp, "\nbucket[%d] -> ", i);
>  		}
>  
>  		while (!rht_is_a_nulls(pos)) {
>  			struct rhlist_head *list = container_of(pos, struct rhlist_head, rhead);
> -			sprintf(buff, "%s[[", buff);
> +			buffp += sprintf(buffp, "[[");
>  			do {
>  				pos = &list->rhead;
>  				list = rht_dereference(list->next, ht);
>  				p = rht_obj(ht, pos);
>  
> -				sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
> +				buffp += sprintf(buffp, "val %d (tid=%d)%s", p->value.id, p->value.tid,
>  					list? ", " : " ");

this removes a space before val, not sure that was intended?

>  				cnt++;
>  			} while (list);
> @@ -528,7 +529,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  			next = !rht_is_a_nulls(pos) ?
>  				rht_dereference(pos->next, ht) : NULL;
>  
> -			sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? " -> " : "");
> +			buffp += sprintf(buffp, "]]%s", !rht_is_a_nulls(pos) ? " -> " : "");
>  		}
>  	}
>  	printk(KERN_ERR "\n---- ht: ----%s\n-------------\n", buff);
> 

Re: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[Arnd Bergmann]

On Tue, Mar 13, 2018 at 3:41 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-03-13 14:21, Arnd Bergmann wrote:
>> gcc-8 warns about a code pattern that is used in the newly added
>> test_rhashtable code:
>>
>> lib/test_rhashtable.c: In function 'print_ht':
>> lib/test_rhashtable.c:511:21: error: '
>> bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
>>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>>                      ^~~~~~~~~
>> lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
>>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> The problem here is using the same fixed-length buffer as input and output
>> of snprintf(), which for an unbounded loop has an actual potential to
>> overflow the buffer. The '512' byte length was apparently chosen to
>> be "long enough" to prevent that in practice, but without any specific
>> guarantees of being the smallest safe size.
>
> well, 1024 would certainly be enough, because the result is anyway
> passed to printk() which formats into a buffer of that size, so anything
> more would certainly just be thrown away...

I was only worried about overflowing the stack here, not about the
output making sense ;-)

>> I can see three possible ways to avoid this warning:
>>
>> - rewrite the code to use pointer arithmetic to forward the buffer,
>>   rather than copying the buffer itself. This is a more conventional
>>   use of sprintf(), and it avoids the warning, but is not any more
>>   safe than the original code.
>> - Rewrite the function in a safe way that avoids both the potential
>>   overflow and the warning.
>> - Ask the gcc developers to not warn for this pattern if we consider
>>   the warning to be inappropriate.
>>
>> This patch implements the first of the above, as an illustration of
>> the problem, and the simplest workaround.
>
> If you use scnprintf() and forward the printed length you can get rid of
> the potential buffer overrun:
>
> len = 0;
> ...
> len += scnprintf(buf + len, sizeof(buf) - len, fmt, args...)
>
> scnprintf has the property that if you pass in a positive value, you get
> back something that is strictly less, so with the above pattern, we
> might eventually have sizeof(buf)-len==1, so all subsequent scnprintfs
> return 0, but we never overflow the buffer. The effect is thus the same
> as if you had done all the formatting with a single snprintf() call.

Right, that was my second proposal above. Using scnprintf(),
that's probably easier than I first thought.

> FWIW, I sent an RFC [1] two years ago trying to get rid of all
> snprintf(buf, ..., "%s...", buf, ...), because I think it's too fragile
> (it obviously breaks horribly if anything precedes the %s with buf as
> its argument), but others disagreed and said that the kernel's
> vsnprintf() instead should be documented to support that special case of
> overlapping src and dst. I don't really recall what happened with the
> patches, perhaps some got applied, but if not, maybe gcc-8 will now warn
> about those places.
>
> [1]
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1096481.html

It looks useful, but not all seem to have landed. I think you
are referring to Andrew's feedback in
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1097554.html
here as the concern about whether it did the right thing, but I'm not
actually sure what his reply meant.

In the case of the analog_name() function that Andrew commented on,
Dmitry later fixed the overflow in a different way in commit
10ca4c0a622a ("Input: fix potential overflows in driver/input/joystick").

>> +                     buffp += sprintf(buffp, "[[");
>>                       do {
>>                               pos = &list->rhead;
>>                               list = rht_dereference(list->next, ht);
>>                               p = rht_obj(ht, pos);
>>
>> -                             sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
>> +                             buffp += sprintf(buffp, "val %d (tid=%d)%s", p->value.id, p->value.tid,
>>                                       list? ", " : " ");
>
> this removes a space before val, not sure that was intended?

It was not, I'll fix it up. Thanks for taking a closer look!

     Arnd

[PATCH] [v2] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[Arnd Bergmann]

gcc-8 warns about a code pattern that is used in the newly added
test_rhashtable code:

lib/test_rhashtable.c: In function 'print_ht':
lib/test_rhashtable.c:511:21: error: '
bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
                     ^~~~~~~~~
lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem here is using the same fixed-length buffer as input and output
of snprintf(), which for an unbounded loop has an actual potential to
overflow the buffer. The '512' byte length was apparently chosen to
be "long enough" to prevent that in practice, but without any specific
guarantees of being the smallest safe size.

I can see three possible ways to avoid this warning:

- rewrite the code to use pointer arithmetic to forward the buffer,
  rather than copying the buffer itself. This is a more conventional
  use of sprintf(), and it avoids the warning, but is not any more
  safe than the original code.
- Rewrite the function in a safe way that avoids both the potential
  overflow and the warning.
- Ask the gcc developers to not warn for this pattern if we consider
  the warning to be inappropriate.

This patch implements the second of the above, using scnprintf()
as Rasmus suggested.

Fixes: 499ac3b60f65 ("test_rhashtable: add test case for rhltable with duplicate objects")
Cc: Martin Sebor <msebor@gcc.gnu.org>
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
My patch is still untested, please try it out before applying.
---
 lib/test_rhashtable.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index f4000c137dbe..fa865a953176 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -496,6 +496,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 	struct rhashtable *ht;
 	const struct bucket_table *tbl;
 	char buff[512] = "";
+	size_t len = 0;
 	unsigned int i, cnt = 0;
 
 	ht = &rhlt->ht;
@@ -508,19 +509,19 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 		next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) : NULL;
 
 		if (!rht_is_a_nulls(pos)) {
-			sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
+			len += scnprintf(buff + len, sizeof(buff) - len, "\nbucket[%d] -> ", i);
 		}
 
 		while (!rht_is_a_nulls(pos)) {
 			struct rhlist_head *list = container_of(pos, struct rhlist_head, rhead);
-			sprintf(buff, "%s[[", buff);
+			len += scnprintf(buff + len, sizeof(buff) - len, "[[");
 			do {
 				pos = &list->rhead;
 				list = rht_dereference(list->next, ht);
 				p = rht_obj(ht, pos);
 
-				sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
-					list? ", " : " ");
+				len += scnprintf(buff + len, sizeof(buff) - len, " val %d (tid=%d)%s",
+						 p->value.id, p->value.tid, list? ", " : " ");
 				cnt++;
 			} while (list);
 
@@ -528,7 +529,8 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 			next = !rht_is_a_nulls(pos) ?
 				rht_dereference(pos->next, ht) : NULL;
 
-			sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? " -> " : "");
+			len += scnprintf(buff + len, sizeof(buff) - len, "]]%s",
+					 !rht_is_a_nulls(pos) ? " -> " : "");
 		}
 	}
 	printk(KERN_ERR "\n---- ht: ----%s\n-------------\n", buff);
-- 
2.9.0

Re: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

[Martin Sebor]

On 03/13/2018 07:21 AM, Arnd Bergmann wrote:
> gcc-8 warns about a code pattern that is used in the newly added
> test_rhashtable code:
>
> lib/test_rhashtable.c: In function 'print_ht':
> lib/test_rhashtable.c:511:21: error: '
> bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>                      ^~~~~~~~~
> lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
>     sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The problem here is using the same fixed-length buffer as input and output
> of snprintf(), which for an unbounded loop has an actual potential to
> overflow the buffer. The '512' byte length was apparently chosen to
> be "long enough" to prevent that in practice, but without any specific
> guarantees of being the smallest safe size.
>
> I can see three possible ways to avoid this warning:
>
> - rewrite the code to use pointer arithmetic to forward the buffer,
>   rather than copying the buffer itself. This is a more conventional
>   use of sprintf(), and it avoids the warning, but is not any more
>   safe than the original code.
> - Rewrite the function in a safe way that avoids both the potential
>   overflow and the warning.
> - Ask the gcc developers to not warn for this pattern if we consider
>   the warning to be inappropriate.

A couple of other approaches that may be worth considering are
to either a) use a precision in the %s directive to constrain
the number of characters to copy (e.g., %.490s") or b) call
snprintf() instead and use the returned value to handle
the possible truncation.

For GCC 9 we'd like to integrate the warning with the strlen pass
that tracks string lengths.  That should add another mechanism for
suppressing the warning: add an assertion before the sprintf call
bounding the string length.

Martin

> This patch implements the first of the above, as an illustration of
> the problem, and the simplest workaround.
>
> Fixes: 499ac3b60f65 ("test_rhashtable: add test case for rhltable with duplicate objects")
> Cc: Martin Sebor <msebor@gcc.gnu.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> My patch is untested, please try it out before applying.
> ---
>  lib/test_rhashtable.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> index f4000c137dbe..a0f4fb03d2de 100644
> --- a/lib/test_rhashtable.c
> +++ b/lib/test_rhashtable.c
> @@ -496,6 +496,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  	struct rhashtable *ht;
>  	const struct bucket_table *tbl;
>  	char buff[512] = "";
> +	char *buffp = buff;
>  	unsigned int i, cnt = 0;
>
>  	ht = &rhlt->ht;
> @@ -508,18 +509,18 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  		next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) : NULL;
>
>  		if (!rht_is_a_nulls(pos)) {
> -			sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
> +			buffp += sprintf(buffp, "\nbucket[%d] -> ", i);
>  		}
>
>  		while (!rht_is_a_nulls(pos)) {
>  			struct rhlist_head *list = container_of(pos, struct rhlist_head, rhead);
> -			sprintf(buff, "%s[[", buff);
> +			buffp += sprintf(buffp, "[[");
>  			do {
>  				pos = &list->rhead;
>  				list = rht_dereference(list->next, ht);
>  				p = rht_obj(ht, pos);
>
> -				sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
> +				buffp += sprintf(buffp, "val %d (tid=%d)%s", p->value.id, p->value.tid,
>  					list? ", " : " ");
>  				cnt++;
>  			} while (list);
> @@ -528,7 +529,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
>  			next = !rht_is_a_nulls(pos) ?
>  				rht_dereference(pos->next, ht) : NULL;
>
> -			sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? " -> " : "");
> +			buffp += sprintf(buffp, "]]%s", !rht_is_a_nulls(pos) ? " -> " : "");
>  		}
>  	}
>  	printk(KERN_ERR "\n---- ht: ----%s\n-------------\n", buff);
>