[Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning

[Christophe Fergeau]

On Wed, Mar 20, 2019 at 11:20:20AM -0400, Frediano Ziglio wrote:
> > 
> > On Wed, Mar 20, 2019 at 02:51:29PM +0000, Frediano Ziglio wrote:
> > > Although id is not supposed to be big prevent possible
> > > warning/overflow.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  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)-4000000000)
> > 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
-2000000000 

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190320/563c17a4/attachment.sig>