| Message ID | 20101109130530.GD22705@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/09/10 14:05, Michael S. Tsirkin wrote: > On Tue, Nov 09, 2010 at 12:50:11PM +0100, Gerd Hoffmann wrote: >> Hi, >> >>>>>> +static int get_bool(QEMUFile *f, void *pv, size_t size) >>>>>> +{ >>>>>> + bool *v = pv; >>>>>> + *v = qemu_get_byte(f); >>>>>> + return 0; >> >>> I think we should verify that value is 0 or 1 and fail >>> migration otherwise, to make it more robust. >> >> I still think such a check doesn't belong into the migration code as >> such a bug would exist without migration too. And if anything we >> should check on save not on load, otherwise qemu can write out >> savevm images which it will refuse to load. I wouldn't call this >> "robust". >> >> cheers, >> Gerd > > I think we should verify on load: e.g. the image could have > got corrupted. For catching corruption checksums work much better. > What, exactly, do you want to check on save? I don't want to check anything. I'm just saying that *if* we are sanity-checking bool to catch bugs it is much more useful to do that when saving. > savevm: validate bool values on load > > We always save 0 or 1 values for booleans. Validate on input to > increase the chance of detecting input corruption. NACK. cheers, Gerd
On Tue, Nov 09, 2010 at 02:28:37PM +0100, Gerd Hoffmann wrote: > On 11/09/10 14:05, Michael S. Tsirkin wrote: > >On Tue, Nov 09, 2010 at 12:50:11PM +0100, Gerd Hoffmann wrote: > >> Hi, > >> > >>>>>>+static int get_bool(QEMUFile *f, void *pv, size_t size) > >>>>>>+{ > >>>>>>+ bool *v = pv; > >>>>>>+ *v = qemu_get_byte(f); > >>>>>>+ return 0; > >> > >>>I think we should verify that value is 0 or 1 and fail > >>>migration otherwise, to make it more robust. > >> > >>I still think such a check doesn't belong into the migration code as > >>such a bug would exist without migration too. And if anything we > >>should check on save not on load, otherwise qemu can write out > >>savevm images which it will refuse to load. I wouldn't call this > >>"robust". > >> > >>cheers, > >> Gerd > > > >I think we should verify on load: e.g. the image could have > >got corrupted. > > For catching corruption checksums work much better. Unless there's a bug in software that writes the file, then checksum will match. > >What, exactly, do you want to check on save? > > I don't want to check anything. Why did you suggest it above then? > I'm just saying that *if* we are sanity-checking bool My patch doesn't check bool. Look at it. I am sanity checking a byte read from file. File can have any values, there is no guarantee that it has the same value that the same version of qemu wrote out. > to catch bugs > it is much more useful to do that when saving. There's nothing we *can* check. if (v == true || v == false) is always true according to the language standard. How is it useful to stick always true conditions that compiler will likely remove in code? > >savevm: validate bool values on load > > > >We always save 0 or 1 values for booleans. Validate on input to > >increase the chance of detecting input corruption. > > NACK. > > cheers, > Gerd
diff --git a/savevm.c b/savevm.c index 4e49765..da2fdfa 100644 --- a/savevm.c +++ b/savevm.c @@ -680,7 +680,12 @@ uint64_t qemu_get_be64(QEMUFile *f) static int get_bool(QEMUFile *f, void *pv, size_t size) { bool *v = pv; - *v = qemu_get_byte(f); + uint8_t b; + b = qemu_get_byte(f); + if (b != (uint8_t)true && b != (uint8_t)false) { + return -EINVAL; + } + *v = b; return 0; }