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@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
Frediano++
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel