Conversation
peter-jerry-ye
force-pushed
the
zihang/add-clap
branch
from
9756a9c to
fd78428
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| while (*env != NULL) { | ||
| // Find the '=' character in the environment variable string | ||
| char *equals = strchr(*env, '='); | ||
| if (equals != NULL) { | ||
| // Calculate lengths of key and value | ||
| size_t key_len = equals - *env; | ||
| size_t val_len = strlen(equals + 1); | ||
|
|
||
| // Create bytes object for key and copy data | ||
| moonbit_bytes_t key = moonbit_make_bytes(key_len, 0); | ||
| memcpy(key, *env, key_len); | ||
|
|
||
| // Create bytes object for value and copy data | ||
| moonbit_bytes_t value = moonbit_make_bytes(val_len, 0); | ||
| memcpy(value, equals + 1, val_len); | ||
|
|
||
| // Store key and value in result array | ||
| // Even indices store keys, odd indices store values | ||
| result[i * 2] = key; | ||
| result[i * 2 + 1] = value; | ||
| } | ||
| env++; | ||
| i++; | ||
| } |
There was a problem hiding this comment.
🔴 Uninitialized array elements in get_env_vars() when environment variable lacks '=' separator
In the native C stub for get_env_vars(), the code allocates an array of size count * 2 based on the total count of environment variables, then iterates through and only populates array elements when an environment variable contains an = character. If an environment variable lacks this separator (edge case), the corresponding array slots remain uninitialized.
Root Cause
In native_stub.c:104-127 (Unix) and lines 67-84 (Windows), the loop increments i regardless of whether the equals pointer is NULL:
while (*env != NULL) {
char *equals = strchr(*env, '=');
if (equals != NULL) {
// Only populate result[i * 2] and result[i * 2 + 1] here
}
env++;
i++; // Always incremented, even if equals == NULL
}The MoonBit code in env_native.mbt:50-54 then iterates through the entire array length assuming all elements are valid:
for i = 0; i < tmp.length(); i = i + 2 {
let key = utf8_bytes_to_mbt_string(tmp[i]) // May access uninitialized memory
let value = utf8_bytes_to_mbt_string(tmp[i + 1])
res[key] = value
}Impact: Could cause undefined behavior or crashes when accessing uninitialized array elements, though this edge case is rare in practice since environment variables normally always contain =.
Prompt for agents
Fix the get_env_vars() function in env/native_stub.c to handle environment variables that lack an '=' separator. Two approaches: (1) Only count variables that have '=' in the first pass, so the array size matches actual populated entries, or (2) Skip incrementing 'i' when equals is NULL. The same fix needs to be applied to both the Unix code (lines 92-128) and Windows code (lines 46-87). For example, in the Unix section, change the loop to only increment 'i' inside the 'if (equals != NULL)' block, not after it.
Was this helpful? React with 👍 or 👎 to provide feedback.
Pull Request Test Coverage Report for Build 2565Details
💛 - Coveralls |
| let with_index = [] | ||
| let without_index = [] | ||
| for arg in args { | ||
| if arg.long is None && arg.short is None { |
There was a problem hiding this comment.
🟡 Inconsistent positional argument detection in positional_args vs is_positional_arg
The positional_args function only checks arg.long is None && arg.short is None to identify positional arguments, but is_positional_arg also checks that all alias arrays are empty. This inconsistency means an argument with no short/long but with aliases would be incorrectly treated as a positional argument by positional_args, causing it to be assigned positional values when it shouldn't be.
Root Cause
At arg_parser/clap_command.mbt:1235, the check is:
if arg.long is None && arg.short is None {
But at arg_parser/clap_command.mbt:585-591, is_positional_arg checks:
arg.short is None &&
arg.long is None &&
arg.aliases.length() == 0 &&
arg.short_aliases.length() == 0 &&
arg.visible_aliases.length() == 0 &&
arg.visible_short_aliases.length() == 0
Impact: If a user creates an Arg with only aliases (no short/long) but intends it to be a named option via those aliases, positional_args would incorrectly include it in the positional arguments list.
| if arg.long is None && arg.short is None { | |
| if is_positional_arg(arg) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
| .about("demo") | ||
| .arg(@arg_parser.Arg::new("count").long("count").option()) | ||
| .arg(@arg_parser.Arg::new("name").index(0)) | ||
| let mut text = "" |
There was a problem hiding this comment.
Command(name="demo",about="demo", args = [
OptionalArg("count", long = "count", optional = true),
PositionalArg("name", index = 0)
])
|
Continue in #3213. The |
3 participants
Uh oh!
There was an error while loading. Please reload this page.