v3/control: replace unchecked type asserts in DecodeControl with comma-ok#589
Merged
cpuschma merged 2 commits intoMay 17, 2026
Merged
Conversation
…a-ok DecodeControl trusts that every controlType is a string and every criticality is a bool, and falls back to value.Value.(string) for any unknown control type. When a malformed or hostile server returns a non-string (or nil) in any of these positions, the .(string) / .(bool) cast panics inside the caller's goroutine. The user repro in go-ldap#561 hit the default case where value.Value was nil: panic: interface conversion: interface {} is nil, not string go-ldap/ldap/v3.DecodeControl(...) go-ldap/ldap/v3.(*Conn).SimpleBind(...) Use comma-ok asserts at every control-frame field, return clear errors when the cast fails, and in the default ControlString case fall back to value.Data.String() when value.Value isn't a string so the application gets the bytes instead of a crash. The existing TestDecodeControl table covers the normal-path cases and still passes; the new behaviour only kicks in on malformed input where the previous code panicked. Closes go-ldap#561 Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>
Member
|
@c-tonneslan Thank you for your PR Could you extend the current test suite for the controls to cover your addition? |
Covers the new error paths (non-string control type in the 1/2/3-child cases, non-bool criticality in the 3-child case) and the ControlString fallback when value.Value isn't a string. All four would have panicked before go-ldap#589. Signed-off-by: Charlie Tonneslan <cst0520@gmail.com>
Contributor
Author
|
Done, pushed a follow-up commit with table-driven cases for the new error paths (non-string control type in each of the 1/2/3-child branches and non-bool criticality in the 3-child branch) plus a positive test for the ControlString fallback when value.Value isn't a string. Each one would've panicked before this PR. |
cpuschma
approved these changes
May 16, 2026
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
2 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.
Closes #561.
`DecodeControl` assumes every controlType is a string and every criticality is a bool, and the default `ControlString` case casts `value.Value.(string)` unconditionally. When a malformed or hostile server returns a non-string (or nil) in any of these positions, the cast panics the caller's goroutine — the repro in #561 hit the default case where `value.Value` was nil:
```
panic: interface conversion: interface {} is nil, not string
go-ldap/ldap/v3.DecodeControl(...)
go-ldap/ldap/v3.(*Conn).SimpleBind(...)
```
Switched every cast to comma-ok form, returning a clear error when the assertion fails. In the default `ControlString` branch I fall back to `value.Data.String()` when `value.Value` isn't a string, so the application gets the raw bytes instead of a crash.
Existing `TestDecodeControl` table still passes; the new behaviour only kicks in on input that previously panicked.