>
> 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@xxxxxxxxxx>
>
> > + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel