[PR #129] [CLOSED] fix: prevent resource leaks and unbounded allocation in tools #178

New Issue

2026-06-06T22:09:34-04:00

yindo commented

2026-06-06 22:09:34 -04:00

📋 Pull Request Information

Original PR: https://github.com/vxcontrol/pentagi/pull/129
Author: @mason5052
Created: 2/22/2026
Status: ❌ Closed

Base: feature/project_improvements ← Head: fix/resource-cleanup-browser-terminal

📝 Commits (2)

b6fba4e fix: prevent resource leaks and unbounded allocation in tools
78e7dfa Merge branch 'feature/project_improvements' into fix/resource-cleanup-browser-terminal

📊 Changes

2 files changed (+11 additions, -7 deletions)

View changed files

📝 backend/pkg/tools/browser.go (+4 -2)
📝 backend/pkg/tools/terminal.go (+7 -5)

📄 Description

Description of the Change

Problem

Three resource management issues in the tools package:

Response body leak in browser.go (callScraper): The defer resp.Body.Close() was placed after the HTTP status code check. When the server returns a non-200 status, the function returns early without closing the response body. This leaks the underlying TCP connection (held open until GC finalizer runs), which can exhaust connection pools under load.
Unbounded memory allocation in terminal.go (ReadFile): The tarHeader.Size field from a Docker CopyFromContainer response is used directly in make([]byte, tarHeader.Size) without validation. A corrupted or malicious tar archive with a multi-GB Size header field triggers an OOM crash. Negative sizes also cause a panic.
Incomplete tar archive in terminal.go (WriteFile): The tar writer is never explicitly closed after writing content. Without tarWriter.Close(), the two 512-byte zero blocks that mark the end of a tar archive are not flushed. This produces a structurally invalid archive that may fail silently or cause extraction errors on the container side.

Solution

Move defer resp.Body.Close() immediately after the error check for client.Get(), before the status code check. This ensures the body is always closed regardless of the response status.
Add size validation before the make() call: reject files over 50MB and reject negative sizes with clear error messages.
Add explicit tarWriter.Close() with error handling before passing the archive to CopyToContainer.

Ref: #101

Type of Change

Bug fix (non-breaking change which fixes an issue)
Security update

Areas Affected

Security Tools Integration

Testing and Verification

Test Steps

Code review confirms defer resp.Body.Close() now executes on all paths
Verified tar header size validation rejects sizes > 50MB and < 0
Verified tarWriter.Close() is called before archive is sent to container

Security Considerations

Response body leak: Prevents TCP connection exhaustion under sustained non-200 responses from scraper service
Unbounded allocation: Prevents OOM via crafted tar headers (defense-in-depth for container file operations)
Tar writer close: Ensures archive integrity for file write operations

Checklist

Code Quality

My code follows the project's coding standards
All new and existing tests pass
I have run go fmt and go vet (for Go code)

Security

I have considered security implications
Changes maintain or improve the security model

Compatibility

Changes are backward compatible

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/vxcontrol/pentagi/pull/129 **Author:** [@mason5052](https://github.com/mason5052) **Created:** 2/22/2026 **Status:** ❌ Closed **Base:** `feature/project_improvements` ← **Head:** `fix/resource-cleanup-browser-terminal` --- ### 📝 Commits (2) - [`b6fba4e`](https://github.com/vxcontrol/pentagi/commit/b6fba4ef1c1e8a6705258dea2c859e496e0c327b) fix: prevent resource leaks and unbounded allocation in tools - [`78e7dfa`](https://github.com/vxcontrol/pentagi/commit/78e7dfa014ef0021e08d719deb562c225a3302e4) Merge branch 'feature/project_improvements' into fix/resource-cleanup-browser-terminal ### 📊 Changes **2 files changed** (+11 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `backend/pkg/tools/browser.go` (+4 -2) 📝 `backend/pkg/tools/terminal.go` (+7 -5) </details> ### 📄 Description ## Description of the Change ### Problem Three resource management issues in the tools package: 1. **Response body leak in `browser.go`** (`callScraper`): The `defer resp.Body.Close()` was placed **after** the HTTP status code check. When the server returns a non-200 status, the function returns early without closing the response body. This leaks the underlying TCP connection (held open until GC finalizer runs), which can exhaust connection pools under load. 2. **Unbounded memory allocation in `terminal.go`** (`ReadFile`): The `tarHeader.Size` field from a Docker `CopyFromContainer` response is used directly in `make([]byte, tarHeader.Size)` without validation. A corrupted or malicious tar archive with a multi-GB `Size` header field triggers an OOM crash. Negative sizes also cause a panic. 3. **Incomplete tar archive in `terminal.go`** (`WriteFile`): The tar writer is never explicitly closed after writing content. Without `tarWriter.Close()`, the two 512-byte zero blocks that mark the end of a tar archive are not flushed. This produces a structurally invalid archive that may fail silently or cause extraction errors on the container side. ### Solution 1. Move `defer resp.Body.Close()` immediately after the error check for `client.Get()`, before the status code check. This ensures the body is always closed regardless of the response status. 2. Add size validation before the `make()` call: reject files over 50MB and reject negative sizes with clear error messages. 3. Add explicit `tarWriter.Close()` with error handling before passing the archive to `CopyToContainer`. Ref: #101 ### Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [x] Security update ### Areas Affected - [x] Security Tools Integration ### Testing and Verification #### Test Steps 1. Code review confirms `defer resp.Body.Close()` now executes on all paths 2. Verified tar header size validation rejects sizes > 50MB and < 0 3. Verified `tarWriter.Close()` is called before archive is sent to container ### Security Considerations - **Response body leak**: Prevents TCP connection exhaustion under sustained non-200 responses from scraper service - **Unbounded allocation**: Prevents OOM via crafted tar headers (defense-in-depth for container file operations) - **Tar writer close**: Ensures archive integrity for file write operations ### Checklist #### Code Quality - [x] My code follows the project's coding standards - [x] All new and existing tests pass - [x] I have run `go fmt` and `go vet` (for Go code) #### Security - [x] I have considered security implications - [x] Changes maintain or improve the security model #### Compatibility - [x] Changes are backward compatible --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

yindo added the pull-request label 2026-06-06 22:09:34 -04:00

yindo closed this issue

2026-06-06 22:09:35 -04:00

yindo referenced this issue

2026-06-06 22:09:44 -04:00

[PR #178] [MERGED] fix: prevent infinite loop in performAgentChain on repeating tool calls #213

yindo referenced this issue

2026-06-06 22:09:44 -04:00

[PR #180] [MERGED] test: add unit tests for repeatingDetector and clearCallArguments #217

yindo referenced this issue

2026-06-06 22:09:49 -04:00

[PR #206] [MERGED] Agent Supervision System and Open Source Model Enhancements #235