Skip to content

Conversation

xujuntwt95329
Copy link
Collaborator

No description provided.

Comment on lines 2392 to 2394
if (dst_offset + len >= wasm_array_obj_length(dst_obj)
|| src_offset + len
>= wasm_array_obj_length(src_obj)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer overflow may happen in dst_offset + len and src_offset + len. And when len is 0, it should be allowed? Had better be like below?

if (len > 0) {
    if (dst_offset + len > dst_offset
        || dst_offset + len >= wasm_array_obj_length(dst_obj)
        || src_offset + len > src_offset
        || src_offset + len >= wasm_array_obj_length(src_obj)) {
        wasm_set_exception(module, "array index out of bounds");
        goto got_exception;
    }
    wasm_array_obj_copy(dst_obj, dst_offset, src_obj,
                        src_offset, len);
}
HANDLE_OP_END();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks
Using dst_offset + len < dst_offset to check overflow actually triggers an overflow, which is not guaranteed by C standard, so I changed the operand to uint64 and check if the result exceeds UINT32_MAX

wasm_set_exception(module, "null array object");
goto got_exception;
}
if (dst_offset + len >= wasm_array_obj_length(dst_obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

#if WASM_ENABLE_FAST_INTERP != 0
emit_uint32(loader_ctx, type_idx);
#endif
/* typeidx1 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeidx2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done

}

if (len > 0) {
if (((uint64)dst_offset + (uint64)len >= UINT32_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= should be allowed. And how about checking it according to SDL rule:
len > UINT32_MAX - dst_offset
We used it in:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/common/wasm_memory.c#L277-L278

if (len > 0) {
if (((uint64)dst_offset + (uint64)len >= UINT32_MAX)
|| (dst_offset + len
>= wasm_array_obj_length(dst_obj))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>= to >? = is allowed

Comment on lines 2227 to 2233
if (((uint64)dst_offset + (uint64)len >= UINT32_MAX)
|| (dst_offset + len
>= wasm_array_obj_length(dst_obj))
|| ((uint64)src_offset + (uint64)len
>= UINT32_MAX)
|| (src_offset + len
>= wasm_array_obj_length(src_obj))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wenyongh wenyongh merged commit 786cf6a into bytecodealliance:dev/gc_refactor Apr 10, 2023
wenyongh added a commit to wenyongh/wasm-micro-runtime that referenced this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants