Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly

[Igor Mammedov]

On Thu, 27 Sep 2018 10:58:47 +0200
David Hildenbrand <address@hidden> wrote:

> On 27/09/2018 10:47, Igor Mammedov wrote:
> > On Thu, 27 Sep 2018 10:13:07 +0200
> > David Hildenbrand <address@hidden> wrote:
> >   
> >> On 27/09/2018 10:02, Igor Mammedov wrote:  
> >>> On Wed, 26 Sep 2018 11:41:59 +0200
> >>> David Hildenbrand <address@hidden> wrote:
> >>>     
> >>>> Make address_space_end point at the real end, instead of end + 1, so we 
> >>>> don't
> >>>> have to handle special cases like it being 0. This will allow us to
> >>>> place a memory device at the very end of the guest physical 64bit address
> >>>> space (if ever possible).    
> >>>
> >>> [...]
> >>>     
> >>>> @@ -115,7 +116,7 @@ uint64_t memory_device_get_free_addr(MachineState 
> >>>> *ms, const uint64_t *hint,
> >>>>      }
> >>>>      address_space_start = ms->device_memory->base;
> >>>>      address_space_end = address_space_start +
> >>>> -                        memory_region_size(&ms->device_memory->mr);
> >>>> +                        memory_region_size(&ms->device_memory->mr) - 1; 
> >>>>    
> >>> I'm terrible at math, lets assume size = 1 so after this
> >>> 'real end' would end up at 'address_space_start', which makes
> >>> it rather confusing beside not matching variable name anymore.    
> >>
> >> (very simply and unrealistic) counter example as given in the
> >> description. You should get the idea.
> >>
> >> address_space_start = 0xffffffffffffffffull;  
> > that should never happen, nor valid in any conditions
> > just add assert so we would catch error if some patch would introduce it
> >   
> >> size = 1;
> >>  
> >> -> address_space_end = 0;    
> >>
> >> While this would be perfectly valid, we would have to handle
> >> address_space_end potentially being 0 in the code below, because this
> >> would be a valid overflow. This, I avoid.  
> > assert(address_space_end > address_space_start)
> > would be much better for unrealistic values without messing up with
> > meaning of variables.
> >    
> >> And form my POV, the variable name here matches perfectly. It points at
> >> the last address of the address space. (yes people have different
> >> opinions on this)  
> > start,end pair is a range, there shouldn't be any other interpretations to
> > this variables.
> >   
> 
> Please see include/qemu/range.h:struct Range;
> 
> So I am using _the definition_.
> >>>
> >>> I'd drop it and maybe extend the following assert to abort
> >>> on overflow condition.    
> >>
> >> I'll leave it like this, handling address_space_end = 0 is ugly.  
> > Looks like I have to insist on dropping this hunk.  
> 
> You're going from "I'd" to "I have to insist". Please make up your mind.
my suggestions heavily influenced by the fact that the code should
be easy (for me and hopefully for others) to maintain in future,
when I have to review related code later down the road when original
author is not reachable/care anymore. (i.e. by pure selfishness)

So far I'm not convinced that this change is for a better hence we stuck
without consensus.

> I have a R-b from David. But I am willing to change it if you can give
> me a good reason why we should add asserts instead of fixing the code
> (== making it work in all possibly valid scenarios, especially end of
> address space).
if it's about valid cases then it's worth fixing,
but how many of the cases targeted here are in need of fixing in practice?

I'd keep it simple so it would be readable and do the job
but not clutter it with not necessary logic.

If we =actually= need to handle every possible permutation, it might
be better to reuse range_foo() helpers you've mentioned, instead of
open coding conditions over again. But only if we really need it or
if it makes current code simpler to read.

> With the new local variable new_end, the code looks pretty clean.
>