Improvements #25

New Issue

jared · 2026-02-05T12:33:02-05:00

jared commented

2026-02-05 12:33:02 -05:00

Missing Cleanup Trap

The script creates temporary directories but doesn't handle cleanup on interruption:

Around line 796

SMART_CACHE_DIR="$(mktemp -d)"

Fix: Add a trap at the beginning:

cleanup() {
if -n "${SMART_CACHE_DIR:-}" && -d "$SMART_CACHE_DIR" ; then
rm -rf "$SMART_CACHE_DIR"
fi
}
trap cleanup EXIT INT TERM

No Bash Version Check

The script uses bash 4.2+ features (declare -g -A) without checking the version. Add after the shebang:

if ((BASH_VERSINFO[0] < 4 || (BASH_VERSINFO[0] == 4 && BASH_VERSINFO[1] < 2))); then
echo "ERROR: This script requires Bash 4.2 or higher (current: $BASH_VERSION)" >&2
exit 1
fi

Unsafe Background Job Handling

Lines 796-804 spawn background jobs without proper error handling:

for bay in $all_bays; do
device="${DRIVE_MAP[$bay]}"
if ; then
(get_drive_smart_info "$device" > "$SMART_CACHE_DIR/$device") &
fi
done
wait

If interrupted, background jobs continue running. The trap I suggested above helps, but consider adding job limit:

MAX_PARALLEL_JOBS=10
job_count=0
for bay in $all_bays; do
# ... existing checks ...
(get_drive_smart_info "$device" > "$SMART_CACHE_DIR/$device") &
((job_count++))
if ((job_count >= MAX_PARALLEL_JOBS)); then
wait -n # Wait for any job to complete
((job_count--))
fi
done
wait # Wait for remaining jobs

Functional Issues

Potential Command Injection

Line 657: The hostname is used without sanitization:

HOSTNAME=$(hostname)

While unlikely, if hostname contains special characters, it could cause issues in array lookups. Consider:

HOSTNAME=$(hostname | tr -cd '[:alnum:]-_.')

Error Stderr Handling

Line 615: Temporary file for stderr could leak:

smart_stderr="$(mktemp)"

... use it ...

rm -f "$smart_stderr"

If the function exits early (error), the temp file isn't cleaned. Use trap or ensure cleanup in all exit paths.

Numeric Comparison Without Validation

Lines 674-678 assume temperature is numeric:

if "$temp" -ge "$SMART_TEMP_CRIT" ; then

This will fail if $temp is non-numeric. The code does check with regex later, but the ordering should be reversed.

lsblk Cache Race Condition

Lines 755-767: The lsblk cache is built once, but if devices change during script execution, the cache becomes stale. Consider adding a timestamp check or refresh logic for long-running executions.

Documentation Issues

README Contains Same HTTP URLs

The README shows:

bash <(curl -s http://10.10.10.63:3000/...)

Minor Improvements

Inconsistent Quoting

Some variables are quoted, others aren't. Be consistent:

if -n "$DEBUG" ; then # Good
for bay in $all_bays; do # Should be "$all_bays" if it could have spaces

Printf Format String Vulnerability

Lines 868-875 use printf with -b flag and user-controlled content. While unlikely to be exploited here, be aware that colored output could theoretically contain format string characters.

Recommendations Summary

Critical:

✅ Add cleanup trap for temporary directories

✅ Add bash version check

2. Missing Cleanup Trap The script creates temporary directories but doesn't handle cleanup on interruption: # Around line 796 SMART_CACHE_DIR="$(mktemp -d)" Fix: Add a trap at the beginning: cleanup() { if [[ -n "${SMART_CACHE_DIR:-}" && -d "$SMART_CACHE_DIR" ]]; then rm -rf "$SMART_CACHE_DIR" fi } trap cleanup EXIT INT TERM 3. No Bash Version Check The script uses bash 4.2+ features (declare -g -A) without checking the version. Add after the shebang: if ((BASH_VERSINFO[0] < 4 || (BASH_VERSINFO[0] == 4 && BASH_VERSINFO[1] < 2))); then echo "ERROR: This script requires Bash 4.2 or higher (current: $BASH_VERSION)" >&2 exit 1 fi 4. Unsafe Background Job Handling Lines 796-804 spawn background jobs without proper error handling: for bay in $all_bays; do device="${DRIVE_MAP[$bay]}" if [[ -n "$device" && "$device" != "EMPTY" && -b "/dev/$device" ]]; then (get_drive_smart_info "$device" > "$SMART_CACHE_DIR/$device") & fi done wait If interrupted, background jobs continue running. The trap I suggested above helps, but consider adding job limit: MAX_PARALLEL_JOBS=10 job_count=0 for bay in $all_bays; do # ... existing checks ... (get_drive_smart_info "$device" > "$SMART_CACHE_DIR/$device") & ((job_count++)) if ((job_count >= MAX_PARALLEL_JOBS)); then wait -n # Wait for any job to complete ((job_count--)) fi done wait # Wait for remaining jobs Functional Issues 5. Potential Command Injection Line 657: The hostname is used without sanitization: HOSTNAME=$(hostname) While unlikely, if hostname contains special characters, it could cause issues in array lookups. Consider: HOSTNAME=$(hostname | tr -cd '[:alnum:]-_.') 6. Error Stderr Handling Line 615: Temporary file for stderr could leak: smart_stderr="$(mktemp)" # ... use it ... rm -f "$smart_stderr" If the function exits early (error), the temp file isn't cleaned. Use trap or ensure cleanup in all exit paths. 7. Numeric Comparison Without Validation Lines 674-678 assume temperature is numeric: if [[ "$temp" -ge "$SMART_TEMP_CRIT" ]]; then This will fail if $temp is non-numeric. The code does check with regex later, but the ordering should be reversed. 8. lsblk Cache Race Condition Lines 755-767: The lsblk cache is built once, but if devices change during script execution, the cache becomes stale. Consider adding a timestamp check or refresh logic for long-running executions. Documentation Issues 9. README Contains Same HTTP URLs The README shows: bash <(curl -s http://10.10.10.63:3000/...) Minor Improvements 10. Inconsistent Quoting Some variables are quoted, others aren't. Be consistent: if [[ -n "$DEBUG" ]]; then # Good for bay in $all_bays; do # Should be "$all_bays" if it could have spaces 11. Printf Format String Vulnerability Lines 868-875 use printf with -b flag and user-controlled content. While unlikely to be exploited here, be aware that colored output could theoretically contain format string characters. Recommendations Summary Critical: ✅ Add cleanup trap for temporary directories ✅ Add bash version check

jared referenced this issue from a commit

2026-02-06 18:10:00 -05:00

Add robustness improvements: bash version check, cleanup trap, hostname sanitization, parallel job limit

jared closed this issue

2026-02-06 18:10:01 -05:00

jared referenced this issue from a commit

2026-02-06 18:15:48 -05:00

Fix local keyword used outside function, document storage-01 HBA layout

jared referenced this issue from a commit

2026-02-06 18:20:47 -05:00

Add storage-01 HBA bay 5 mapping (phy9)

jared referenced this issue from a commit