Skip to content

fix(adhoc-sweep-fixes): 35 review findings across 25 files #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 25 commits into
base: master
Choose a base branch
from
Draft

fix(adhoc-sweep-fixes): 35 review findings across 25 files #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9a43456
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
be80d30
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
849a16b
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
60fe92d
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
7c32d05
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
1edcf03
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
e77e300
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
01dd311
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
ed4bac6
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
589bd0e
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
7fa6904
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
dbb9fbb
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
91e5d7c
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
ba0fd83
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
9519cb0
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
3840ccb
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
cd9b903
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
0fd44a3
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
194a329
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
3afcb9f
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
c5e07b5
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
ad80f6c
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
3c80c69
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
a749e47
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
23fd847
fix(adhoc-sweep-fixes): 35 review findings across 25 files
flamingo[bot] Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .github/workflows/build-openssl-linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ jobs:

- name: Create build report
run: |
cat > BUILD_REPORT.md << 'EOF'
cat > BUILD_REPORT.md << EOF
# OpenSSL 3.5.4 Linux Build Report

## Build Date
Comment on lines 271 to 277

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 build-openssl-linux.yml BUILD_REPORT.md heredoc uses unescaped $(date) and $(ls) expansions that will not execute inside a quoted heredoc

In the "Create build report" step of the combine-artifacts job, changed the heredoc delimiter from quoted << 'EOF' to unquoted << EOF so that $(date ...) and $(ls ...) command substitutions are actually evaluated by the shell when generating BUILD_REPORT.md. Since unquoting the delimiter also re-enables expansion of literal backticks/$/backslashes elsewhere in the body, I escaped the two Markdown code-fence backtick sequences (```) and the inline code backticks around the workflow filename with backslashes (\``and`...``) so they are emitted literally instead of being misinterpreted, preserving the rest of the report text unchanged.

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents 
In .github/workflows/build-openssl-linux.yml around line 232, review and complete this code-review fix: build-openssl-linux.yml BUILD_REPORT.md heredoc uses unescaped $(date) and $(ls) expansions that will not execute inside a quoted heredoc.
What the draft fix changed: In the "Create build report" step of the `combine-artifacts` job, changed the heredoc delimiter from quoted `<< 'EOF'` to unquoted `<< EOF` so that `$(date ...)` and `$(ls ...)` command substitutions are actually evaluated by the shell when generating BUILD_REPORT.md. Since unquoting the delimiter also re-enables expansion of literal backticks/`$`/backslashes elsewhere in the body, I escaped the two Markdown code-fence backtick sequences (```` ``` ````) and the inline code backticks around the workflow filename with backslashes (`\`\`\`` and `\`...\``) so they are emitted literally instead of being misinterpreted, preserving the rest of the report text unchanged.

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -327,12 +327,12 @@ jobs:

## Build Configuration
All builds use identical OpenSSL configure options:
```
\`\`\`
no-weak-ssl-ciphers no-srp no-psk no-comp no-zlib no-zlib-dynamic
no-threads no-dso no-shared no-asm no-rc5 no-idea
no-md4 no-rmd160 no-ssl no-ssl3 no-seed no-camellia no-bf no-cast
no-md2 no-mdc2
```
\`\`\`

## Compiler
- GCC from Debian Bullseye (glibc builds)
Expand All @@ -348,7 +348,7 @@ jobs:
- MIPS: Cross-compilation on amd64 host

## GitHub Actions Workflow
Built using: `.github/workflows/build-openssl-linux.yml`
Built using: \`.github/workflows/build-openssl-linux.yml\`

EOF

Expand All @@ -358,3 +358,4 @@ jobs:
name: build-report
path: BUILD_REPORT.md
retention-days: 90

14 changes: 8 additions & 6 deletions .github/workflows/build-openssl-macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,15 @@ jobs:
- name: Upload combined artifact
uses: actions/upload-artifact@v4
with:
name: openssl-3.5.4-macos-all
name: openssl-${{ github.event.inputs.openssl_version || '3.5.4' }}-macos-all
path: openssl-macos-libs/
retention-days: 90

- name: Create build report
run: |
cat > BUILD_REPORT.md << 'EOF'
# OpenSSL 3.5.4 macOS Build Report
OPENSSL_VERSION="${{ github.event.inputs.openssl_version || '3.5.4' }}"
cat > BUILD_REPORT.md << EOF
# OpenSSL $OPENSSL_VERSION macOS Build Report

## Build Date
$(date -u +"%Y-%m-%d %H:%M:%S UTC")
Comment on lines 113 to 127

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔵 BUILD_REPORT.md generation uses a single-quoted heredoc so $(date ...) and other command substitutions are never expanded

In the "Create build report" step, changed the heredoc delimiter from single-quoted << 'EOF' to unquoted << EOF, and escaped literal ` characters (backtick code fences) with backslashes so they are not misinterpreted as command substitution by the shell now that the heredoc body is expanded. This allows $(date -u +...) and the $(ls -lh ...) listings to be executed and substituted into BUILD_REPORT.md as intended.

🤖 Prompt for AI agents 
In .github/workflows/build-openssl-macos.yml around line 112, review and complete this code-review fix: BUILD_REPORT.md generation uses a single-quoted heredoc so $(date ...) and other command substitutions are never expanded.
What the draft fix changed: In the "Create build report" step, changed the heredoc delimiter from single-quoted `<< 'EOF'` to unquoted `<< EOF`, and escaped literal `` ` `` characters (backtick code fences) with backslashes so they are not misinterpreted as command substitution by the shell now that the heredoc body is expanded. This allows `$(date -u +...)` and the `$(ls -lh ...)` listings to be executed and substituted into BUILD_REPORT.md as intended.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Comment on lines 113 to 127

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔵 GitHub Actions matrix downloads openssl_version input string but combine-artifacts job's build report hardcodes version 3.5.4 independent of the workflow_dispatch input

Introduced OPENSSL_VERSION="${{ github.event.inputs.openssl_version || '3.5.4' }}" in the "Create build report" step and used $OPENSSL_VERSION in the report title instead of the hardcoded "3.5.4". Also changed the "Upload combined artifact" step's name: field from hardcoded openssl-3.5.4-macos-all to openssl-${{ github.event.inputs.openssl_version || '3.5.4' }}-macos-all so the artifact name reflects the actual dispatched version. Not verified against actual GitHub Actions artifact-name character restrictions for arbitrary version strings, but default behavior is unchanged.

🤖 Prompt for AI agents 
In .github/workflows/build-openssl-macos.yml around line 105, review and complete this code-review fix: GitHub Actions matrix downloads openssl_version input string but combine-artifacts job's build report hardcodes version 3.5.4 independent of the workflow_dispatch input.
What the draft fix changed: Introduced `OPENSSL_VERSION="${{ github.event.inputs.openssl_version || '3.5.4' }}"` in the "Create build report" step and used `$OPENSSL_VERSION` in the report title instead of the hardcoded "3.5.4". Also changed the "Upload combined artifact" step's `name:` field from hardcoded `openssl-3.5.4-macos-all` to `openssl-${{ github.event.inputs.openssl_version || '3.5.4' }}-macos-all` so the artifact name reflects the actual dispatched version. Not verified against actual GitHub Actions artifact-name character restrictions for arbitrary version strings, but default behavior is unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

Expand All @@ -139,20 +140,20 @@ jobs:

## Build Configuration
All builds use identical OpenSSL configure options:
```
\`\`\`
no-weak-ssl-ciphers no-srp no-psk no-comp no-zlib no-zlib-dynamic
no-threads no-dso no-shared no-asm no-rc5 no-idea
no-md4 no-rmd160 no-ssl no-ssl3 no-seed no-camellia no-bf no-cast
no-md2 no-mdc2
```
\`\`\`

## Compiler
- Xcode Command Line Tools (Clang)
- x86_64: darwin64-x86_64-cc target with -mmacosx-version-min=10.13
- ARM64: darwin64-arm64-cc target with -mmacosx-version-min=11.0

## GitHub Actions Workflow
Built using: `.github/workflows/build-openssl-macos.yml`
Built using: \`.github/workflows/build-openssl-macos.yml\`

EOF

Expand All @@ -162,3 +163,4 @@ jobs:
name: build-report
path: BUILD_REPORT.md
retention-days: 90

9 changes: 5 additions & 4 deletions .github/workflows/build-openssl-windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ jobs:

- name: Create build report
run: |
cat > BUILD_REPORT.md << 'EOF'
cat > BUILD_REPORT.md << EOF
# OpenSSL 3.5.4 Windows Build Report

## Build Date
Comment on lines 134 to 140

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 OpenSSL build report heredoc uses unescaped $(...) command substitution that will not execute inside a single-quoted heredoc

In the 'Create build report' step (combine-artifacts job), changed the heredoc delimiter from the quoted << 'EOF' to the unquoted << EOF so that $(date ...) and $(ls -lh ...) command substitutions are evaluated by the shell when the report is generated. Since the heredoc body now undergoes shell expansion, the literal backtick-fenced code blocks containing ` characters were unaffected, but the literal .github/workflows/build-openssl-windows.yml path is wrapped in backticks for markdown — those backticks are now escaped as \...`to prevent them from being interpreted as command substitution by the shell (backticks are also a substitution mechanism in unquoted heredocs). Same escaping applied to the ``` ` code fences surrounding the configure options block.

🤖 Prompt for AI agents 
In .github/workflows/build-openssl-windows.yml around line 130, review and complete this code-review fix: OpenSSL build report heredoc uses unescaped $(...) command substitution that will not execute inside a single-quoted heredoc.
What the draft fix changed: In the 'Create build report' step (combine-artifacts job), changed the heredoc delimiter from the quoted `<< 'EOF'` to the unquoted `<< EOF` so that `$(date ...)` and `$(ls -lh ...)` command substitutions are evaluated by the shell when the report is generated. Since the heredoc body now undergoes shell expansion, the literal backtick-fenced code blocks containing `` ` `` characters were unaffected, but the literal `.github/workflows/build-openssl-windows.yml` path is wrapped in backticks for markdown — those backticks are now escaped as `\`...\`` to prevent them from being interpreted as command substitution by the shell (backticks are also a substitution mechanism in unquoted heredocs). Same escaping applied to the ` ```  ` code fences surrounding the configure options block.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -163,19 +163,19 @@ jobs:

## Build Configuration
All builds use identical OpenSSL configure options:
```
\`\`\`
no-weak-ssl-ciphers no-srp no-psk no-comp no-zlib no-zlib-dynamic
no-threads no-err no-dso no-shared no-asm no-rc5 no-idea
no-md4 no-rmd160 no-ssl no-ssl3 no-seed no-camellia no-bf no-cast
no-md2 no-mdc2
```
\`\`\`

## Compiler
- Visual Studio 2022 (MSVC)
- Static runtime linking (/MT and /MTd)

## GitHub Actions Workflow
Built using: `.github/workflows/build-openssl-windows.yml`
Built using: \`.github/workflows/build-openssl-windows.yml\`

EOF

Expand All @@ -185,3 +185,4 @@ jobs:
name: build-report
path: BUILD_REPORT.md
retention-days: 90

16 changes: 15 additions & 1 deletion docs/modules/apply_labels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#!/usr/bin/env python3

import argparse
import os
import subprocess
import re
import sys

# Label mappings
LABELS = {
Expand Down Expand Up @@ -54,6 +56,10 @@ def apply_label(filepath, platform):
if platform not in LABELS:
return False

if sys.platform != 'darwin':
print(f"Error applying label to {filepath}: Finder labels via xattr are only supported on macOS")
return False

label_hex, label_name = LABELS[platform]
hex_value = f"000000000000000000{label_hex}00000000000000000000000000000000000000000000"

Comment on lines 56 to 65

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔵 apply_labels.py uses macOS-only xattr/Finder labeling with no cross-platform guard

In apply_label(), added a sys.platform != 'darwin' guard (with import sys added) before invoking subprocess.run(['xattr', ...]). On non-macOS platforms it now prints a clear error message and returns False instead of letting subprocess.run raise FileNotFoundError for the missing xattr binary. Behavior on macOS is unchanged.

🤖 Prompt for AI agents 
In docs/modules/apply_labels.py around line 52, review and complete this code-review fix: apply_labels.py uses macOS-only xattr/Finder labeling with no cross-platform guard.
What the draft fix changed: In `apply_label()`, added a `sys.platform != 'darwin'` guard (with `import sys` added) before invoking `subprocess.run(['xattr', ...])`. On non-macOS platforms it now prints a clear error message and returns `False` instead of letting `subprocess.run` raise `FileNotFoundError` for the missing `xattr` binary. Behavior on macOS is unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

Expand All @@ -69,7 +75,15 @@ def apply_label(filepath, platform):
return False

def main():
doc_dir = '/Users/peet/GitHub/MeshAgent_installer/bin/modules_documentation'
parser = argparse.ArgumentParser()
parser.add_argument(
'doc_dir',
nargs='?',
default=os.path.join(os.path.dirname(os.path.abspath(__file__)), 'modules_documentation'),
help='Directory containing module documentation markdown files'
)
args = parser.parse_args()
doc_dir = args.doc_dir

print("Processing module documentation files...")
print("=" * 60)
Comment on lines 75 to 89

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 apply_labels.py hardcodes a developer's local macOS path, making the script unusable elsewhere

In main(), replaced the hardcoded doc_dir = '/Users/peet/GitHub/...' string with an argparse-based CLI argument (doc_dir, positional, optional). Added import argparse. To preserve zero-arg usability, the default falls back to a modules_documentation directory located relative to the script's own path via os.path.dirname(os.path.abspath(__file__)), rather than requiring the argument every time. Users can now run apply_labels.py /path/to/docs on any machine.

🤖 Prompt for AI agents 
In docs/modules/apply_labels.py around line 77, review and complete this code-review fix: apply_labels.py hardcodes a developer's local macOS path, making the script unusable elsewhere.
What the draft fix changed: In `main()`, replaced the hardcoded `doc_dir = '/Users/peet/GitHub/...'` string with an `argparse`-based CLI argument (`doc_dir`, positional, optional). Added `import argparse`. To preserve zero-arg usability, the default falls back to a `modules_documentation` directory located relative to the script's own path via `os.path.dirname(os.path.abspath(__file__))`, rather than requiring the argument every time. Users can now run `apply_labels.py /path/to/docs` on any machine.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

Expand Down
2 changes: 2 additions & 0 deletions meshcore/KVM/Linux/linux_compression.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ int write_JPEG_buffer(JSAMPLE * image_buffer, int image_width, int image_height,

jpeg_create_compress(&cinfo);
cinfo.dest = (struct jpeg_destination_mgr *) malloc(sizeof(struct jpeg_destination_mgr));

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 write_JPEG_buffer has no loop-exit check for row_pointer writes, unconditional infinite risk absent but return code always 0

In write_JPEG_buffer (linux_compression.c), added a NULL check immediately after cinfo.dest = (struct jpeg_destination_mgr *) malloc(...), calling ILIBCRITICALEXIT(254) on failure before any dereference of cinfo.dest, consistent with the defensive pattern already used in init_destination, empty_output_buffer, and term_destination. This directly fixes the NULL pointer dereference risk on OOM. The finding also mentions the function always returning 0 regardless of jpeg_finish_compress/callback success; I did not change the return value semantics because jpeg_finish_compress/jpeg_write_scanlines failures in libjpeg are reported via the error_exit longjmp/exit mechanism (jpeg_error_handler) rather than return codes, so altering the return contract would be a larger, riskier behavioral change beyond the scope of a minimal fix — a reviewer wanting return-code propagation would need to introduce setjmp-based error handling, which is architectural and not addressed here.

🤖 Prompt for AI agents 
In meshcore/KVM/Linux/linux_compression.c around line 97, review and complete this code-review fix: write_JPEG_buffer has no loop-exit check for row_pointer writes, unconditional infinite risk absent but return code always 0.
What the draft fix changed: In write_JPEG_buffer (linux_compression.c), added a NULL check immediately after `cinfo.dest = (struct jpeg_destination_mgr *) malloc(...)`, calling `ILIBCRITICALEXIT(254)` on failure before any dereference of `cinfo.dest`, consistent with the defensive pattern already used in init_destination, empty_output_buffer, and term_destination. This directly fixes the NULL pointer dereference risk on OOM. The finding also mentions the function always returning 0 regardless of jpeg_finish_compress/callback success; I did not change the return value semantics because jpeg_finish_compress/jpeg_write_scanlines failures in libjpeg are reported via the error_exit longjmp/exit mechanism (jpeg_error_handler) rather than return codes, so altering the return contract would be a larger, riskier behavioral change beyond the scope of a minimal fix — a reviewer wanting return-code propagation would need to introduce setjmp-based error handling, which is architectural and not addressed here.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

if (cinfo.dest == NULL) { ILIBCRITICALEXIT(254); }
cinfo.dest->init_destination = &init_destination;
cinfo.dest->empty_output_buffer = &empty_output_buffer;
cinfo.dest->term_destination = &term_destination;
Expand Down Expand Up @@ -132,3 +133,4 @@ int write_JPEG_buffer(JSAMPLE * image_buffer, int image_width, int image_height,

return 0;
}

14 changes: 11 additions & 3 deletions meshcore/meshinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,13 @@ int info_GetLocalInterfaces(char* data, int maxdata)
pAdapterInfo = (IP_ADAPTER_INFO *)malloc(sizeof(IP_ADAPTER_INFO));
if (pAdapterInfo == NULL) return 0;
ulOutBufLen = sizeof(IP_ADAPTER_INFO);
if (GetAdaptersInfo(pAdapterInfo, &ulOutBufLen) != ERROR_SUCCESS) { free(pAdapterInfo); if (ulOutBufLen == 0) return 0; pAdapterInfo = (IP_ADAPTER_INFO *)malloc(ulOutBufLen); }
if (GetAdaptersInfo(pAdapterInfo, &ulOutBufLen) != ERROR_SUCCESS)
{
free(pAdapterInfo);
if (ulOutBufLen == 0) return 0;
pAdapterInfo = (IP_ADAPTER_INFO *)malloc(ulOutBufLen);
if (pAdapterInfo == NULL) return 0;
}

// Get the list of all local interfaces
if ((dwRetVal = GetAdaptersInfo(pAdapterInfo, &ulOutBufLen)) != ERROR_SUCCESS || ulOutBufLen == 0) { free(pAdapterInfo); return 0; }
Comment on lines 85 to 97

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 info_GetLocalInterfaces (Windows) leaks pAdapterInfo/pAdapterAddresses on realloc failure paths and does not check malloc failures

In info_GetLocalInterfaces (Windows/WINSOCK2 branch), added NULL checks after the secondary pAdapterInfo = (IP_ADAPTER_INFO *)malloc(ulOutBufLen); and pAdapterAddresses = (IP_ADAPTER_ADDRESSES *)malloc(ulOutBufLen); reallocation calls, returning 0 (and freeing the sibling allocation where already allocated) instead of falling through to GetAdaptersInfo/GetAdaptersAddresses with a NULL buffer pointer. This directly fixes the unchecked-malloc-then-dereference bug described in the finding.

🤖 Prompt for AI agents 
In meshcore/meshinfo.c around line 84, review and complete this code-review fix: info_GetLocalInterfaces (Windows) leaks pAdapterInfo/pAdapterAddresses on realloc failure paths and does not check malloc failures.
What the draft fix changed: In info_GetLocalInterfaces (Windows/WINSOCK2 branch), added NULL checks after the secondary `pAdapterInfo = (IP_ADAPTER_INFO *)malloc(ulOutBufLen);` and `pAdapterAddresses = (IP_ADAPTER_ADDRESSES *)malloc(ulOutBufLen);` reallocation calls, returning 0 (and freeing the sibling allocation where already allocated) instead of falling through to GetAdaptersInfo/GetAdaptersAddresses with a NULL buffer pointer. This directly fixes the unchecked-malloc-then-dereference bug described in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

Expand All @@ -107,6 +113,7 @@ int info_GetLocalInterfaces(char* data, int maxdata)
return 0;
}
pAdapterAddresses = (IP_ADAPTER_ADDRESSES *)malloc(ulOutBufLen);
if (pAdapterAddresses == NULL) { free(pAdapterInfo); return 0; }
}

// Get the list of all local interfaces
Expand Down Expand Up @@ -366,9 +373,9 @@ int info_GetLocalInterfaces(char* data, int maxdata)
{
++size;
// realloc buffer size until no overflow occurs
if ((ifc.ifc_req = realloc(ifc.ifc_req, IFRSIZE)) == NULL) return 0;
if ((ifc.ifc_req = realloc(ifc.ifc_req, IFRSIZE)) == NULL) { close(sockfd); return 0; }
ifc.ifc_len = IFRSIZE;
if (ioctl(sockfd, SIOCGIFCONF, &ifc) != 0) return 0;
if (ioctl(sockfd, SIOCGIFCONF, &ifc) != 0) { free(ifc.ifc_req); close(sockfd); return 0; }
} while (IFRSIZE <= ifc.ifc_len);

ifr = ifc.ifc_req;
Comment on lines 373 to 381

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 POSIX info_GetLocalInterfaces leaks ifc.ifc_req on early-return paths before the loop

In the POSIX branch of info_GetLocalInterfaces, the do/while loop that grows ifc.ifc_req via realloc now closes sockfd before returning 0 on realloc failure, and frees ifc.ifc_req plus closes sockfd before returning 0 when ioctl(sockfd, SIOCGIFCONF, &ifc) fails, eliminating the previously leaked buffer and file descriptor on those early-return paths.

🤖 Prompt for AI agents 
In meshcore/meshinfo.c around line 258, review and complete this code-review fix: POSIX info_GetLocalInterfaces leaks ifc.ifc_req on early-return paths before the loop.
What the draft fix changed: In the POSIX branch of info_GetLocalInterfaces, the do/while loop that grows `ifc.ifc_req` via realloc now closes `sockfd` before returning 0 on realloc failure, and frees `ifc.ifc_req` plus closes `sockfd` before returning 0 when `ioctl(sockfd, SIOCGIFCONF, &ifc)` fails, eliminating the previously leaked buffer and file descriptor on those early-return paths.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -650,3 +657,4 @@ int MeshInfo_PowerState(enum AgentPowerStateActions flg, int force)

#endif
#endif

30 changes: 3 additions & 27 deletions meshcore/openframe_file_logger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ OpenFrame File Logger - Duplicates printf to both console and file
Usage: Call enable_file_logging() at the start of main()

Features:
- Single log file: meshagent.log
- Single log file: meshcentral-agent.log
- Auto-rotation at 10MB
- Keeps only 1 archive (meshagent.log.old.gz)
- Keeps only 1 archive (meshcentral-agent.log.old.gz)
*/

#ifndef OPENFRAME_FILE_LOGGER_H
Comment on lines 3 to 11

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔵 openframe_file_logger.h header comment/macro names disagree with actual LOG_FILENAME value

Updated the top-of-file doc comment (lines 5-7) to say "meshcentral-agent.log" and "meshcentral-agent.log.old.gz" instead of "meshagent.log" and "meshagent.log.old.gz", matching the actual LOG_FILENAME/LOG_ARCHIVE_FILENAME macro values. Left the runtime log messages ("MeshAgent Log Started", etc.) unchanged since the finding only flagged the header doc comment as stale/misleading, and changing user-visible log text was out of scope for a minimal fix.

🤖 Prompt for AI agents 
In meshcore/openframe_file_logger.h around line 1, review and complete this code-review fix: openframe_file_logger.h header comment/macro names disagree with actual LOG_FILENAME value.
What the draft fix changed: Updated the top-of-file doc comment (lines 5-7) to say "meshcentral-agent.log" and "meshcentral-agent.log.old.gz" instead of "meshagent.log" and "meshagent.log.old.gz", matching the actual LOG_FILENAME/LOG_ARCHIVE_FILENAME macro values. Left the runtime log messages ("MeshAgent Log Started", etc.) unchanged since the finding only flagged the header doc comment as stale/misleading, and changing user-visible log text was out of scope for a minimal fix.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -39,6 +39,7 @@ Usage: Call enable_file_logging() at the start of main()
#include <io.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <zlib.h>
#endif

/* Macro to ignore return values */
Expand Down Expand Up @@ -173,30 +174,6 @@ static inline long get_file_size(const char* filepath) {
}

static inline int compress_file_to_gzip(const char* source_path, const char* dest_path) {
#ifdef WIN32
FILE* src = fopen(source_path, "rb");
FILE* dst = fopen(dest_path, "wb");

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 compress_file_to_gzip WIN32 branch never actually gzip-compresses; produces a plain-copy "archive" with .gz extension and misleading fallback semantics

In compress_file_to_gzip(), removed the WIN32-only plain-copy branch (which used fopen/fwrite with no compression) and replaced the entire function with the single zlib-based implementation (fopen + gzopen/gzwrite/gzclose) that was previously only compiled for non-WIN32. Added #include <zlib.h> to the WIN32 include block so gzopen/gzwrite/gzclose resolve on Windows too. This makes rotated archives real gzip streams on all platforms, matching the .gz extension and the header's documented behavior. Risk: this assumes zlib is available and linkable on the Windows build (a build-system/toolchain concern outside this header); if the Windows build previously avoided zlib on purpose (e.g., no zlib dependency configured for WIN32 target), this change could break compilation until the build is updated to link zlib for Windows.

🤖 Prompt for AI agents 
In meshcore/openframe_file_logger.h around line 178, review and complete this code-review fix: compress_file_to_gzip WIN32 branch never actually gzip-compresses; produces a plain-copy "archive" with .gz extension and misleading fallback semantics.
What the draft fix changed: In compress_file_to_gzip(), removed the WIN32-only plain-copy branch (which used fopen/fwrite with no compression) and replaced the entire function with the single zlib-based implementation (fopen + gzopen/gzwrite/gzclose) that was previously only compiled for non-WIN32. Added `#include <zlib.h>` to the WIN32 include block so gzopen/gzwrite/gzclose resolve on Windows too. This makes rotated archives real gzip streams on all platforms, matching the `.gz` extension and the header's documented behavior. Risk: this assumes zlib is available and linkable on the Windows build (a build-system/toolchain concern outside this header); if the Windows build previously avoided zlib on purpose (e.g., no zlib dependency configured for WIN32 target), this change could break compilation until the build is updated to link zlib for Windows.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

char buffer[8192];
size_t bytes;

if (!src || !dst) {
if (src) fclose(src);
if (dst) fclose(dst);
return 0;
}

while ((bytes = fread(buffer, 1, sizeof(buffer), src)) > 0) {
if (fwrite(buffer, 1, bytes, dst) != bytes) {
fclose(src);
fclose(dst);
return 0;
}
}

fclose(src);
fclose(dst);
return 1;
#else
FILE* src = fopen(source_path, "rb");
gzFile dst = gzopen(dest_path, "wb9");
char buffer[8192];
Expand All @@ -219,7 +196,6 @@ static inline int compress_file_to_gzip(const char* source_path, const char* des
fclose(src);
gzclose(dst);
return 1;
#endif
}

static inline int rotate_log_file(void) {
Expand Down
3 changes: 2 additions & 1 deletion microstack/ILibAsyncUDPSocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ void ILibAsyncUDPSocket_SetMulticastTTL(ILibAsyncUDPSocket_SocketModule module,
{
struct sockaddr_in6 localAddress;
#if defined(__SYMBIAN32__)
return 0;
return;
#else
#if defined(WIN32) || defined(_WIN32_WCE)
SOCKET s = *((SOCKET*)ILibAsyncSocket_GetSocket(module));
Comment on lines 286 to 292

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔵 return 0 used inside void-returning ILibAsyncUDPSocket_SetMulticastTTL under SYMBIAN32 guard

In ILibAsyncUDPSocket_SetMulticastTTL, changed return 0; to return; inside the #if defined(__SYMBIAN32__) branch, since the function is declared void and returning a value is invalid. This is a minimal, mechanical fix that resolves the latent compile-time defect noted in the finding without altering behavior on any currently-built platform (the guard remains dead code on all supported toolchains).

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents 
In microstack/ILibAsyncUDPSocket.c around line 224, review and complete this code-review fix: return 0 used inside void-returning ILibAsyncUDPSocket_SetMulticastTTL under __SYMBIAN32__ guard.
What the draft fix changed: In ILibAsyncUDPSocket_SetMulticastTTL, changed `return 0;` to `return;` inside the `#if defined(__SYMBIAN32__)` branch, since the function is declared `void` and returning a value is invalid. This is a minimal, mechanical fix that resolves the latent compile-time defect noted in the finding without altering behavior on any currently-built platform (the guard remains dead code on all supported toolchains).

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer

Expand All @@ -310,3 +310,4 @@ void ILibAsyncUDPSocket_SetMulticastLoopback(ILibAsyncUDPSocket_SocketModule mod
ILibAsyncSocket_GetLocalInterface(module, (struct sockaddr*)&localAddress);
if (setsockopt(s, localAddress.sin6_family == PF_INET6 ? IPPROTO_IPV6 : IPPROTO_IP, localAddress.sin6_family == PF_INET6 ? IPV6_MULTICAST_LOOP : IP_MULTICAST_LOOP, (char*)&loopback, sizeof(loopback)) != 0) ILIBCRITICALERREXIT(253);
}

4 changes: 2 additions & 2 deletions microstack/ILibMulticastSocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ int ILibMulticastSocket_ResetMulticast(struct ILibMulticastSocket_StateModule *m
#if !defined(NACL)
if (setsockopt(socket, IPPROTO_IP, IP_MULTICAST_TTL, (const char*)&(module->TTL), sizeof(int)) != 0) ILIBCRITICALERREXIT(253);
if (setsockopt(socket, IPPROTO_IP, IP_MULTICAST_LOOP, (const char*)&(module->Loopback), sizeof(int)) != 0) ILIBCRITICALERREXIT(253);
if (setsockopt(socket, IPPROTO_IP, IP_MULTICAST_LOOP, (const char*)&(module->Loopback), sizeof(int)) != 0) ILIBCRITICALERREXIT(253);
#endif
module->AddressListV4[i].sin_port = 0;

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Duplicate IP_MULTICAST_LOOP setsockopt call, missing IP_MULTICAST_TTL-equivalent redundancy is a copy/paste bug

In ILibMulticastSocket_ResetMulticast, removed the duplicate setsockopt(socket, IPPROTO_IP, IP_MULTICAST_LOOP, ...) line, leaving a single IP_MULTICAST_TTL call followed by a single IP_MULTICAST_LOOP call per interface socket. No new option was invented (e.g., IP_MULTICAST_IF/SO_REUSEADDR) since the finding only identifies the duplication as a bug and doesn't specify with certainty what the second call should have been; the minimal safe fix is to eliminate the redundant syscall rather than guess at unverified intent.

🤖 Prompt for AI agents 
In microstack/ILibMulticastSocket.c around line 175, review and complete this code-review fix: Duplicate IP_MULTICAST_LOOP setsockopt call, missing IP_MULTICAST_TTL-equivalent redundancy is a copy/paste bug.
What the draft fix changed: In `ILibMulticastSocket_ResetMulticast`, removed the duplicate `setsockopt(socket, IPPROTO_IP, IP_MULTICAST_LOOP, ...)` line, leaving a single IP_MULTICAST_TTL call followed by a single IP_MULTICAST_LOOP call per interface socket. No new option was invented (e.g., IP_MULTICAST_IF/SO_REUSEADDR) since the finding only identifies the duplication as a bug and doesn't specify with certainty what the second call should have been; the minimal safe fix is to eliminate the redundant syscall rather than guess at unverified intent.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -214,7 +213,7 @@ void ILibMulticastSocket_BroadcastUdpPacketV4(struct ILibMulticastSocket_StateMo
#ifndef NACL
if (module->UDPServers[i] != NULL)
{
socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer);
socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]);
setsockopt(socket, IPPROTO_IP, IP_MULTICAST_IF, (const char*)&(module->AddressListV4[i].sin_addr), sizeof(struct in_addr));
setsockopt(socket, IPPROTO_IP, IP_MULTICAST_TTL, (const char*)&(module->TTL), sizeof(int));
for (j = 0; j < count; j++) sendto(socket, data, datalen, 0, (struct sockaddr*)addr, sizeof(struct sockaddr_in));
Comment on lines 213 to 219

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Broadcast helper uses shared module->UDPServer socket instead of the per-interface socket it just checked

In ILibMulticastSocket_BroadcastUdpPacketV4, changed socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer); to socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]); so IP_MULTICAST_IF/IP_MULTICAST_TTL are set and the packet is sent on the correct per-interface socket, matching the suggested fix exactly.

🤖 Prompt for AI agents 
In microstack/ILibMulticastSocket.c around line 221, review and complete this code-review fix: Broadcast helper uses shared module->UDPServer socket instead of the per-interface socket it just checked.
What the draft fix changed: In `ILibMulticastSocket_BroadcastUdpPacketV4`, changed `socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer);` to `socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]);` so IP_MULTICAST_IF/IP_MULTICAST_TTL are set and the packet is sent on the correct per-interface socket, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -397,3 +396,4 @@ void ILibMulticastSocket_WakeOnLan(void *module, char* mac)
ILibMulticastSocket_Broadcast((struct ILibMulticastSocket_StateModule*)module, ILibScratchPad, 102, 1);
}
}

Loading