Correct passthrough sequences after refactor #4125
Merged
Conversation
This file contains 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
… the ProcessString loop continues to work correctly with _run storage.
…tring. Add comments to loop to explain _run operation since it's confusing.
DHowett-MSFT
approved these changes
Jan 6, 2020
zadjii-msft
approved these changes
Jan 7, 2020
|
Nit: reword title to be imperative |
|
Checked x86 conparser.unit.tests.dll locally
So I'm going to re-run the x86 failed job. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
3 tasks
ghost
pushed a commit
that referenced
this pull request
Jan 14, 2020
## Summary of the Pull Request Perform checking on `std::basic_string_view<T>.substr()` calls to prevent running out of bounds and sporadic Privileged Instruction throws during x86 tests. ## PR Checklist * [x] Closes the x86 tests failing all over the place since #4125 for no apparent reason * [x] I work here * [x] Tests pass ## Detailed Description of the Pull Request / Additional comments It appears that not all `std::basic_string_view<T>.substr()` calls are created equally. I rooted around for other versions of the code in our source tree and found several versions that were less careful about checking the start position and the size than the one that appears when building locally on dev machines. My theory is that one of these older versions is deployed somewhere in the CI. Instead of clamping down the size parameter appropriately or throwing correctly when the position is out of bounds, I believe that it's just creating a substring with a bad range over an invalid/uninitialized memory region. Then when the test operates on that, sometimes it turns out to trigger the privileged instruction NTSTATUS error we are seeing in CI. ## Test Procedure 1. Fixed the thing 2. Ran the CI and it worked 3. Reverted everything and turned off all of the CI build except just the parser tests (and supporting libraries) 4. Ran CI and it failed 5. Put the fix back on top (cherry-pick) 6. It worked. 7. Ran it again. 8. It worked. 9. Turn all the rest of the CI build back on
DHowett-MSFT
pushed a commit
that referenced
this pull request
Jan 24, 2020
## Summary of the Pull Request Perform checking on `std::basic_string_view<T>.substr()` calls to prevent running out of bounds and sporadic Privileged Instruction throws during x86 tests. ## PR Checklist * [x] Closes the x86 tests failing all over the place since #4125 for no apparent reason * [x] I work here * [x] Tests pass ## Detailed Description of the Pull Request / Additional comments It appears that not all `std::basic_string_view<T>.substr()` calls are created equally. I rooted around for other versions of the code in our source tree and found several versions that were less careful about checking the start position and the size than the one that appears when building locally on dev machines. My theory is that one of these older versions is deployed somewhere in the CI. Instead of clamping down the size parameter appropriately or throwing correctly when the position is out of bounds, I believe that it's just creating a substring with a bad range over an invalid/uninitialized memory region. Then when the test operates on that, sometimes it turns out to trigger the privileged instruction NTSTATUS error we are seeing in CI. ## Test Procedure 1. Fixed the thing 2. Ran the CI and it worked 3. Reverted everything and turned off all of the CI build except just the parser tests (and supporting libraries) 4. Ran CI and it failed 5. Put the fix back on top (cherry-pick) 6. It worked. 7. Ran it again. 8. It worked. 9. Turn all the rest of the CI build back on (cherry picked from commit 4129ceb)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 of the Pull Request
When refactoring the
StateMachine::ProcessStringalgorithm to use safer structures, I made an off-by-one error when attempting to simplify the loop.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
The algorithm in use exploited holding onto some pointers and sizes as it rotated around the loop to call back as member variables in the pass-through function
FlushToTerminal.As a part of the refactor, I adjusted to persisting a
std::wstring_viewof the currently processing string instead of pointer/size. I also attempted to simplify the loop at the same time as both the individual and group branches were performing some redundant operations in respect to updating the "run" length.Turns out, I made a mistake here. I wrote it so it worked correctly for the bottom half where we transition from bulk printing to an escape but then I messed up the top case.
Validation Steps Performed
ProcessStringloop that work with the_runvariable.