Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
On Wed, Mar 20, 2019 at 11:20:20AM -0400, Frediano Ziglio wrote: > > > > On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > > > Although id is not supposed to be big prevent possible > > > warning/overflow. > > > > > > Signed-off-by: Frediano Ziglio > > > --- > > > server/red-worker.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > This was signaled by Christophe Fergeau > > > > > > diff --git a/server/red-worker.c b/server/red-worker.c > > > index 8051d1e4..a25a0cd8 100644 > > > --- a/server/red-worker.c > > > +++ b/server/red-worker.c > > > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > > > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > > > worker->driver_cap_monitors_config = 0; > > > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > > > -sprintf(worker_str, "display[%d]", worker->qxl->id); > > > +snprintf(worker_str, sizeof(worker_str), "display[%d]", > > > worker->qxl->id); > > > > You pointed out that in the protocol, the id is 8 bits, so I'd change to > > worker->qxl->id & 0xff while at it. > > > > Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can > > still get snprintf to misbehave: > > "display[]" is 9 bytes > > %d may need 11 bytes to be printed (if id is less than (unsigned > > int)-40) > > so we'd be need 20 bytes in the buffer plus the trailing \0. > > > > Christophe > > > > Your patch was better and it has a better explanation. > > The number you brought as example is weird! It does not fit into > a 32 bit so cannot be worker->qxl->id. Yep, sorry, unsigned/signed mix up in my head ;) It should have been -20 > As we currently support only > architectures where int is a 32 bit the (unsigned int)whatever will > fit into 10 characters however formatting with %d will be converted > to signed (so int) which won't fit in 10 characters (with the > additional sign it requires 11 characters). Yep, this is what I meant, with a wrong example. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
> > On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > > Although id is not supposed to be big prevent possible > > warning/overflow. > > > > Signed-off-by: Frediano Ziglio > > --- > > server/red-worker.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > This was signaled by Christophe Fergeau > > > > diff --git a/server/red-worker.c b/server/red-worker.c > > index 8051d1e4..a25a0cd8 100644 > > --- a/server/red-worker.c > > +++ b/server/red-worker.c > > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > > worker->driver_cap_monitors_config = 0; > > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > > -sprintf(worker_str, "display[%d]", worker->qxl->id); > > +snprintf(worker_str, sizeof(worker_str), "display[%d]", > > worker->qxl->id); > > You pointed out that in the protocol, the id is 8 bits, so I'd change to > worker->qxl->id & 0xff while at it. > > Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can > still get snprintf to misbehave: > "display[]" is 9 bytes > %d may need 11 bytes to be printed (if id is less than (unsigned > int)-40) > so we'd be need 20 bytes in the buffer plus the trailing \0. > > Christophe > Your patch was better and it has a better explanation. The number you brought as example is weird! It does not fit into a 32 bit so cannot be worker->qxl->id. As we currently support only architectures where int is a 32 bit the (unsigned int)whatever will fit into 10 characters however formatting with %d will be converted to signed (so int) which won't fit in 10 characters (with the additional sign it requires 11 characters). Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote: > Although id is not supposed to be big prevent possible > warning/overflow. > > Signed-off-by: Frediano Ziglio > --- > server/red-worker.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > This was signaled by Christophe Fergeau > > diff --git a/server/red-worker.c b/server/red-worker.c > index 8051d1e4..a25a0cd8 100644 > --- a/server/red-worker.c > +++ b/server/red-worker.c > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, > worker->zlib_glz_state = reds_get_zlib_glz_state(reds); > worker->driver_cap_monitors_config = 0; > char worker_str[SPICE_STAT_NODE_NAME_MAX]; > -sprintf(worker_str, "display[%d]", worker->qxl->id); > +snprintf(worker_str, sizeof(worker_str), "display[%d]", worker->qxl->id); You pointed out that in the protocol, the id is 8 bits, so I'd change to worker->qxl->id & 0xff while at it. Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can still get snprintf to misbehave: "display[]" is 9 bytes %d may need 11 bytes to be printed (if id is less than (unsigned int)-40) so we'd be need 20 bytes in the buffer plus the trailing \0. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning
Although id is not supposed to be big prevent possible warning/overflow. Signed-off-by: Frediano Ziglio --- server/red-worker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) This was signaled by Christophe Fergeau diff --git a/server/red-worker.c b/server/red-worker.c index 8051d1e4..a25a0cd8 100644 --- a/server/red-worker.c +++ b/server/red-worker.c @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl, worker->zlib_glz_state = reds_get_zlib_glz_state(reds); worker->driver_cap_monitors_config = 0; char worker_str[SPICE_STAT_NODE_NAME_MAX]; -sprintf(worker_str, "display[%d]", worker->qxl->id); +snprintf(worker_str, sizeof(worker_str), "display[%d]", worker->qxl->id); stat_init_node(>stat, reds, NULL, worker_str, TRUE); stat_init_counter(>wakeup_counter, reds, >stat, "wakeups", TRUE); stat_init_counter(>command_counter, reds, >stat, "commands", TRUE); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
