Skip to content
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.

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

refactor: Ensure that HTTP response body objects are always closed. #592

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

SahibYar
Copy link
Contributor

This PR fixes a potential resource leak issue in our code. Previously, the HTTP response body was being ignored in cases like:

_, err := c.PostForm("/sudo", i, nil)

This oversight could lead to resource exhaustion over time. (Reference)

Changes:

•	Explicitly added resp.Body.Close() in all relevant places where HTTP requests are made.
•	Ensures that resources are properly released after the response is handled.

Testing

I ran make all and this is my output after these changes

go-fastly % make all
==> Downloading Go module
==> Downloading development dependencies
Warning: semgrep 1.104.0 is already installed and up-to-date.
To reinstall 1.104.0, run:
	brew reinstall semgrep
==> Tidying module
==> Running gofmt
==> Fixing imports
==> Testing go-fastly
?       github.com/fastly/go-fastly/v9/fastly/domains   [no test files]
?       github.com/fastly/go-fastly/v9/fastly/products  [no test files]
?       github.com/fastly/go-fastly/v9/internal/test_utils      [no test files]
ok      github.com/fastly/go-fastly/v9/fastly   2.628s
ok      github.com/fastly/go-fastly/v9/fastly/computeacls       (cached)
ok      github.com/fastly/go-fastly/v9/fastly/domains/v1        (cached)
ok      github.com/fastly/go-fastly/v9/fastly/imageoptimizerdefaultsettings     (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/botmanagement    (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/brotlicompression        (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/ddosprotection   (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/domaininspector  (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/fanout   (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/imageoptimizer   (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/logexplorerinsights      (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/ngwaf    (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/origininspector  (cached)
ok      github.com/fastly/go-fastly/v9/fastly/products/websockets       (cached)
ok      github.com/fastly/go-fastly/v9/internal/productcore     (cached)
==> Running go vet
==> Running staticcheck
staticcheck 2023.1.7 (v0.4.7)
if command -v semgrep &> /dev/null; then semgrep ci --config auto --exclude-rule generic.secrets.security.detected-private-key.detected-private-key ; fi
					
					
┌────────────────┐
│ Debugging Info │
└────────────────┘
					
	SCAN ENVIRONMENT
	versions    - semgrep 1.104.0 on python 3.13.1                       
	environment - running in environment git, triggering event is unknown
																														
	Scanning 1282 files (only git-tracked) with 1057 Code rules:
			
	CODE RULES
																														
	Language      Rules   Files          Origin      Rules                                                                
	─────────────────────────────        ───────────────────                                                               
	<multilang>      47    1162          Community    1057                                                                
	yaml             31     988                                                                                           
	go               83     153                                                                                           
	bash              4       5                                                                                           
																														
					
	SUPPLY CHAIN RULES
					
	No rules to run.
					
			
	PROGRESS
	
	━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        
				
				
┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
	Scan was limited to files tracked by git.
	Partially scanned: 1 files only partially analyzed due to parsing or internal Semgrep errors
	Scan skipped: 1 files larger than 1.0 MB, 119 files matching .semgrepignore patterns
	For a full list of skipped files, run semgrep with the --verbose flag.

(need more rules? `semgrep login` for additional free Semgrep Registry rules)

CI scan completed successfully.
	Found 0 findings (0 blocking) from 1057 rules.
	No blocking findings so exiting with code 0

@Integralist
Copy link
Collaborator

@philippschulte this might be worth including in a patch release.

@kpfleming kpfleming changed the title Addresses potential resource leaks caused by unclosed response bodies refactor: Ensure that HTTP response body objects are always closed. Feb 6, 2025
@kpfleming
Copy link
Contributor

Thanks for this contribution! I'm not thrilled about having to repeat this code pattern all over the place, as it will likely lead to a future programmer forgetting to do it, but I can't think of a practical way to avoid the duplication.

@kpfleming kpfleming merged commit 3161fa7 into fastly:main Feb 6, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants