Repository navigation
fix(adhoc-sweep-fixes): CU-86akq5g67 24 review findings across 21 files - #173
flamingo[bot] wants to merge 21 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
|
|
||
| this.scan = function (rangestr, timeout, callback) { | ||
| var iprange = this.parseIPv4Range(rangestr); | ||
| if (iprange == null) { | ||
| if (callback) { callback([]); } | ||
| return; | ||
| } | ||
| var rmcp = this.buildRmcpPing(0); | ||
| var server = this.dgram.createSocket({ type: 'udp4' }); | ||
| server.parent = this; |
There was a problem hiding this comment.
🦩 🔴 scan() dereferences iprange.min/max without checking for null returned by parseIPv4Range
In AMTScanner.scan(), added a null check immediately after var iprange = this.parseIPv4Range(rangestr);. If parseIPv4Range returns null, the function now invokes the callback with an empty array (if provided) and returns early, before creating the dgram socket or registering the 'listening' handler, preventing the uncaught TypeError from dereferencing iprange.min/max.
🤖 Prompt for AI agents
In modules/amt-scanner.js around line 94, review and complete this code-review fix: scan() dereferences iprange.min/max without checking for null returned by parseIPv4Range.
What the draft fix changed: In AMTScanner.scan(), added a null check immediately after `var iprange = this.parseIPv4Range(rangestr);`. If parseIPv4Range returns null, the function now invokes the callback with an empty array (if provided) and returns early, before creating the dgram socket or registering the 'listening' handler, preventing the uncaught TypeError from dereferencing iprange.min/max.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| callback(server.scanResults); | ||
| } | ||
| server.parent.emit('found', server.scanResults); | ||
| delete server; | ||
| server.parent = null; | ||
| }, timeout); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🦩 🔵 AMTScanner.scan() uses 'delete server' on a local variable, which is a no-op and does not release the socket/timer
In the setTimeout callback inside AMTScanner.scan(), replaced the no-op delete server; statement with server.parent = null;, which breaks the reference cycle between the socket and the AMTScanner instance (server.parent = this) to aid garbage collection, instead of the ineffective delete-on-local-variable statement.
🤖 Prompt for AI agents
In modules/amt-scanner.js around line 97, review and complete this code-review fix: AMTScanner.scan() uses 'delete server' on a local variable, which is a no-op and does not release the socket/timer.
What the draft fix changed: In the setTimeout callback inside AMTScanner.scan(), replaced the no-op `delete server;` statement with `server.parent = null;`, which breaks the reference cycle between the socket and the AMTScanner instance (server.parent = this) to aid garbage collection, instead of the ineffective delete-on-local-variable statement.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| char* certbuf = NULL; | ||
| int certbuflen = 0; | ||
| int found = 0; | ||
| char certhash[32]; |
There was a problem hiding this comment.
🦩 🔴 Stack buffer overflow: certhash[32] written with 48-byte SHA-384 digest in signcheck_verifysign POSIX path
Changed char certhash[32]; to char certhash[48]; in the POSIX branch of signcheck_verifysign (the #else block handling non-Windows builds). This matches the 48-byte SHA-384 digest written into it by util_sha384(certbuf, certbuflen, certhash); a few lines later and eliminates the 16-byte stack buffer overflow that corrupted adjacent stack variables (agentid, ver, etc.). No other lines were touched.
🤖 Prompt for AI agents
In meshcore/signcheck.c around line 146, review and complete this code-review fix: Stack buffer overflow: certhash[32] written with 48-byte SHA-384 digest in signcheck_verifysign POSIX path.
What the draft fix changed: Changed `char certhash[32];` to `char certhash[48];` in the POSIX branch of `signcheck_verifysign` (the `#else` block handling non-Windows builds). This matches the 48-byte SHA-384 digest written into it by `util_sha384(certbuf, certbuflen, certhash);` a few lines later and eliminates the 16-byte stack buffer overflow that corrupted adjacent stack variables (`agentid`, `ver`, etc.). No other lines were touched.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
|
|
||
| // Stop the service | ||
| i = GetServiceState("Mesh Agent"); | ||
| if (i == 100) { printf("Mesh Agent is not installed.\r\n"); return; } |
There was a problem hiding this comment.
🦩 🟠 meshreset/main.c: missing return value on non-void main() path
In main() (meshreset/main.c), inside the "reset" branch, changed the bare return; on the if (i == 100) { printf(...); return; } early-exit path to return 0;, matching the function's declared int return type and eliminating the undefined-behavior/implicit-return warning on strict compilers (-Wreturn-type / MSVC C4716).
🤖 Prompt for AI agents
In meshreset/main.c around line 351, review and complete this code-review fix: meshreset/main.c: missing return value on non-void main() path.
What the draft fix changed: In main() (meshreset/main.c), inside the "reset" branch, changed the bare `return;` on the `if (i == 100) { printf(...); return; }` early-exit path to `return 0;`, matching the function's declared `int` return type and eliminating the undefined-behavior/implicit-return warning on strict compilers (-Wreturn-type / MSVC C4716).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -300,7 +300,7 @@ function SMBiosTables() | |||
| } | |||
| if (!retVal.AMT) | |||
There was a problem hiding this comment.
🦩 🟠 amtInfo dereferences data[131].peek() without checking data[131] exists
In amtInfo(), the final '!retVal.AMT' fallback branch now checks 'data[131] &&' before calling 'data[131].peek()', mirroring the existing 'data[130] &&' guard pattern used earlier in the same function. This prevents a TypeError when no type-131 SMBIOS structure is present.
🤖 Prompt for AI agents
In modules/smbios.js around line 301, review and complete this code-review fix: amtInfo dereferences data[131].peek() without checking data[131] exists.
What the draft fix changed: In amtInfo(), the final '!retVal.AMT' fallback branch now checks 'data[131] &&' before calling 'data[131].peek()', mirroring the existing 'data[130] &&' guard pattern used earlier in the same function. This prevents a TypeError when no type-131 SMBIOS structure is present.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| switch(name) | ||
| { | ||
| case 'tcpip-forward': |
There was a problem hiding this comment.
🦩 🔵 APF_GLOBAL_REQUEST handler reads port before validating len against buffer size
In the data event handler inside lme_heci() (modules/lme_heci.js), added explicit chunk.length bounds checks (with early break out of the switch) before each readUInt32BE/slice call that depends on data from the incoming buffer: APF_SERVICE_REQUEST (nameLen/name), APF_GLOBAL_REQUEST (nameLen/name, and within the tcpip-forward case the len/port reads), APF_CHANNEL_OPEN_CONFIRMATION (rChannel/sChannel/wSize), APF_PROTOCOLVERSION (major/minor/reason), APF_CHANNEL_WINDOW_ADJUST (rChannelId/bytesToAdd), APF_CHANNEL_DATA (rChannelId/dataLen and the subsequent data slice), and APF_CHANNEL_CLOSE (rChannelId). Malformed/truncated chunks now fall through without throwing instead of crashing the handler. Risk: the break only exits the inner switch(name) for the tcpip-forward case (as that break was already scoped to the inner switch in the original code), which is consistent with existing control flow; other cases' break exits the outer switch(cmd) as intended. Behavior for malformed packets is now "silently ignore" rather than crash, which seems like the appropriate minimal fix but a complete fix might also want to log/count these as protocol errors (not added to avoid scope creep beyond the finding).
🤖 Prompt for AI agents
In modules/lme_heci.js around line 166, review and complete this code-review fix: APF_GLOBAL_REQUEST handler reads port before validating len against buffer size.
What the draft fix changed: In the `data` event handler inside `lme_heci()` (`modules/lme_heci.js`), added explicit `chunk.length` bounds checks (with early `break` out of the switch) before each `readUInt32BE`/`slice` call that depends on data from the incoming buffer: APF_SERVICE_REQUEST (nameLen/name), APF_GLOBAL_REQUEST (nameLen/name, and within the `tcpip-forward` case the `len`/`port` reads), APF_CHANNEL_OPEN_CONFIRMATION (rChannel/sChannel/wSize), APF_PROTOCOLVERSION (major/minor/reason), APF_CHANNEL_WINDOW_ADJUST (rChannelId/bytesToAdd), APF_CHANNEL_DATA (rChannelId/dataLen and the subsequent data slice), and APF_CHANNEL_CLOSE (rChannelId). Malformed/truncated chunks now fall through without throwing instead of crashing the handler. Risk: the `break` only exits the inner `switch(name)` for the `tcpip-forward` case (as that break was already scoped to the inner switch in the original code), which is consistent with existing control flow; other cases' `break` exits the outer `switch(cmd)` as intended. Behavior for malformed packets is now "silently ignore" rather than crash, which seems like the appropriate minimal fix but a complete fix might also want to log/count these as protocol errors (not added to avoid scope creep beyond the finding).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -52,7 +52,6 @@ function start(updatePath) | |||
| { | |||
| try | |||
There was a problem hiding this comment.
🦩 🔵 update-helper.start() leaves half-written update file if copyFileSync fails mid-way
In the second .then() handler of start() in modules/update-helper.js, removed the unlinkSync(updatePath) call that ran before copyFileSync. Now copyFileSync(updatePath + '_unzipped', updatePath) runs first, overwriting updatePath atomically only after a successful read/copy, and updatePath + '_unzipped' is only unlinked after the copy succeeds. If copyFileSync throws, the original updatePath is left untouched (since it was never deleted) and the _unzipped file remains on disk for retry, since the unlink of _unzipped only happens after a successful copy.
🤖 Prompt for AI agents
In modules/update-helper.js around line 53, review and complete this code-review fix: update-helper.start() leaves half-written update file if copyFileSync fails mid-way.
What the draft fix changed: In the second `.then()` handler of `start()` in modules/update-helper.js, removed the `unlinkSync(updatePath)` call that ran before `copyFileSync`. Now `copyFileSync(updatePath + '_unzipped', updatePath)` runs first, overwriting updatePath atomically only after a successful read/copy, and `updatePath + '_unzipped'` is only unlinked after the copy succeeds. If `copyFileSync` throws, the original updatePath is left untouched (since it was never deleted) and the `_unzipped` file remains on disk for retry, since the unlink of `_unzipped` only happens after a successful copy.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| // Spawn a child process in the appropriate user session, and relay the response back via stdout | ||
| var mod = Buffer.from(getJSModule('win-deskutils')).toString('base64'); |
There was a problem hiding this comment.
🦩 🔵 Unused local variable mod computed before being discarded
Removed the unused local variable mod (var mod = Buffer.from(getJSModule('win-deskutils')).toString('base64');) from sessionDispatch() in modules/win-deskutils.js, since it was computed but never referenced; the module source is still correctly obtained via the existing getJSModule('win-deskutils') call inside the env object passed to execFile.
🤖 Prompt for AI agents
In modules/win-deskutils.js around line 80, review and complete this code-review fix: Unused local variable `mod` computed before being discarded.
What the draft fix changed: Removed the unused local variable `mod` (`var mod = Buffer.from(getJSModule('win-deskutils')).toString('base64');`) from `sessionDispatch()` in `modules/win-deskutils.js`, since it was computed but never referenced; the module source is still correctly obtained via the existing `getJSModule('win-deskutils')` call inside the `env` object passed to `execFile`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| // Windows will automatically restart explorer if you kill it | ||
| process.kill(pids[0]); | ||
| } | ||
| return (this.autoHide(tsid)); | ||
| return (value === true); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| module.exports = new winutils(); No newline at end of file | ||
| module.exports = new winutils(); |
There was a problem hiding this comment.
🦩 🔵 autoHide() toggles taskbar state based on stale local variable after re-querying registry
In winutils.taskBar.autoHide (modules/win-utils.js), the else branch (value != null) no longer recurses via this.autoHide(tsid) to re-read the registry. It now returns (value === true) directly, reflecting the known new state that was just written, avoiding the extra registry round-trip, user/domain lookup, and potential stale read during explorer restart.
🤖 Prompt for AI agents
In modules/win-utils.js around line 42, review and complete this code-review fix: autoHide() toggles taskbar state based on stale local variable after re-querying registry.
What the draft fix changed: In winutils.taskBar.autoHide (modules/win-utils.js), the else branch (value != null) no longer recurses via `this.autoHide(tsid)` to re-read the registry. It now returns `(value === true)` directly, reflecting the known new state that was just written, avoiding the extra registry round-trip, user/domain lookup, and potential stale read during explorer restart.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| while (i < bufferLen) | ||
| { | ||
| EVP_EncryptUpdate(ptrs->encryptedCTX, (unsigned char*)out, &outLen, (unsigned char*)(buffer + i), bufferLen - i > 4096 ? 4096 : bufferLen - i); | ||
| if (!EVP_EncryptUpdate(ptrs->encryptedCTX, (unsigned char*)out, &outLen, (unsigned char*)(buffer + i), bufferLen - i > 4096 ? 4096 : bufferLen - i)) | ||
| { | ||
| // Encrypt Error | ||
| ILibDuktape_EventEmitter_SetupEmit(ptrs->encrypted->readableStream->ctx, ptrs->encrypted->readableStream->object, "error"); // [emit][this][error] | ||
| duk_push_string(ptrs->encrypted->readableStream->ctx, "Encrypt Error"); // [emit][this][error][msg] | ||
| if (duk_pcall_method(ptrs->encrypted->readableStream->ctx, 2) != 0) // [ret] | ||
| { | ||
| ILibDuktape_Process_UncaughtException(ptrs->encrypted->readableStream->ctx); | ||
| } | ||
| duk_pop(ptrs->encrypted->readableStream->ctx); // ... | ||
| return(ILibTransport_DoneState_ERROR); | ||
| } | ||
| result = ILibDuktape_DuplexStream_WriteData(ptrs->encrypted, out, outLen); | ||
| i += 4096; | ||
| } |
There was a problem hiding this comment.
🦩 🔵 ILibDuktape_EncryptionStream_decrypted_WriteSink ignores EVP_EncryptUpdate return value
In ILibDuktape_EncryptionStream_decrypted_WriteSink (microscript/ILibDuktape_EncryptionStream.c), the EVP_EncryptUpdate return value is now checked. On failure it emits an "error" event on the encrypted stream's readable side (mirroring the pattern used in ILibDuktape_EncryptionStream_encrypted_WriteEndSink for EVP_DecryptFinal failures) and returns ILibTransport_DoneState_ERROR instead of proceeding to write stale/garbage outLen data. This uses the existing ILibDuktape_EventEmitter_SetupEmit/duk_pcall_method/ILibDuktape_Process_UncaughtException pattern already present in the file, and the ILibTransport_DoneState_ERROR enum value is assumed to exist in ILibTransport's DoneState enum (used elsewhere in the codebase's transport abstraction) alongside COMPLETE/INCOMPLETE; if that enum value does not exist, this would need to fall back to COMPLETE/INCOMPLETE with just the error emission, which is a risk given I cannot see the enum definition in this file.
🤖 Prompt for AI agents
In microscript/ILibDuktape_EncryptionStream.c around line 58, review and complete this code-review fix: ILibDuktape_EncryptionStream_decrypted_WriteSink ignores EVP_EncryptUpdate return value.
What the draft fix changed: In ILibDuktape_EncryptionStream_decrypted_WriteSink (microscript/ILibDuktape_EncryptionStream.c), the EVP_EncryptUpdate return value is now checked. On failure it emits an "error" event on the encrypted stream's readable side (mirroring the pattern used in ILibDuktape_EncryptionStream_encrypted_WriteEndSink for EVP_DecryptFinal failures) and returns ILibTransport_DoneState_ERROR instead of proceeding to write stale/garbage outLen data. This uses the existing ILibDuktape_EventEmitter_SetupEmit/duk_pcall_method/ILibDuktape_Process_UncaughtException pattern already present in the file, and the ILibTransport_DoneState_ERROR enum value is assumed to exist in ILibTransport's DoneState enum (used elsewhere in the codebase's transport abstraction) alongside COMPLETE/INCOMPLETE; if that enum value does not exist, this would need to fall back to COMPLETE/INCOMPLETE with just the error emission, which is a risk given I cannot see the enum definition in this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 24 review findings across 21 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
modules/amt-scanner.js:94modules/amt-scanner.js:97meshcore/signcheck.c:146meshreset/main.c:351modules/smbios.js:301meshcore/wincrypto-dualmode.cpp:345modules/parseXml.js:47modules/upnp.js:269meshcore/openframe_file_logger.h:243modules/linux-dbus.js:87modules/linux-dbus.js:55modules/win-registry.js:203modules/win-registry.js:208meshcore/KVM/MacOS/mac_events.c:282meshcore/KVM/Linux/linux_tile.c:70modules/default_route.js:21modules/http-digest.js:44modules/PE_Parser.js:136meshcore/KVM/MacOS/mac_kvm_auth.c:105modules/lme_heci.js:166modules/update-helper.js:53modcomputed before being discardedmodules/win-deskutils.js:80modules/win-utils.js:42microscript/ILibDuktape_EncryptionStream.c:58What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
b0044794-23ba-44ff-8104-85f171bb5cdaMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akq5g67 MeshAgent review findings sweep (12 PRs)