CodeQL 4: fix: dispose disposables on exception paths#192
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.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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.
2 participants
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)