[Spice-devel] [PATCH spice-server 1/8] test-stream-device: Factor out VMC emulation

[Victor Toso]

Hi,

On Tue, Oct 08, 2019 at 10:04:25AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Oct 07, 2019 at 11:38:59AM +0100, Frediano Ziglio wrote:
> > > Allows to reuse code for emulating a character device.
> > > It will be used for Smardcard test.
> > > 
> 
> ...
> 
> > > +
> > > +void vmc_emu_reset(VmcEmu *vmc)
> > > +{
> > > +    vmc->pos = 0;
> > > +    vmc->write_pos = 0;
> > > +    vmc->message_sizes_curr = vmc->message_sizes;
> > > +    vmc->message_sizes_end = vmc->message_sizes;
> > > +}
> > > +
> > > +void vmc_emu_add_read_till(VmcEmu *vmc, uint8_t *end)
> > > +{
> > > +    g_assert(vmc->message_sizes_end - vmc->message_sizes <
> > > G_N_ELEMENTS(vmc->message_sizes));
> > 
> > I'd move the unsigned size here and change both asserts to use
> > it, that is:
> > 
> >     unsigned size = end - vmc->message;
> >     g_assert(size >= 0);
> >     g_assert(size <= G_N_ELEMENTS(vmc->message));
> > 
> 
> They are not exactly the same.
> size >= 0 will be always true, it's unsigned while the initial check
> could be false.

Ouch! True :)

> Also checking end - vmc->message <= G_N_ELEMENTS(vmc->message) and
> using size instead could have different results in case the difference
> is truncated to fit in an unsigned (for instance if unsigned is 32 bit
> and end - vmc->message is more than 2**32).

That's interesting, I'd guess that end - vmc->message would also
truncate before comporting with G_N_ELEMENTS(vmc->message). I
should start testing this kind of stuff.

> > This is my only nitpick for this patch, feel free to ignore if
> > you want
> > 
> >     Acked-by: Victor Toso <victortoso at redhat.com>
> > 
> > > +    g_assert(end >= vmc->message);
> > > +    g_assert(end - vmc->message <= G_N_ELEMENTS(vmc->message));
> > > +    unsigned prev_size =
> > > +        vmc->message_sizes_end > vmc->message_sizes ?
> > > vmc->message_sizes_end[-1] : 0;
> > 
> > Forgot how long since I saw a negative index in C!
> > 
> > > +    unsigned size = end - vmc->message;
> > > +    g_assert(size >= prev_size);
> > > +    *vmc->message_sizes_end = size;
> > > +    ++vmc->message_sizes_end;
> > > +}
> 
> Mostly OT:
> 
> This is just code for test so it's not meant to be "safe" but it seems
> that a good rule for security checks is "let's the unsafe alone".
> Just an example, having to check if we have a full message sometimes
> we do
> 
> if (sizeof(header) + header.size >= buffer_size) ...
> 
> The "header.size" usually came from network so should be considered
> unsafe but it's not alone, better to change the check to
> 
> if (header.size >= buffer_size - sizeof(header)) ...
> 
> This to avoid the possible overflow with the addition (and I would
> remember that memory is not infinite but this is another story).
> 
> Frediano

Frediano++
-------------- 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/20191008/2d5ed902/attachment.sig>