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

[Frediano Ziglio]

> 
> 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.
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).

> 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