CodeQL 4: fix: dispose disposables on exception paths#192
Open
rlorenzo wants to merge 2 commits into
Open
Conversation
Closes CodeQL cs/dispose-not-called-on-throw (10) and cs/local-not-disposed (6). Effort Excel-generation services (7 files, 10 XLWorkbook sites): convert `var wb = new XLWorkbook(); ...; wb.Dispose();` to `using var wb = new XLWorkbook();` so the workbook is released even if SaveAs or any of the intermediate ExcelHelper calls throw. Other disposables that were leaked entirely: - CMS.cs DecryptFile/DecryptAES: add `using` to Aes, MemoryStream, and StreamWriter locals. - BiorenderStudentLookup: add `using` to the SemaphoreSlim throttler. - F5HttpRequest.HandleConnectionFail: add `using` to the probe HttpRequestMessage.
Contributor
📝 WalkthroughWalkthroughThis PR converts manual resource disposal to C# ChangesResource Lifecycle Management
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
=======================================
Coverage 42.96% 42.96%
=======================================
Files 877 877
Lines 51468 51456 -12
Branches 4802 4802
=======================================
- Hits 22113 22110 -3
+ Misses 28831 28822 -9
Partials 524 524
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ReSharper UsingStatementResourceInitialization: object initializer inside 'using' means an exception during property assignment would skip Dispose. Split into 'using var x = new()' + per-property assign.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/Classes/Utilities/F5HttpRequest.cs (1)
49-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispose the warm-up response returned by
Send(newRequest).The response from the session-establishing GET is ignored and never disposed. Wrap it in
usingbefore issuing the retried request.Proposed fix
-//don't care about the response -await Send(newRequest); +// don't care about payload, but dispose response resources +using var warmupResponse = await Send(newRequest); return await Send(await CopyHttpRequest(request), 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Classes/Utilities/F5HttpRequest.cs` around lines 49 - 50, The call to Send(newRequest) performs a warm-up GET whose HttpResponseMessage is never disposed; change the logic in the method that calls Send(newRequest) so that the result is awaited into a variable and wrapped in a using (or disposed) before proceeding to call Send(await CopyHttpRequest(request), 1) — i.e., obtain the response from Send(newRequest), dispose it (using a using block or calling Dispose()), then execute the retried request built by CopyHttpRequest(request).web/Areas/Computing/Services/BiorenderStudentLookup.cs (1)
23-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSemaphore permit leak can deadlock the loop on invalid emails.
WaitAsynchappens before validation, butReleaseonly happens in the task path. Invalid emails consume permits permanently and can block at Line 25 after 20 invalid entries.Proposed fix
foreach (var email in emails) { - await throttler.WaitAsync(); - var emailTrimmed = email.Trim(); if (!emailTrimmed.Contains('@')) { emailTrimmed += "@ucdavis.edu"; } - if (IsValidEmail(emailTrimmed)) - { - resultList.Add( - Task.Run(async () => - { - try - { - return await GetSingleStudent(emailTrimmed); - } - finally - { - throttler.Release(); - } - }) - ); - } + if (!IsValidEmail(emailTrimmed)) + continue; + + await throttler.WaitAsync(); + resultList.Add( + Task.Run(async () => + { + try + { + return await GetSingleStudent(emailTrimmed); + } + finally + { + throttler.Release(); + } + }) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs` around lines 23 - 47, The loop calls throttler.WaitAsync() before validating emails, causing permits to be consumed for invalid addresses and never released; move the throttler.WaitAsync call so it's only awaited when you actually schedule a task (i.e., perform emailTrimmed = email.Trim(); normalize domain; if (IsValidEmail(emailTrimmed)) { await throttler.WaitAsync(); resultList.Add(Task.Run(async () => { try { return await GetSingleStudent(emailTrimmed); } finally { throttler.Release(); } })); } ), or alternatively ensure you call throttler.Release() on the invalid path immediately after WaitAsync; update the code around the foreach, IsValidEmail, throttler.WaitAsync, GetSingleStudent and throttler.Release usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/Areas/Computing/Services/BiorenderStudentLookup.cs`:
- Around line 23-47: The loop calls throttler.WaitAsync() before validating
emails, causing permits to be consumed for invalid addresses and never released;
move the throttler.WaitAsync call so it's only awaited when you actually
schedule a task (i.e., perform emailTrimmed = email.Trim(); normalize domain; if
(IsValidEmail(emailTrimmed)) { await throttler.WaitAsync();
resultList.Add(Task.Run(async () => { try { return await
GetSingleStudent(emailTrimmed); } finally { throttler.Release(); } })); } ), or
alternatively ensure you call throttler.Release() on the invalid path
immediately after WaitAsync; update the code around the foreach, IsValidEmail,
throttler.WaitAsync, GetSingleStudent and throttler.Release usage accordingly.
In `@web/Classes/Utilities/F5HttpRequest.cs`:
- Around line 49-50: The call to Send(newRequest) performs a warm-up GET whose
HttpResponseMessage is never disposed; change the logic in the method that calls
Send(newRequest) so that the result is awaited into a variable and wrapped in a
using (or disposed) before proceeding to call Send(await
CopyHttpRequest(request), 1) — i.e., obtain the response from Send(newRequest),
dispose it (using a using block or calling Dispose()), then execute the retried
request built by CopyHttpRequest(request).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b073be6-48c3-4aab-a5c2-0425ba3acebf
📒 Files selected for processing (10)
web/Areas/CMS/Data/CMS.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/Effort/Services/DeptSummaryService.csweb/Areas/Effort/Services/EvaluationReportService.csweb/Areas/Effort/Services/MeritMultiYearService.csweb/Areas/Effort/Services/MeritReportService.csweb/Areas/Effort/Services/MeritSummaryService.csweb/Areas/Effort/Services/SchoolSummaryService.csweb/Areas/Effort/Services/TeachingActivityService.csweb/Classes/Utilities/F5HttpRequest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes ~16 CodeQL alerts in two related rule families:
cs/dispose-not-called-on-throw(10) andcs/local-not-disposed(6). All same fix shape - wrap IDisposable locals withusing.Effort Excel-generation services (10 alerts)
Seven files have the pattern
var wb = new XLWorkbook(); ...; wb.Dispose();where the explicit Dispose at the bottom is bypassed ifSaveAsor any of the intermediateExcelHelper/ExcelAccessibilityHelpercalls throw. Converted tousing var wb = new XLWorkbook();and removed the now-redundant Dispose:web/Areas/Effort/Services/TeachingActivityService.cs(2 sites)web/Areas/Effort/Services/SchoolSummaryService.cs(1 site)web/Areas/Effort/Services/MeritSummaryService.cs(1 site)web/Areas/Effort/Services/MeritMultiYearService.cs(1 site)web/Areas/Effort/Services/MeritReportService.cs(2 sites)web/Areas/Effort/Services/DeptSummaryService.cs(1 site)web/Areas/Effort/Services/EvaluationReportService.cs(2 sites)Other leaked disposables (6 alerts)
web/Areas/CMS/Data/CMS.cs::DecryptFile-Aeslocal was never disposed.web/Areas/CMS/Data/CMS.cs::DecryptAES-MemoryStream mmsStream,StreamWriter srwTemp,MemoryStream outstreamall leaked.web/Areas/Computing/Services/BiorenderStudentLookup.cs::GetBiorenderStudentInfo-SemaphoreSlim throttlerleaked.web/Classes/Utilities/F5HttpRequest.cs::HandleConnectionFail- probeHttpRequestMessage newRequestleaked.CMS.cs touch is outside the range PR #184 modifies (#184 covers lines 469-516 in DownloadZip; these edits are lines 631+).
Context
Fourth in the
CodeQL N:cleanup series (after #189, #190, #191).Test plan
npm run test:backend- 1946 tests passingnpm run verify:build- clean (0 errors)