On 30/03/14 13:12, Stas Sergeev wrote:
> 30.03.2014 14:03, Andrew Bird пишет:
>> Hi Stas, Bart,
>> Firstly, sorry if you already saw this on linux-msdos list, I didn't
>> realise this list existed but I'll only post here from now on.
>> Here are some patches I made against today's devel branch. I had no
>> specific bug to fix, but just wanted to fix some minor bounds checks
>> and initialisation issues. If you appreciate what I am trying to do
>> I'll spend some more time looking for others.
>>
>> All compile successfully, but I have very few DOS programs to exercise
>> Dosemu properly. Any standalone memory exerciser programs for XMS, EMS
>> or DPMI would be helpful?
> Thanks for the patches!
> I applied the ones that look obvious.
> I would prefer the patches in bts, one per ticket.
> There is a concern that in an ML the patches are
> better reviewed. This is true but bts also sends the
> notification e-mails, so whoever is interested in
> doing a review, can as well subscribe there I think.
Hi Stas,
Thanks for reviewing. Yes, I'll send the next set of patches via the
issue tracker.
>
>> - if (handle_info[handle].active != 1)
>> + if (!emm_valid_handle(handle))
>> return (FALSE);
> Please double check that this is valid.
> For instance, .active can be set to 0xff, which means
> non-freeable handle.
An oversight, I hadn't realised .active could be anything else but 0 or
1! I'm not sure of the proper semantics here, so it may be better not to
use the 'emm_valid_handle()' function here. If you agree I'll regenerate
the patches without?
>
>> - if ((handle < 0) || (handle >= MAX_HANDLES) || \
>> - (handle_info[handle].active == 0)) { \
>> + if ((handle < 0) || (handle >= MAX_HANDLES)) { \
>> + E_printf("Invalid Handle handle=%x\n", handle); \
>> + SETHIGH(&(state->eax), EMM_INV_HAN); \
>> + return; \
>> + } \
>> + if (handle_info[handle].active == 0) { \
> I don't think this fixes something.
> The .active check will trigger only if both first
> conditions evaluate to false, unless I forgot all of the
> C programming.
Yes you would be correct if there was no E_printf that used the value
too. For example in the case that 'handle' is 255 the condition '
handle >= MAX_HANDLES' is true and as you say
'handle_info[handle].active == 0' is not evaluated, but the error is in
the is in the printf of the value of 'handle_info[handle].active' which
is off the end of the array. See the snippet below which shows the
unpatched version.
if ((handle < 0) || (handle >= MAX_HANDLES) ||
(handle_info[handle].active == 0)) {
E_printf("Invalid Handle handle=%x, active=%d\n",
handle, handle_info[handle].active);
return EMM_INV_HAN;
>
>> - memcheck_reserve('U', addr_start, size);
>>
>> umb = umb_find_unused();
>> + if (umb == UMB_NULL) {
> I haven't looked closely, but isn't this an assert condition?
Well umb_find_unused() looks perfectly capable of returning -1 without
asserting. Do you mean that the compiler should auto generate asserts
for negative indexes into an array. I've never checked that this occurs
myself, but if you are sure this happens feel free to drop the patch.
Thanks,
Andrew
------------------------------------------------------------------------------
_______________________________________________
Dosemu-devel mailing list
Dosemu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dosemu-devel
