Messages in this thread | | | Subject | Re: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning | From | Martin Sebor <> | Date | Tue, 13 Mar 2018 10:04:29 -0600 |
| |
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); >
| |