Fix SCMR failure actions marshaling and add regression test (Issue #2046)#2160
Open
alexisbalbachan wants to merge 3 commits intofortra:masterfrom
Open
Fix SCMR failure actions marshaling and add regression test (Issue #2046)#2160alexisbalbachan wants to merge 3 commits intofortra:masterfrom
alexisbalbachan wants to merge 3 commits intofortra:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect marshaling of SERVICE_CONFIG_FAILURE_ACTIONS in scmr.RChangeServiceConfig2W() when lpsaActions is non-empty, and adds a regression test to cover the failure-actions path.
Changes:
- Model
SERVICE_FAILURE_ACTIONSW.lpsaActionsas a pointer to a conformant array ([size_is(cActions)] SC_ACTION*). - Synchronize
cActionswith the pointed action array during marshaling. - Refactor SCMR config query logic in tests and add a dedicated regression test for failure actions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
impacket/dcerpc/v5/scmr.py |
Updates NDR modeling for failure actions to correctly represent lpsaActions as a pointer to an SC_ACTION conformant array and syncs cActions at marshal time. |
tests/dcerpc/test_scmr.py |
Adds query helpers and a regression test validating ChangeServiceConfig2W failure-actions behavior and round-tripping via QueryServiceConfig2W. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Applied Code review suggestions to the test Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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.
Fix
scmr.RChangeServiceConfig2W()forSERVICE_CONFIG_FAILURE_ACTIONSwhenlpsaActionsis populated. (Issue #2046)SERVICE_FAILURE_ACTIONSW.lpsaActionswas modeled as an inline structure, but the SCMR protocol defines itas
[size_is(cActions)] SC_ACTION*. That caused requests with non-empty failure actions to be marshaled incorrectly andfail with rpc_x_bad_stub_data.
Changes:
lpsaActionsas a pointer to a conformant array ofSC_ACTIONcActionssynchronized with the pointed array during marshalingReferences
https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-scmr/58032b71-1e5c-4f2e-8545-34b0f2e8c6ad
https://learn.microsoft.com/en-us/windows/win32/api/winsvc/ns-winsvc-sc_action