Fix some issues reported by CodeQL #3064
Conversation
| @@ -3134,8 +3134,7 @@ resolve_execute_mode(const uint8 *buf, uint32 size, bool *p_mode, | |||
| p += 8; | |||
| while (p < p_end) { | |||
| read_uint32(p, p_end, section_type); | |||
| if (section_type <= AOT_SECTION_TYPE_SIGANATURE | |||
| || section_type == AOT_SECTION_TYPE_TARGET_INFO) { | |||
There was a problem hiding this comment.
AOT_SECTION_TYPE_TARGET_INFO == 2, also <= AOT_SECTION_TYPE_SIGANATURE
| @@ -2294,7 +2294,7 @@ wasm_module_new(wasm_store_t *store, const wasm_byte_vec_t *binary) | |||
| (uint8 *)module_ex->binary->data, (uint32)module_ex->binary->size, | |||
| error_buf, (uint32)sizeof(error_buf)); | |||
| if (!(module_ex->module_comm_rt)) { | |||
| LOG_ERROR(error_buf); | |||
| LOG_ERROR("%s", error_buf); | |||
There was a problem hiding this comment.
Had better use constant fmt.
| @@ -884,7 +884,7 @@ os_socket_set_ip_ttl(bh_socket_t socket, uint8_t ttl_s) | |||
| int | |||
| os_socket_get_ip_ttl(bh_socket_t socket, uint8_t *ttl_s) | |||
| { | |||
| socklen_t opt_len = sizeof(ttl_s); | |||
| socklen_t opt_len = sizeof(*ttl_s); | |||
There was a problem hiding this comment.
According to os_socket_set_ip_ttl above, L877, I think it should be sizeof(uint8_t), but I am not very sure.
@loganek could you help check whether the size here is sizeof(uint8_t)? The issue is reported by CodeQL introduced by PR #2812:
https://github.com/bytecodealliance/wasm-micro-runtime/security/code-scanning/70
https://github.com/bytecodealliance/wasm-micro-runtime/security/code-scanning/69
There was a problem hiding this comment.
sockopt is an ancient api with a lot of confusions and compatibility hacks.
man 4 ip on macOS has an example like:
int ttl = 60; /* max = 255 */
setsockopt(s, IPPROTO_IP, IP_TTL, &ttl, sizeof(ttl));
u_char ttl; /* range: 0 to 255, default = 1 */
setsockopt(s, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl));
otoh,
https://www.man7.org/linux/man-pages/man7/ip.7.html says:
(see the last sentence)
IP_MULTICAST_TTL (since Linux 1.2)
Set or read the time-to-live value of outgoing multicast
packets for this socket. It is very important for
multicast packets to set the smallest TTL possible. The
default is 1 which means that multicast packets don't
leave the local network unless the user program explicitly
requests it. Argument is an integer.
https://www.ibm.com/docs/en/i/7.1?topic=ssw_ibm_i_71/apis/ssocko.htm even says:
IP_TTL
....
Note:Only the rightmost octet (least significant octet) of the integer value is used.
There was a problem hiding this comment.
I think we should be passing sizeof(*ttl_s) (regardless of what type is that. I'm a bit worried that under the hood, some of the implementations will try to read as much as we pass as a size - if we pass sizeof(int), three bytes will contain a garbage. If we really want this to be four bytes long, we should rather change the type of the parameter to be uint32_t*, but looks like most of the implementations expect the value to be within 0-255 range anyway.
There was a problem hiding this comment.
Yes, it's a little confusing, what the size should be may depend on which option to set/get and what the platform is. But IIUC, for the same option and same platform, the size to set/get should be the same, if we change the size in os_socket_get_ip_ttl to sizeof(int), then we should also change the size in os_socket_set_ip_ttl to sizeof(int) accordingly.
There was a problem hiding this comment.
I think we should be passing
sizeof(*ttl_s)(regardless of what type is that. I'm a bit worried that under the hood, some of the implementations will try to read as much as we pass as a size - if we passsizeof(int), three bytes will contain a garbage. If we really want this to be four bytes long, we should rather change the type of the parameter to beuint32_t*, but looks like most of the implementations expect the value to be within 0-255 range anyway.
Yes, agree.
| @@ -906,7 +906,7 @@ os_socket_set_ip_multicast_ttl(bh_socket_t socket, uint8_t ttl_s) | |||
| int | |||
| os_socket_get_ip_multicast_ttl(bh_socket_t socket, uint8_t *ttl_s) | |||
| { | |||
| socklen_t opt_len = sizeof(ttl_s); | |||
| socklen_t opt_len = sizeof(*ttl_s); | |||
There was a problem hiding this comment.
Same as above, sizeof(ttl_s) is used in L898.
Refer to #2812
and https://github.com/bytecodealliance/wasm-micro-runtime/security/code-scanning?query=pr%3A2812+is%3Aopen
Though most of them are minor issues (like some unnecessary checks),
we had better fix them.