fix: Critical memory leaks in sys.unixshellcommand (2/3/4-param modes)

[jsavin]

Problem

Critical memory leaks identified in PR #192 code review across all multi-parameter modes:

2-Parameter Leak (Line 672-675)

If langsetvalue() fails in the 2-parameter case, hstdout is not disposed.

3-Parameter Leak (Line 706-710)

When the FIRST langsetvalue() fails in the 3-parameter case, hstdout is not disposed. The code correctly disposes hstderr, but forgets hstdout.

4-Parameter Leak (Line 757-760)

When the FIRST langsetvalue() fails in the 4-parameter case, hstdout is not disposed. The code correctly disposes hstderr, but forgets hstdout.

Root Cause

Per handle ownership semantics (documented at lines 754-756):

• langsetvalue() only adopts handles on SUCCESS
• If it FAILS, the caller must manually dispose the handle

Why 3-param and 4-param are different from 1-param

• 1-parameter case: No handles involved (returns string directly)
• 2-parameter case: ONE handle at risk (just hstdout)
• 3-parameter case: TWO handles (both hstdout and hstderr) need disposal on first failure
• 4-parameter case: TWO handles (both hstdout and hstderr) need disposal on first failure

Fixes Applied

Fix 1: 2-Parameter Case (shellsysverbs.c:672-675)

Before:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype))
    return (false);  // BUG: hstdout leaks!

After:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstdout);
    return (false);
}

Fix 2: 3-Parameter Case (shellsysverbs.c:706-710)

Before:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstderr);  // Disposes hstderr
    return (false);            // BUG: hstdout leaks!
}

After:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstdout);   // Dispose unadopted hstdout
    disposehandle (hstderr);   // Dispose unadopted hstderr
    return (false);
}

Fix 3: 4-Parameter Case (shellsysverbs.c:757-760)

Before:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstderr);  // Only disposes hstderr
    return (false);            // BUG: hstdout leaks!
}

After:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstdout);   // Dispose unadopted hstdout
    disposehandle (hstderr);   // Dispose unadopted hstderr
    return (false);
}

Impact

• Leak size: One or two handles per failure (typically small)
• Frequency: Only when langsetvalue() fails (low probability - hash table insertion failure)
• Severity: Critical (memory leaks)
• Scope: Affects all multi-parameter modes (2/3/4-param)

Testing

• ✅ Reviewed all parameter cases (1/2/3/4) for consistency
• ✅ Verified error handling matches documented ownership semantics
• ✅ All multi-parameter modes now have correct error handling
• ✅ No remaining leaks in handle ownership paths

Follow-Up

Bot review suggests adding error path test coverage (testing langsetvalue() failures). This should be tracked in a separate issue as it requires test infrastructure work.

Related

• PR feat: Enhanced sys.unixshellcommand and sys.winshellcommand with optional stderr capture #192 - Original implementation
• Bot code review comments identifying all three leaks

---

🤖 Generated with Claude Code

[claude]

Code Review - PR #195

Summary

✅ APPROVE with critical follow-up required

This PR correctly fixes the memory leak in the 2-parameter case (line 672-675), but code review reveals an identical bug in the 3-parameter case that must also be fixed.

---

What This PR Gets Right ✅

1. Correct fix for 2-param case: The added disposehandle(hstdout) at line 673 properly handles the failure path when langsetvalue() fails to adopt the handle.

2. Follows ownership semantics: The fix correctly implements the documented pattern (lines 752-755) where callers must dispose handles that langsetvalue() fails to adopt.

3. Consistent with 4-param case: Matches the error handling pattern in the 4-parameter implementation (line 756-758).

4. Good documentation: PR description clearly explains the ownership semantics and root cause.

---

Critical Issue Found: 3-Parameter Case Has Same Bug ❌

Location: Common/source/shellsysverbs.c:706-709

The bug:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstderr);  // ✅ Correctly disposes hstderr
    return (false);            // ❌ BUG: hstdout leaks!
}

Why this is wrong:

• When the first langsetvalue() fails, hstdout was never adopted
• The code correctly disposes hstderr, but forgets to dispose hstdout
• This is the exact same leak as the 2-parameter case

Correct fix:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstdout);   // Must dispose unadopted handle
    disposehandle (hstderr);
    return (false);
}

Why the 4-param case gets this right:
Lines 756-758 only dispose hstderr because there's only ONE handle at risk when the first langsetvalue() fails. The 3-param case has TWO handles (hstdout and hstderr), so both must be disposed on the first failure.

---

Recommendations

Option 1: Expand this PR (recommended)

• Add the 3-parameter fix to this same PR
• Update PR title to "fix: Memory leaks in sys.unixshellcommand 2-param and 3-param modes"
• Add test coverage if possible

Option 2: File follow-up issue

• Merge this PR as-is (fixes half the problem)
• Create Issue #XXX for 3-param leak
• Risk: Easy to forget the follow-up

I recommend Option 1 - fixing both leaks in a single PR ensures consistency and completeness.

---

Testing Suggestions

Consider adding a test that:

1. Calls sys.unixshellcommand() with invalid variable addresses (to trigger langsetvalue() failure)
2. Monitors memory usage before/after
3. Verifies no leaks in 2-param, 3-param, and 4-param cases

This would catch both the current fix and the 3-param bug.

---

Minor Notes

• ✅ Code style matches existing conventions (space after disposehandle, consistent bracing)
• ✅ Fix is minimal and surgical (low regression risk)
• ✅ PR description references ownership semantics documentation

---

Verdict

Approve the approach, but please fix the 3-parameter case before merging. This is the same class of bug and should be addressed together.

Great catch on the 2-param leak! The ownership semantics analysis is solid - just needs to be applied consistently across all parameter cases.

---

🤖 Code review by Claude Code (Sonnet 4.5)

[claude]

Code Review: PR #195

Summary

This PR correctly fixes critical memory leaks in the 2-parameter and 3-parameter modes of sys.unixshellcommand. The fixes follow the documented ownership semantics and are consistent with the existing error handling patterns.

---

✅ Positive Findings

1. Correct Handle Ownership Pattern

• Both fixes correctly dispose handles when langsetvalue() fails
• Follows documented semantics at lines 753-756: "langsetvalue() adopts the handle on SUCCESS... If langsetvalue() FAILS, the handle is NOT adopted and must be disposed manually"
• Disposal order in 3-param case is correct: dispose both hstdout and hstderr since neither was adopted

2. Consistent with 4-Parameter Case Pattern

• The 3-param fix at line 707-708 matches the pattern used in 4-param case line 762-765
• When first langsetvalue() fails: dispose all unadopted handles
• When second langsetvalue() fails: only dispose the second handle (first was already adopted)

3. Code Quality

• Clean, minimal changes
• No unnecessary refactoring
• Follows existing code style
• Good explanatory PR description with clear before/after examples

---

⚠️ Critical Issue Found: 4-Parameter Case Has Same Bug

Location: shellsysverbs.c:757-759 (4-parameter mode, first langsetvalue() error path)

Current code:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstderr);  // Only disposes hstderr
    return (false);            // BUG: hstdout leaks!
}

Should be:

if (!langsetvalue (htable, varname, hstdout, stringvaluetype)) {
    disposehandle (hstdout);   // Dispose unadopted hstdout
    disposehandle (hstderr);   // Dispose unadopted hstderr
    return (false);
}

Why this is a bug:

• When the first langsetvalue() fails, BOTH hstdout and hstderr are unadopted
• Current code only disposes hstderr, leaking hstdout
• This is the EXACT same bug pattern that PR fix: Critical memory leaks in sys.unixshellcommand (2/3/4-param modes) #195 fixes in the 2-param and 3-param cases
• The 4-param case was simply missed during the fix

Impact: Same severity as the bugs this PR fixes (memory leak on langsetvalue() failure)

---

📋 Recommendations

1. Fix the 4-Parameter Case (Required before merge)

• Add disposehandle(hstdout) at line 758 (before disposehandle(hstderr))
• This makes all parameter cases consistent

2. Add Error Path Test Coverage (Recommended follow-up)

• Current tests in test_sys_shell_command_verbs.c only test success paths
• Should add tests that verify proper cleanup when langsetvalue() fails
• Test strategy:
  • Mock langsetvalue() to return failure
  • Use memory leak detection tools (valgrind, AddressSanitizer)
  • Verify no handles leak on error paths

3. Document Handle Ownership Pattern (Low priority)

• The comment at lines 753-756 is excellent
• Consider adding similar comment above the 2-param and 3-param cases
• Or reference the 4-param comment: "See line 753 for handle ownership semantics"

---

🔍 Code Review Checklist

• ✅ Fixes stated bugs correctly
• ✅ Follows ownership semantics
• ✅ Consistent with existing patterns
• ✅ No performance regressions
• ✅ No security concerns
• ❌ Incomplete: 4-param case has same bug
• ❌ Missing: Error path test coverage

---

🎯 Action Required

Before merge:

1. Add disposehandle(hstdout) at line 758 in the 4-parameter case
2. Update PR description to mention fixing all three cases (2-param, 3-param, 4-param)
3. Consider running with AddressSanitizer/valgrind to verify no leaks remain

After merge (follow-up issue):

• Create GitHub issue for error path test coverage
• Add tests that verify handle cleanup on langsetvalue() failures

---

📖 References

• Handle ownership semantics: shellsysverbs.c:753-756
• PR feat: Enhanced sys.unixshellcommand and sys.winshellcommand with optional stderr capture #192: Original implementation
• CLAUDE.md: Memory leak patterns and testing standards

---

Overall Assessment: The fixes are correct and well-implemented, but the PR is incomplete because it misses the same bug in the 4-parameter case. With that additional fix, this PR should merge.

cc: @jsavin - Please add the 4-param fix before merging.

[claude]

Code Review: PR #195 - Critical Memory Leak Fixes ✅

Summary

APPROVED - This PR correctly fixes critical memory leaks in all multi-parameter modes of sys.unixshellcommand. The fixes are correct, complete, and follow proper handle ownership semantics.

---

Code Quality & Best Practices ✅

Excellent work on:

1. Correct handle ownership semantics - All fixes properly dispose handles when langsetvalue() fails
2. Complete coverage - All three leak sites (2-param, 3-param, 4-param) are fixed
3. Consistent error handling - The pattern matches across all parameter modes
4. Clear documentation - PR description thoroughly explains root cause and impact
5. Follows existing patterns - Matches the error handling at lines 700-703 and 747-750

Code correctness verified:

• Line 672-675 (2-param): ✅ Disposes hstdout on langsetvalue() failure
• Line 706-710 (3-param): ✅ Disposes BOTH hstdout and hstderr on first failure
• Line 757-760 (4-param): ✅ Disposes BOTH hstdout and hstderr on first failure

Handle ownership analysis:

• ✅ If langsetvalue() succeeds → handle is adopted by table (no disposal needed)
• ✅ If langsetvalue() fails → handle NOT adopted (caller must dispose)
• ✅ All error paths correctly dispose unadopted handles
• ✅ No double-free risks (handles only disposed when NOT adopted)

---

Potential Bugs or Issues ✅ NONE FOUND

Verified no issues with:

• ✅ No double-free risk (handles only disposed on langsetvalue() failure)
• ✅ No use-after-free risk (early returns prevent further handle use)
• ✅ Correct ordering (dispose in reverse allocation order where applicable)
• ✅ All code paths covered (no missing error branches)

Edge case analysis:

• ✅ 3-param SECOND failure (line 712-717): Correctly disposes ONLY hstderr because hstdout was already adopted
• ✅ 4-param SECOND failure (line 763-768): Same pattern - only disposes hstderr
• ✅ 4-param THIRD failure (line 772-776): No disposal needed - both handles already adopted

---

Performance Considerations ✅

• No performance impact - These are error paths that only execute on failure
• Minor improvement - Prevents memory leaks from accumulating on repeated failures
• Disposal overhead - Negligible (disposehandle() is fast on small handles)

---

Security Concerns ✅ NONE

• No security vulnerabilities introduced - This is purely cleanup code
• Improves security posture - Memory leaks can lead to DoS vulnerabilities in long-running processes
• No information leakage - Handles are properly disposed, preventing data exposure

---

Test Coverage ⚠️ Acknowledged Gap

Current state:

• ✅ Visual code inspection confirms all paths are correct
• ✅ Pattern matches other error handling in same file
• ❌ No automated test coverage for langsetvalue() failure paths

Recommendation:
This PR is ready to merge. Test coverage for error paths should be tracked separately as it requires test infrastructure work. The bot review comment suggesting error path testing is valid but should not block this fix.

Suggested follow-up issue:

"Add test coverage for sys.unixshellcommand error paths (langsetvalue failures)"

• Test 2-param mode with hash table insertion failure
• Test 3-param mode with first and second langsetvalue failures
• Test 4-param mode with all three failure points
• Verify no memory leaks using valgrind or similar

---

Alignment with CLAUDE.md Standards ✅

Verified compliance:

• ✅ No fprintf(stderr) violations (no logging changes in this PR)
• ✅ No magic numbers introduced
• ✅ Follows existing error handling patterns
• ✅ PR description references related PR feat: Enhanced sys.unixshellcommand and sys.winshellcommand with optional stderr capture #192
• ✅ Clear commit message explaining the fix

No architectural concerns:

• ✅ No database format changes
• ✅ No global state modifications
• ✅ No threading implications
• ✅ Pure bug fix - no new features

---

Final Verdict: ✅ APPROVED - READY TO MERGE

Strengths:

1. Correctly fixes all three memory leaks identified in bot review
2. Follows proper handle ownership semantics throughout
3. Complete coverage of all multi-parameter modes
4. Clear documentation and rationale
5. No performance, security, or architectural concerns

Minor follow-up:

• Consider adding error path test coverage (separate issue)

Recommendation:
✅ MERGE immediately - This fixes critical memory leaks with no downside

---

Reviewed by: Claude Code (system-architect mode)
Review type: Comprehensive (code quality, bugs, performance, security, test coverage)
Review date: 2025-12-28