Add test coverage for memory protection error handling #1051
Conversation
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
github-actions
bot
added
impact:non-functional
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
os-d
reviewed
Nov 13, 2025
Javagedes
reviewed
Nov 13, 2025
Javagedes
reviewed
Nov 13, 2025
Javagedes
reviewed
Nov 13, 2025
Collaborator
Javagedes
left a comment
Javagedes
left a comment
There was a problem hiding this comment.
Overall, The tests look good. Thank you! Approving with the expectation that my comments are resolved and the debug_assertion portions are replaced with the #[should_panic(expected = "<panic message>"] as others requested.
makubacki
reviewed
Nov 13, 2025
makubacki
reviewed
Nov 13, 2025
os-d
reviewed
Nov 14, 2025
garybeihl
force-pushed
the
main
branch
4 times, most recently
from
b2d144e to
a632ee4
Compare
os-d
approved these changes
Nov 18, 2025
Contributor
|
@Javagedes @makubacki can you re-review when you have a chance? |
makubacki
approved these changes
Nov 18, 2025
This was referenced Nov 19, 2025
garybeihl
force-pushed
the
main
branch
3 times, most recently
from
96508f5 to
89364ca
Compare
…handling Task #1030 requests improved unit test coverage in patina_dxe_core/src/image.rs. This commit adds comprehensive test coverage for all three error paths introduced by Fix #176, improving overall coverage metrics. New tests added: 1. apply_memory_protections_should_fail_when_section_address_not_in_gcd() - Tests error path #1: GCD descriptor lookup failure - Strategy: Initialize GCD without adding memory descriptors (empty RBT) - Verifies that missing GCD descriptors cause image load to fail with NotFound 2. load_image_should_fail_with_section_alignment_overflow() - Tests error path #2: Section size alignment calculation overflow - Strategy: Craft PE image with VirtualSize that overflows when aligned - Verifies alignment overflow errors are propagated as LoadError 3. load_image_should_fail_with_unaligned_section_address() - Tests error path #3: set_memory_space_attributes failure - Strategy: Create PE image with unaligned section VirtualAddress - Verifies memory attribute setting failures cause image load to fail 4. load_image_should_succeed_with_proper_memory_protections() - Positive test: Verifies valid images still load successfully - Validates both full load_image() flow and direct core_load_pe_image() calls - Ensures Fix #176 doesn't break normal operation - All critical error handling paths from Fix #176 now have test coverage All 387 tests pass successfully with no regressions. Coverage verified with cargo llvm-cov. Addresses #1030 Related to #176
This commit addresses all reviewer feedback on the test coverage implementation for Fix #176 memory protection error handling. Changes made in response to review comments: 1. Replaced #[cfg(not(debug_assertions))] conditionals with proper Result checking - Tests now check Result returned by with_global_lock() - Debug builds: assert!(result.is_err()) to verify panic was caught - Release builds: assert!(result.is_ok()) to verify normal completion 2. Fixed variable naming error (_result vs result) - Changed `let _result = ...` to `let result = ...` in all three error tests - Tests now properly use the result variable for assertions 3. Narrowed unsafe blocks to only cover actual unsafe operations - Test initialization functions isolated in unsafe blocks with SAFETY comments - Memory allocation operations isolated with SAFETY comments - Safe code moved outside unsafe blocks 4. Added SAFETY comments explaining rationale for each unsafe block - Test initialization: "manipulate global state (GCD, protocol DB, system table)" - Memory allocation: "Allocating memory for fake image buffer to construct test data" - Slice creation: "Creating a raw slice from allocated buffer for test purposes" 5. Retained Task #1030 references in test comments alongside Fix #176 - All test comments now reference both "Fix #176" and "(Task #1030 coverage improvement)" All 13 tests in image.rs pass successfully. Total workspace: 1,197+ tests pass with no failures. Addresses review feedback on #1051 Related to #1030, #176
This commit addresses reviewer feedback on PR #1051: 1. Removed overarching comment block before test section - The comprehensive comment explaining all three test scenarios was redundant since each test has sufficient self-explanatory comments - Individual test comments provide sufficient documentation 2. Removed #[cfg(debug_assertions)] conditionals - Replaced conditional assertions on with_global_lock() return value with simple comment explaining debug vs release behavior - Tests now rely on assertions inside the closure to validate error behavior, rather than checking the with_global_lock wrapper - Removes conditional compilation directives across all three tests Changes improve code clarity while maintaining full test coverage. All 13 image.rs tests continue to pass. Addresses review feedback on #1051 Related to #1030, #176 Signed-off-by: Gary Beihl <garybeihl@microsoft.com>
Add explicit 'let _ =' to ignore Result values from test_support::with_global_lock() calls in three test functions. This satisfies clippy's unused-must-use lint. Signed-off-by: Gary Beihl <garybeihl@microsoft.com>
Fix three tests that were hitting debug_assert!(false) in debug builds. The panics were being caught by with_global_lock() but not properly validated, causing tests to pass without actually checking error paths. Changes: - Capture Result from with_global_lock() instead of ignoring with let _ - Add conditional assertions after test closures: * Debug builds: Verify panic was caught (result.is_err()) * Release builds: Verify test completed successfully (result.is_ok()) - Remove specific line number references from comments - Mark third test as debug-only (#[cfg(debug_assertions)]) as it specifically tests a debug_assert path that doesn't cause real errors in release builds Tests fixed: - apply_memory_protections_should_fail_when_section_address_not_in_gcd - load_image_should_fail_with_section_alignment_overflow - load_image_should_fail_with_unaligned_section_address (debug-only) Verified: - Debug mode: 13 tests pass - Release mode: 12 tests pass (third test excluded) Addresses feedback from os-d on PR #1051.
makubacki
approved these changes
Nov 21, 2025
makubacki
pushed a commit
that referenced
this pull request
Dec 2, 2025
Fix bug where set_memory_space_attributes() errors were logged but not propagated, causing silent failures in release builds. When GCD memory attribute setting fails, the error must be returned to prevent images from loading with incorrect memory protections. In debug builds, debug_assert!(false) would panic, masking the issue. In release builds, the assert compiles away, allowing execution to continue despite the error. This could lead to GCD and page table desynchronization. The fix adds `return Err(e);` after the debug_assert to properly propagate the error to callers, matching the pattern used elsewhere in the same function for page table attribute failures (line 2524). Fixes #1083 Related to PR #1051 which adds test coverage for this error path. Signed-off-by: Gary Beihl <garybeihl@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Description
Task #1030 requests improved unit test coverage for memory protection error handling. This commit adds comprehensive test coverage for all three error paths introduced by #176, improving overall coverage metrics.
How This Was Tested
Integration Instructions
N/A if nothing is required.