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 <mse...@gcc.gnu.org> > Signed-off-by: Arnd Bergmann <a...@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); >
