| Message ID | 20241014151023.85698-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ui/console-vc: Silence warning about sprintf() on OpenBSD | expand |
On Mon, Oct 14, 2024 at 05:10:23PM +0200, Thomas Huth wrote: > The linker on OpenBSD complains: > > ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]): > warning: sprintf() is often misused, please use snprintf() > > Using snprintf() is certainly better here, so let's switch to that > function instead. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > ui/console-vc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/ui/console-vc.c b/ui/console-vc.c > index 8393d532e7..336a1520eb 100644 > --- a/ui/console-vc.c > +++ b/ui/console-vc.c > @@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch) > break; > case 6: > /* report cursor position */ > - sprintf(response, "\033[%d;%dR", > - (s->y_base + s->y) % s->total_height + 1, > - s->x + 1); > + snprintf(response, sizeof(response), "\033[%d;%dR", > + (s->y_base + s->y) % s->total_height + 1, > + s->x + 1); > vc_respond_str(vc, response); These two lines are the only place in the code that uses the char response[40]; so even better than switching to snprintf, how about just taking buffer size out of the picture: g_autofree *response = g_strdup_printf("\033[%d;%dR", (s->y_base + s->y) % s->total_height + 1, s->x + 1); vc_respond_str(vc, response); With regards, Daniel
On 14.10.2024 18:15, Daniel P. Berrangé wrote: > These two lines are the only place in the code that uses the > > char response[40]; > > so even better than switching to snprintf, how about just taking > buffer size out of the picture: > > g_autofree *response = > g_strdup_printf("\033[%d;%dR", > (s->y_base + s->y) % s->total_height + 1, > s->x + 1); > vc_respond_str(vc, response); What's the reason to perform memory allocation in trivial places like this? If we're worrying about possible buffer size issue, maybe asprintf() is a better alternative for such small things? Fragmenting heap memory for no reason seems too much overkill. But I'm old-scool, so.. :) /mjt
On Mon, Oct 14, 2024 at 7:10 PM Thomas Huth <thuth@redhat.com> wrote: > > The linker on OpenBSD complains: > > ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]): > warning: sprintf() is often misused, please use snprintf() > > Using snprintf() is certainly better here, so let's switch to that > function instead. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > ui/console-vc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/ui/console-vc.c b/ui/console-vc.c > index 8393d532e7..336a1520eb 100644 > --- a/ui/console-vc.c > +++ b/ui/console-vc.c > @@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch) > break; > case 6: > /* report cursor position */ > - sprintf(response, "\033[%d;%dR", > - (s->y_base + s->y) % s->total_height + 1, > - s->x + 1); > + snprintf(response, sizeof(response), "\033[%d;%dR", > + (s->y_base + s->y) % s->total_height + 1, > + s->x + 1); > vc_respond_str(vc, response); > break; > } > -- > 2.46.1 >
Michael Tokarev <mjt@tls.msk.ru> writes: > On 14.10.2024 18:15, Daniel P. Berrangé wrote: > >> These two lines are the only place in the code that uses the >> char response[40]; >> so even better than switching to snprintf, how about just taking >> buffer size out of the picture: >> g_autofree *response = >> g_strdup_printf("\033[%d;%dR", >> (s->y_base + s->y) % s->total_height + 1, >> s->x + 1); >> vc_respond_str(vc, response); > > What's the reason to perform memory allocation in trivial places > like this? If we're worrying about possible buffer size issue, > maybe asprintf() is a better alternative for such small things? > Fragmenting heap memory for no reason seems too much overkill. > But I'm old-scool, so.. :) I doubt the allocate/free pair will cause much fragmentation but it doesn't look like we are in any hot path here. Anyway snprintf is certainly better than sprintf so: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > /mjt
On Mon, Oct 14, 2024 at 10:50:44PM +0300, Michael Tokarev wrote: > On 14.10.2024 18:15, Daniel P. Berrangé wrote: > > > These two lines are the only place in the code that uses the > > > > char response[40]; > > > > so even better than switching to snprintf, how about just taking > > buffer size out of the picture: > > > > g_autofree *response = > > g_strdup_printf("\033[%d;%dR", > > (s->y_base + s->y) % s->total_height + 1, > > s->x + 1); > > vc_respond_str(vc, response); > > What's the reason to perform memory allocation in trivial places > like this? If we're worrying about possible buffer size issue, > maybe asprintf() is a better alternative for such small things? > Fragmenting heap memory for no reason seems too much overkill. > But I'm old-scool, so.. :) This is not a performance sensitive path, and using g_strdup_printf makes it robust against any futher changes in the future. In the context of all the memory allocation QEMU does, I can't see this making any difference to heap fragmentation whatsoever. snprintf with fixed buffers should only be used where there's a demonstratable performance win, and the return value actually checked with an assert() to prove we're not overflowing. With regards, Daniel
On 15/10/2024 10.14, Daniel P. Berrangé wrote: > On Mon, Oct 14, 2024 at 10:50:44PM +0300, Michael Tokarev wrote: >> On 14.10.2024 18:15, Daniel P. Berrangé wrote: >> >>> These two lines are the only place in the code that uses the >>> >>> char response[40]; >>> >>> so even better than switching to snprintf, how about just taking >>> buffer size out of the picture: >>> >>> g_autofree *response = >>> g_strdup_printf("\033[%d;%dR", >>> (s->y_base + s->y) % s->total_height + 1, >>> s->x + 1); >>> vc_respond_str(vc, response); >> >> What's the reason to perform memory allocation in trivial places >> like this? If we're worrying about possible buffer size issue, >> maybe asprintf() is a better alternative for such small things? >> Fragmenting heap memory for no reason seems too much overkill. >> But I'm old-scool, so.. :) > > This is not a performance sensitive path, and using g_strdup_printf > makes it robust against any futher changes in the future. In the > context of all the memory allocation QEMU does, I can't see this > making any difference to heap fragmentation whatsoever. > > snprintf with fixed buffers should only be used where there's a > demonstratable performance win, and the return value actually > checked with an assert() to prove we're not overflowing. While I'm obviously old-schooled, too (since I used snprintf here in the first place), I agree with Daniel that the few cycles of improved performance likely aren't justified in this case here, so g_strdup_printf() is a better choice here indeed. I just sent a v2 with that change. Thomas
diff --git a/ui/console-vc.c b/ui/console-vc.c index 8393d532e7..336a1520eb 100644 --- a/ui/console-vc.c +++ b/ui/console-vc.c @@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch) break; case 6: /* report cursor position */ - sprintf(response, "\033[%d;%dR", - (s->y_base + s->y) % s->total_height + 1, - s->x + 1); + snprintf(response, sizeof(response), "\033[%d;%dR", + (s->y_base + s->y) % s->total_height + 1, + s->x + 1); vc_respond_str(vc, response); break; }
The linker on OpenBSD complains: ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]): warning: sprintf() is often misused, please use snprintf() Using snprintf() is certainly better here, so let's switch to that function instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- ui/console-vc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)