30.03.2014 22:09, Andrew Bird пишет:
>>
>>> - 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?
Will likely be ok.
>>
>>> - 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.
Ah, that's right!
Applied.
> 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.
I meant, in this particular place (init code) - it really
shouldn't. Since I think there is no valid scenario
in which it can (in that particular place), I'd rather
put some kind of assert there. Simply checking and
ignoring the initialization error looks unreasonable.
------------------------------------------------------------------------------
_______________________________________________
Dosemu-devel mailing list
Dosemu-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dosemu-devel
