[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflow
From: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly |
Date: |
Thu, 27 Sep 2018 10:58:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
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.
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).
With the new local variable new_end, the code looks pretty clean.
--
Thanks,
David / dhildenb
- [Qemu-devel] [PATCH v4 02/24] memory-device: fix error message when hinted address is too small, (continued)
- [Qemu-devel] [PATCH v4 02/24] memory-device: fix error message when hinted address is too small, David Hildenbrand, 2018/09/26
- [Qemu-devel] [PATCH v4 03/24] pc-dimm: pass PCDIMMDevice to pc_dimm_.*plug, David Hildenbrand, 2018/09/26
- [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, David Hildenbrand, 2018/09/26
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, Igor Mammedov, 2018/09/27
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, David Hildenbrand, 2018/09/27
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, Igor Mammedov, 2018/09/27
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly,
David Hildenbrand <=
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, Igor Mammedov, 2018/09/27
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, David Hildenbrand, 2018/09/27
- Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly, Igor Mammedov, 2018/09/27
[Qemu-devel] [PATCH v4 05/24] memory-device: use memory device terminology in error messages, David Hildenbrand, 2018/09/26
[Qemu-devel] [PATCH v4 06/24] memory-device: introduce separate config option, David Hildenbrand, 2018/09/26
[Qemu-devel] [PATCH v4 07/24] memory-device: forward errors in get_region_size()/get_plugged_size(), David Hildenbrand, 2018/09/26
[Qemu-devel] [PATCH v4 08/24] memory-device: document MemoryDeviceClass, David Hildenbrand, 2018/09/26