lkml.org 
[lkml]   [2018]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning
From
Date
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);
>

\
 
 \ /
  Last update: 2018-03-13 15:42    [W:0.087 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site