Refuse TRANS2_SET_FS_INFORMATION with the mandated status (Fixes #1215) - #1284
Merged
Merged
Conversation
[MS-CIFS] 2.2.6.5 says a server receiving this reserved subcommand MUST return STATUS_SMB_NO_SUPPORT, and the handler quoted that requirement while answering STATUS_NOT_SUPPORTED instead. Its comment explained the substitution by saying the mandated status had no constant in windows/errors/nt_status. It does: cifs.go declares NT_STATUS_SMB_NO_SUPPORT as 0xFFFF0002, the ERRSRV/ERRnosupport pair in the composite form [MS-CIFS] gives the rest of its extension values. The comment was written from the [MS-ERREF] table alone, where the name genuinely is absent, without checking the sibling file that carries the CIFS additions. Returning that constant is most of the fix, but not all of it. A client that did not negotiate NT status codes receives the legacy pair instead, and DOSError builds one either from its table or by decomposing a CIFS composite. The decomposition requires a zero top byte, which is how it tells a composite apart from a real NTSTATUS whose severity bits would otherwise be read as an error code. NT_STATUS_SMB_NO_SUPPORT has ErrorCode 0xFFFF, so its top byte is 0xFF and the decomposition declines it — the status would have fallen through to ERRSRV/ERRsrverror, trading a wrong-but-plausible answer for a different one. It is tabulated explicitly instead, which is why the entry carries a comment saying why it cannot simply be decomposed like its neighbours. Both tests move with the behaviour. The subcommand test asserted STATUS_NOT_SUPPORTED and repeated the false claim about the missing constant; it now asserts the mandated status. A new test covers the legacy encoding, since that arm is otherwise unexercised: it pins ERRSRV/ERRnosupport, asserts the value did not fall through to the generic ERRSRV/ERRsrverror, and re-encodes the pair to confirm it is still wire-identical to the NTSTATUS like every other composite. Nothing sends this subcommand — it is reserved — so the change is a conformance fix rather than a live one.
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.
Linked Issue
Closes #1215Root Cause
handleTrans2SetFsInformationquoted the requirement it was breaking. [MS-CIFS] 2.2.6.5 says a server receiving this reserved subcommand MUST returnSTATUS_SMB_NO_SUPPORT; the handler cited that sentence and then returnedSTATUS_NOT_SUPPORTED, explaining the substitution with "STATUS_SMB_NO_SUPPORT has no constant in windows/errors/nt_status, so the closest available is used".That premise was false.
cifs.godeclaresNT_STATUS_SMB_NO_SUPPORTas0xFFFF0002— the ERRSRV/ERRnosupport pair in the composite form [MS-CIFS] uses for its extension values. The comment was written from the [MS-ERREF] table alone, where the name genuinely is absent, without checking the sibling file that carries the CIFS additions. The test then asserted the substituted value and repeated the same claim, so the suite pinned the defect rather than catching it.Fix Description
Returning the right constant is most of the fix. The rest is the legacy encoding path, which is not obvious from the handler:
A client that did not negotiate
SMB_FLAGS2_NT_STATUS_ERROR_CODESreceives the legacy SMBSTATUS pair, whichDOSErrorproduces either from its table or by decomposing a CIFS composite. The decomposition requires a zero top byte — that is how it distinguishes a composite from a real [MS-ERREF] NTSTATUS whose severity bits would otherwise be misread as an ErrorCode.NT_STATUS_SMB_NO_SUPPORThas ErrorCode0xFFFF, so its top byte is0xFFand the decomposition declines it. Left alone, the status would have fallen through tounmappedError(ERRSRV/ERRsrverror) — swapping one wrong answer for another on exactly the clients the legacy path exists to serve.So the value is tabulated explicitly in
dosErrors, with a comment recording why it cannot simply be decomposed like its neighbours.Both tests move with the behaviour, and a new one covers the arm that was otherwise unexercised.
How Verified
TestReservedSubcommandsUseTheirMandatedStatusnow assertsSTATUS_SMB_NO_SUPPORT; both it and the newTestDOSErrorTabulatesSMBNoSupportpass.dosErrorsentry makesTestDOSErrorTabulatesSMBNoSupportfail, so the new coverage genuinely bites rather than passing vacuously. Restored and re-verified afterwards.go test -count=1 ./...clean — 317 packages ok, 0 failures, matchingmain.go vetclean,gofmt -lreports nothing for the package.windows/amd64,windows/386,linux/arm64,linux/386.cifs.go(0xFFFF0002) and its class/code decomposition checked against the ERRSRV/ERRnosupport pair, rather than assumed from the name.Test Coverage
Added —
TestDOSErrorTabulatesSMBNoSupportinerrors_test.go. It pins ERRSRV/0xFFFF, asserts explicitly that the value did not fall through to the generic ERRSRV/ERRsrverror, and re-encodes the pair to confirm it is still wire-identical to the NTSTATUS, asTestDOSErrorDecomposesCIFSStatusrequires of every other composite. Its doc comment records why this one composite is tabulated rather than decomposed, so the entry is not mistaken for redundancy later.Modified —
TestReservedSubcommandsUseTheirMandatedStatusinremaining_subcommands_test.go: asserts the mandated status, and its comment no longer repeats the false claim about the missing constant. Its existing check that the status is not the genericSTATUS_NOT_IMPLEMENTEDis unchanged.Scope of Change
handler_remaining_subcommands.go(the returned status and its comment),errors.go(onedosErrorsentry),errors_test.go(new test),remaining_subcommands_test.go(updated assertion). 43 insertions, 11 deletions.dosErrorsaddition affects onlyNT_STATUS_SMB_NO_SUPPORT, which nothing else returns.Risk and Rollout
Nothing sends this subcommand — it is reserved — so no live path changes; this is a conformance fix. The
dosErrorsentry is additive and keyed to a single status.Notes
Found in Phase 1 of the
windows/errorswork (#1213 / PR #1214), which corrected the stale package path in the two comments that carried the false claim but deliberately left the behaviour alone, so a package move would not smuggle in a behavioural change.The false premise is the same shape as the defects the error-table migration kept finding: a comment or a constant that is self-consistent, agrees with everything around it, and is wrong about something only checkable elsewhere. Here the check was one
grepaway in a sibling file.