Re: [Qemu-devel] [PATCH v2] vmdk: Fix "%x" to PRIx32 in format strings for cid

[Fam Zheng]

On Thu, 04/17 06:00, Eric Blake wrote:
> On 04/17/2014 04:43 AM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <address@hidden>
> > 
> > ---
> > v2: PRIx32 -> SCNx32. (Kevin)
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> 
> > +++ b/block/vmdk.c
> > @@ -262,7 +262,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int 
> > parent)
> >      p_name = strstr(desc, cid_str);
> >      if (p_name != NULL) {
> >          p_name += cid_str_size;
> > -        sscanf(p_name, "%x", &cid);
> > +        sscanf(p_name, "%" SCNx32, &cid);
> 
> sscanf() has undefined behavior on integer overflow.  This is not the
> only vulnerable site in the code base, but if you are ever reading from
> external input, and the ascii string being parsed does not fit in the
> variable requested by SCNx32, you risk silently parsing the wrong
> number.  It is always safer to use the strtol family (or a sane wrapper
> thereof that gets errno handling correct) for parsing strings into
> integers.  That said, I'm not going to reject this patch for using
> sscanf, so much as suggest that you look into a followup patch to avoid it.
> 

Good point, thanks for the explanation. The particular case of sscanf doesn't
matter too much because it's only a time stamp, and the only possible impact of
overflow is denial of using the image, where it is coincidentally appropriate.

I'm putting sscanf replacing on my list and leaving it for future.

Thanks,
Fam