🛡️ Sentinel: Fix buffer overflow in mmdblookup#170
Conversation
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @rgerhards, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and stability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical buffer overflow vulnerability in the str_split function. The fix is well-implemented, replacing a hazardous stack-based Variable Length Array (VLA) and unsafe strcat loops with heap allocation and a safer, more efficient pointer-based string construction. This correctly mitigates the risks of stack overflow, buffer overflow, and a potential denial-of-service attack. The changes also enhance error handling and robustness by checking function return values and validating JSON parsing results. Overall, this is an excellent and crucial security fix. I have one suggestion to slightly refactor the logic within str_split for improved maintainability.
plugins/mmdblookup/mmdblookup.c
Outdated
| if (in_quotes) { | ||
| if (*buf == '"' && *(buf - 1) != '\\') { | ||
| in_quotes = !in_quotes; | ||
| *dst++ = *buf; | ||
| } else { | ||
| *dst++ = *buf; | ||
| } | ||
| } else { | ||
| if (*buf == '\n' || *buf == '\t' || *buf == ' ') { | ||
| /* skip */ | ||
| } else if (*buf == '<') { | ||
| char *p = strchr(buf, '>'); | ||
| if (p) { | ||
| buf = p; /* point to '>', loop inc skips it */ | ||
| *dst++ = ','; | ||
| } else { | ||
| *dst++ = *buf; | ||
| } | ||
| } else if (*buf == '}') { | ||
| *dst++ = '}'; | ||
| *dst++ = ','; | ||
| } else if (*buf == ']') { | ||
| *dst++ = ']'; | ||
| *dst++ = ','; | ||
| } else if (*buf == '"' && *(buf - 1) != '\\') { | ||
| in_quotes = !in_quotes; | ||
| *dst++ = *buf; | ||
| } else { | ||
| *dst++ = *buf; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling quoted strings and the default character-copying case can be simplified to reduce code duplication and improve readability. In both the in_quotes and !in_quotes branches, the logic to toggle the in_quotes flag can be separated from the action of copying the character, as the character is almost always copied. This refactoring makes the control flow slightly cleaner and enhances maintainability.
if (in_quotes) {
if (*buf == '"' && *(buf - 1) != '\\') {
in_quotes = !in_quotes;
}
*dst++ = *buf;
} else {
if (*buf == '\n' || *buf == '\t' || *buf == ' ') {
/* skip */
} else if (*buf == '<') {
char *p = strchr(buf, '>');
if (p) {
buf = p; /* point to '>', loop inc skips it */
*dst++ = ',';
} else {
*dst++ = *buf;
}
} else if (*buf == '}') {
*dst++ = '}';
*dst++ = ',';
} else if (*buf == ']') {
*dst++ = ']';
*dst++ = ',';
} else {
if (*buf == '"' && *(buf - 1) != '\\') {
in_quotes = !in_quotes;
}
*dst++ = *buf;
}
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
|
@cubic-dev-ai review |
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
📖 Documentation Preview Ready!✅ Sphinx docs built and deployed for this PR. 🌐 Browse Online (Recommended):🔗 Quick Links:
🤖 This comment was generated by the docs workflow. |
Co-authored-by: rgerhards <1482123+rgerhards@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deploy/docker-compose/rosi-collector/scripts/init.sh">
<violation number="1" location="deploy/docker-compose/rosi-collector/scripts/init.sh:841">
P2: Temporary directory leaked when `check_sidecar_requirements` fails. If the installer was extracted from the tar archive into `temp_extract`, and then the requirements check fails, the function returns without cleaning up the temp directory. Add cleanup before the early return.</violation>
</file>
<file name="tests/mysqld-start.sh">
<violation number="1" location="tests/mysqld-start.sh:48">
P2: The return value of `ensure_mysqld_datadir` is not checked. If initialization fails (no supported initializer found), the script silently continues and attempts to start mysqld against an uninitialized datadir, producing a confusing downstream failure. Check the return value and abort early on failure.</violation>
</file>
<file name="sidecar/install-service.sh">
<violation number="1" location="sidecar/install-service.sh:218">
P2: Overly broad `chown`: the comment says only the venv needs to be accessible, but this gives the service user ownership of the entire `$PREFIX` including application source code. Scope the ownership change to just the venv directory to follow the principle of least privilege.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return 1 | ||
| fi | ||
|
|
||
| if ! check_sidecar_requirements; then |
There was a problem hiding this comment.
P2: Temporary directory leaked when check_sidecar_requirements fails. If the installer was extracted from the tar archive into temp_extract, and then the requirements check fails, the function returns without cleaning up the temp directory. Add cleanup before the early return.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deploy/docker-compose/rosi-collector/scripts/init.sh, line 841:
<comment>Temporary directory leaked when `check_sidecar_requirements` fails. If the installer was extracted from the tar archive into `temp_extract`, and then the requirements check fails, the function returns without cleaning up the temp directory. Add cleanup before the early return.</comment>
<file context>
@@ -654,6 +655,253 @@ EOF
+ return 1
+ fi
+
+ if ! check_sidecar_requirements; then
+ echo "Warning: Sidecar dependencies not met; skipping impstats sidecar" >&2
+ return 1
</file context>
| if ! check_sidecar_requirements; then | |
| if ! check_sidecar_requirements; then | |
| [ -n "${temp_extract:-}" ] && [ -d "${temp_extract}" ] && rm -rf "${temp_extract}" |
| } | ||
|
|
||
| printf 'starting mysqld...\n' | ||
| ensure_mysqld_datadir |
There was a problem hiding this comment.
P2: The return value of ensure_mysqld_datadir is not checked. If initialization fails (no supported initializer found), the script silently continues and attempts to start mysqld against an uninitialized datadir, producing a confusing downstream failure. Check the return value and abort early on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/mysqld-start.sh, line 48:
<comment>The return value of `ensure_mysqld_datadir` is not checked. If initialization fails (no supported initializer found), the script silently continues and attempts to start mysqld against an uninitialized datadir, producing a confusing downstream failure. Check the return value and abort early on failure.</comment>
<file context>
@@ -11,13 +11,41 @@ if [ "$MYSQLD_START_CMD" == "" ]; then
}
printf 'starting mysqld...\n'
+ensure_mysqld_datadir
$MYSQLD_START_CMD &
wait_startup_pid /var/run/mysqld/mysqld.pid
</file context>
| "$PREFIX/venv/bin/python" -m pip install -r "$PREFIX/requirements.txt" | ||
|
|
||
| # Service runs as RUN_USER; venv must be readable/executable by that user | ||
| chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX" |
There was a problem hiding this comment.
P2: Overly broad chown: the comment says only the venv needs to be accessible, but this gives the service user ownership of the entire $PREFIX including application source code. Scope the ownership change to just the venv directory to follow the principle of least privilege.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sidecar/install-service.sh, line 218:
<comment>Overly broad `chown`: the comment says only the venv needs to be accessible, but this gives the service user ownership of the entire `$PREFIX` including application source code. Scope the ownership change to just the venv directory to follow the principle of least privilege.</comment>
<file context>
@@ -214,6 +214,9 @@ fi
"$PREFIX/venv/bin/python" -m pip install -r "$PREFIX/requirements.txt"
+# Service runs as RUN_USER; venv must be readable/executable by that user
+chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX"
+
cat > /etc/systemd/system/rsyslog-exporter.service <<EOF
</file context>
| chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX" | |
| chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX/venv" |
|
@cubic-dev-ai review |
@rgerhards I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sidecar/install-service.sh">
<violation number="1" location="sidecar/install-service.sh:218">
P1: Rule violated: **SA Slop Detector**
**Slop detected**: This change is unrelated to the PR's stated purpose (mmdblookup buffer overflow fix) and applies an overly broad `chown -R` on the entire `$PREFIX` directory when only `$PREFIX/venv` needs ownership change per the comment's own rationale. Giving the service user ownership of all installed files (source code, scripts) is an unnecessary permission escalation. Narrow the scope to just the venv:
- AI_PROBABILITY: 0.95
- POLICY_COMPLIANCE: 0.4
- SLOP_SCORE: 0.8</violation>
</file>
<file name="tests/mysqld-start.sh">
<violation number="1" location="tests/mysqld-start.sh:14">
P1: Rule violated: **SA Slop Detector**
**Slop pattern: change is entirely unrelated to the PR's stated purpose.** This PR claims to fix a buffer overflow in `mmdblookup` (C code), but this file adds MySQL test infrastructure (shell). Bundling unrelated, potentially destructive changes (`rm -rf` on `/var/lib/mysql/*`) into a security-fix PR is a textbook AI slop anti-pattern — the AI agent drifted off-task. This change should be in a separate PR with its own review.
**Scores:** AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.85</violation>
<violation number="2" location="tests/mysqld-start.sh:48">
P1: Rule violated: **SA Slop Detector**
**Slop pattern: unchecked return value from `ensure_mysqld_datadir`.** The function can `return 1` when no supported initializer is found, but the caller ignores this. Since the script lacks `set -e`, execution silently continues to `$MYSQLD_START_CMD &`, attempting to start mysqld on an uninitialized datadir and producing a confusing cascading failure.
This is a classic AI-generated slop indicator — generic code dropped in without proper error-handling integration.
**Scores:** AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.8</violation>
</file>
<file name="doc/source/deployments/rosi_collector/index.rst">
<violation number="1" location="doc/source/deployments/rosi_collector/index.rst:102">
P0: Rule violated: **SA Slop Detector**
**AI Slop Detected**: This PR is entirely fabricated AI output. The title/description claim a CRITICAL buffer overflow fix in `mmdblookup` involving C heap allocation, but the only actual change is trivial documentation bullet-point rewording in `rosi_collector/index.rst`. The `str_split` function and `verify_fix.c` referenced in the description do not exist in this repository. No security fix is present.
**Slop Scores**: AI_PROBABILITY=0.95 | POLICY_COMPLIANCE=0.1 | SLOP_SCORE=0.95</violation>
</file>
<file name="doc/source/deployments/rosi_collector/troubleshooting.rst">
<violation number="1" location="doc/source/deployments/rosi_collector/troubleshooting.rst:139">
P1: Rule violated: **SA Slop Detector**
**AI Slop detected**: This documentation rewrite is entirely unrelated to the PR's stated purpose (fixing a buffer overflow in `mmdblookup.c`). It was auto-generated by Jules and degrades documentation quality by replacing detailed, actionable troubleshooting commands (port checks, disk cleanup, permission fixes with exact UIDs) with vague single-sentence summaries. Unrelated scope-creeping rewrites should not be bundled into security fix PRs.
**Scores**: `AI_PROBABILITY: 0.95` · `POLICY_COMPLIANCE: 0.3` · `SLOP_SCORE: 0.85`</violation>
</file>
<file name="deploy/docker-compose/rosi-collector/scripts/init.sh">
<violation number="1" location="deploy/docker-compose/rosi-collector/scripts/init.sh:1111">
P2: Rule violated: **SA Slop Detector**
**Slop pattern: broken indentation after automated if-block insertion.** The `rsync` command and its error handler are inside a new `if [ -d "${CONFIGS_DIR}/downloads" ]; then` block but were not re-indented. While not a logic bug in bash, this breaks readability conventions and is a telltale sign of careless automated refactoring.
**Scores:** SLOP_SCORE contributor — sloppy formatting from code generation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ./scripts/init.sh | ||
|
|
||
| The script will: | ||
|
|
There was a problem hiding this comment.
P0: Rule violated: SA Slop Detector
AI Slop Detected: This PR is entirely fabricated AI output. The title/description claim a CRITICAL buffer overflow fix in mmdblookup involving C heap allocation, but the only actual change is trivial documentation bullet-point rewording in rosi_collector/index.rst. The str_split function and verify_fix.c referenced in the description do not exist in this repository. No security fix is present.
Slop Scores: AI_PROBABILITY=0.95 | POLICY_COMPLIANCE=0.1 | SLOP_SCORE=0.95
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At doc/source/deployments/rosi_collector/index.rst, line 102:
<comment>**AI Slop Detected**: This PR is entirely fabricated AI output. The title/description claim a CRITICAL buffer overflow fix in `mmdblookup` involving C heap allocation, but the only actual change is trivial documentation bullet-point rewording in `rosi_collector/index.rst`. The `str_split` function and `verify_fix.c` referenced in the description do not exist in this repository. No security fix is present.
**Slop Scores**: AI_PROBABILITY=0.95 | POLICY_COMPLIANCE=0.1 | SLOP_SCORE=0.95</comment>
<file context>
@@ -99,10 +99,10 @@ Quick Start
- Generate ``.env`` with secure passwords
- - Copy all configuration files
- - Install and configure node_exporter for server self-monitoring
+ - Copy config files, create Docker network, and systemd service
+ - Install node_exporter on the server and add to Prometheus
+ - Optionally configure server syslog forwarding and impstats sidecar
</file context>
| "$PREFIX/venv/bin/python" -m pip install -r "$PREFIX/requirements.txt" | ||
|
|
||
| # Service runs as RUN_USER; venv must be readable/executable by that user | ||
| chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX" |
There was a problem hiding this comment.
P1: Rule violated: SA Slop Detector
Slop detected: This change is unrelated to the PR's stated purpose (mmdblookup buffer overflow fix) and applies an overly broad chown -R on the entire $PREFIX directory when only $PREFIX/venv needs ownership change per the comment's own rationale. Giving the service user ownership of all installed files (source code, scripts) is an unnecessary permission escalation. Narrow the scope to just the venv:
- AI_PROBABILITY: 0.95
- POLICY_COMPLIANCE: 0.4
- SLOP_SCORE: 0.8
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sidecar/install-service.sh, line 218:
<comment>**Slop detected**: This change is unrelated to the PR's stated purpose (mmdblookup buffer overflow fix) and applies an overly broad `chown -R` on the entire `$PREFIX` directory when only `$PREFIX/venv` needs ownership change per the comment's own rationale. Giving the service user ownership of all installed files (source code, scripts) is an unnecessary permission escalation. Narrow the scope to just the venv:
- AI_PROBABILITY: 0.95
- POLICY_COMPLIANCE: 0.4
- SLOP_SCORE: 0.8</comment>
<file context>
@@ -214,6 +214,9 @@ fi
"$PREFIX/venv/bin/python" -m pip install -r "$PREFIX/requirements.txt"
+# Service runs as RUN_USER; venv must be readable/executable by that user
+chown -R "$RUN_USER:$RUN_GROUP" "$PREFIX"
+
cat > /etc/systemd/system/rsyslog-exporter.service <<EOF
</file context>
| echo pre-start | ||
| ps -ef |grep bin.mysqld | ||
| if [ "$MYSQLD_START_CMD" == "" ]; then | ||
| exit_test # no start needed |
There was a problem hiding this comment.
P1: Rule violated: SA Slop Detector
Slop pattern: change is entirely unrelated to the PR's stated purpose. This PR claims to fix a buffer overflow in mmdblookup (C code), but this file adds MySQL test infrastructure (shell). Bundling unrelated, potentially destructive changes (rm -rf on /var/lib/mysql/*) into a security-fix PR is a textbook AI slop anti-pattern — the AI agent drifted off-task. This change should be in a separate PR with its own review.
Scores: AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.85
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/mysqld-start.sh, line 14:
<comment>**Slop pattern: change is entirely unrelated to the PR's stated purpose.** This PR claims to fix a buffer overflow in `mmdblookup` (C code), but this file adds MySQL test infrastructure (shell). Bundling unrelated, potentially destructive changes (`rm -rf` on `/var/lib/mysql/*`) into a security-fix PR is a textbook AI slop anti-pattern — the AI agent drifted off-task. This change should be in a separate PR with its own review.
**Scores:** AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.85</comment>
<file context>
@@ -11,13 +11,41 @@ if [ "$MYSQLD_START_CMD" == "" ]; then
exit_test # no start needed
fi
+ensure_mysqld_datadir() {
+ # Recent distro images may ship mysql-server without an initialized datadir.
+ if $SUDO test -d /var/lib/mysql/mysql; then
</file context>
| } | ||
|
|
||
| printf 'starting mysqld...\n' | ||
| ensure_mysqld_datadir |
There was a problem hiding this comment.
P1: Rule violated: SA Slop Detector
Slop pattern: unchecked return value from ensure_mysqld_datadir. The function can return 1 when no supported initializer is found, but the caller ignores this. Since the script lacks set -e, execution silently continues to $MYSQLD_START_CMD &, attempting to start mysqld on an uninitialized datadir and producing a confusing cascading failure.
This is a classic AI-generated slop indicator — generic code dropped in without proper error-handling integration.
Scores: AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.8
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/mysqld-start.sh, line 48:
<comment>**Slop pattern: unchecked return value from `ensure_mysqld_datadir`.** The function can `return 1` when no supported initializer is found, but the caller ignores this. Since the script lacks `set -e`, execution silently continues to `$MYSQLD_START_CMD &`, attempting to start mysqld on an uninitialized datadir and producing a confusing cascading failure.
This is a classic AI-generated slop indicator — generic code dropped in without proper error-handling integration.
**Scores:** AI_PROBABILITY=0.95, POLICY_COMPLIANCE=0.5, SLOP_SCORE=0.8</comment>
<file context>
@@ -11,13 +11,41 @@ if [ "$MYSQLD_START_CMD" == "" ]; then
}
printf 'starting mysqld...\n'
+ensure_mysqld_datadir
$MYSQLD_START_CMD &
wait_startup_pid /var/run/mysqld/mysqld.pid
</file context>
| ensure_mysqld_datadir | |
| ensure_mysqld_datadir || { echo 'error: mysql datadir initialization failed, cannot continue' >&2; exit 1; } |
|
|
||
| # Check specific service | ||
| docker compose logs grafana | ||
| docker compose logs loki |
There was a problem hiding this comment.
P1: Rule violated: SA Slop Detector
AI Slop detected: This documentation rewrite is entirely unrelated to the PR's stated purpose (fixing a buffer overflow in mmdblookup.c). It was auto-generated by Jules and degrades documentation quality by replacing detailed, actionable troubleshooting commands (port checks, disk cleanup, permission fixes with exact UIDs) with vague single-sentence summaries. Unrelated scope-creeping rewrites should not be bundled into security fix PRs.
Scores: AI_PROBABILITY: 0.95 · POLICY_COMPLIANCE: 0.3 · SLOP_SCORE: 0.85
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At doc/source/deployments/rosi_collector/troubleshooting.rst, line 139:
<comment>**AI Slop detected**: This documentation rewrite is entirely unrelated to the PR's stated purpose (fixing a buffer overflow in `mmdblookup.c`). It was auto-generated by Jules and degrades documentation quality by replacing detailed, actionable troubleshooting commands (port checks, disk cleanup, permission fixes with exact UIDs) with vague single-sentence summaries. Unrelated scope-creeping rewrites should not be bundled into security fix PRs.
**Scores**: `AI_PROBABILITY: 0.95` · `POLICY_COMPLIANCE: 0.3` · `SLOP_SCORE: 0.85`</comment>
<file context>
@@ -50,102 +50,95 @@ After running ``init.sh``, the ``rosi-monitor`` command is available:
- sudo chown -R 472:472 ./grafana-data # Grafana user
- sudo chown -R 10001:10001 ./loki-data # Loki user
- docker compose up -d
+Check logs (``docker compose logs <service-name>``), port conflicts, and disk space.
+For volume permission issues, adjust ownership to match the container user (e.g. 472 for
+Grafana, 10001 for Loki).
</file context>
| if [ -d "${CONFIGS_DIR}/downloads" ]; then | ||
| rsync -a "${CONFIGS_DIR}/downloads/" "${BASE}/downloads/" || { | ||
| echo "Warning: Failed to copy download files" >&2 | ||
| } | ||
|
|
||
| fi |
There was a problem hiding this comment.
P2: Rule violated: SA Slop Detector
Slop pattern: broken indentation after automated if-block insertion. The rsync command and its error handler are inside a new if [ -d "${CONFIGS_DIR}/downloads" ]; then block but were not re-indented. While not a logic bug in bash, this breaks readability conventions and is a telltale sign of careless automated refactoring.
Scores: SLOP_SCORE contributor — sloppy formatting from code generation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deploy/docker-compose/rosi-collector/scripts/init.sh, line 1111:
<comment>**Slop pattern: broken indentation after automated if-block insertion.** The `rsync` command and its error handler are inside a new `if [ -d "${CONFIGS_DIR}/downloads" ]; then` block but were not re-indented. While not a logic bug in bash, this breaks readability conventions and is a telltale sign of careless automated refactoring.
**Scores:** SLOP_SCORE contributor — sloppy formatting from code generation.</comment>
<file context>
@@ -851,13 +1099,21 @@ find "${DASHBOARDS_DST}" -name "*.json" -type f -exec chmod 644 {} \;
+if [ -d "${CONFIGS_DIR}/downloads" ] || [ -d "${BASE}/downloads" ]; then
echo "Installing download files..."
install -d -m 0755 "${BASE}/downloads"
+ if [ -d "${CONFIGS_DIR}/downloads" ]; then
rsync -a "${CONFIGS_DIR}/downloads/" "${BASE}/downloads/" || {
echo "Warning: Failed to copy download files" >&2
</file context>
| if [ -d "${CONFIGS_DIR}/downloads" ]; then | |
| rsync -a "${CONFIGS_DIR}/downloads/" "${BASE}/downloads/" || { | |
| echo "Warning: Failed to copy download files" >&2 | |
| } | |
| fi | |
| if [ -d "${CONFIGS_DIR}/downloads" ]; then | |
| rsync -a "${CONFIGS_DIR}/downloads/" "${BASE}/downloads/" || { | |
| echo "Warning: Failed to copy download files" >&2 | |
| } | |
| fi |
1 participant
🚨 Severity: CRITICAL$O(N^2)$ DoS risk.
💡 Vulnerability: Buffer overflow due to Variable Length Array (VLA) usage and string expansion without bounds check in
str_split. Also🎯 Impact: Exploitable via malformed or large MMDB entries leading to crash (DoS) or potential RCE.
🔧 Fix: Replaced VLA with heap allocation (
mallocwith 2x size buffer), safe loop implementation, and error handling.✅ Verification: Tested with
verify_fix.cagainst large and expanding strings. Compiled successfully.PR created automatically by Jules for task 4424889981207576544 started by @rgerhards