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 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);
>

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