Ensure --addr-pool mask accepts numbers only #4619
Conversation
linear0211
requested review from
loganek,
lum1n0us,
no1wudi,
TianlongLiang,
wenyongh,
xujuntwt95329 and
yamt
as code owners
| @@ -3833,7 +3834,15 @@ wasm_runtime_init_wasi(WASMModuleInstanceCommon *module_inst, | |||
| goto fail; | |||
| } | |||
|
|
|||
| ret = addr_pool_insert(apool, address, (uint8)atoi(mask)); | |||
| mask_val = strtol(mask, &endptr, 10); | |||
There was a problem hiding this comment.
IMU, strtol() can handle "a100" and "0x100", but how does it deal with "+100" and "-100"?
There was a problem hiding this comment.
- a stricter way to check strtoxxx result is something like https://github.com/yamt/toywasm/blob/36e1f4aa9b390474da09f0e1b831eed702f67fd0/cli/str_to_uint.c#L11-L21
- i guess you can make addr_pool_insert reject masks inappropriate for the address family.
i feel addr_pool_insert is a bit awkward api as it takes addr as a string but mask as an integer. but it's another story.
There was a problem hiding this comment.
According to the Linux man page:
The string may begin with an arbitrary amount of white space (as determined by isspace(3)) followed by a single optional '+' or '-' sign.
So, +100 and -100 are parsed as positive 100 and negative 100.
I’ll also update the code to add stricter strtol checks and enforce mask range validation in addr_pool_insert.
lum1n0us
added
the
bug-fix
| goto fail; | ||
| } | ||
|
|
||
| ret = addr_pool_insert(apool, address, (uint8)mask_val); | ||
| wasm_runtime_free(cp); |
There was a problem hiding this comment.
It seems that cp is not freed when the execution jumps to the goto fail above.
The
--addr-pooloption previously usedatoito parse the mask value.If non-numeric characters were included,
atoireturned0, causing the pool to unintentionally accept all IP addresses.This change ensures the mask accepts only numeric values, preventing unexpected behavior.