Skip to content

Address panic in GetLDAPError, add fuzzer#582

Merged
cpuschma merged 5 commits intogo-ldap:masterfrom
TomSellers:panic/getldaperror_nil_result_code
Mar 1, 2026
Merged

Address panic in GetLDAPError, add fuzzer#582
cpuschma merged 5 commits intogo-ldap:masterfrom
TomSellers:panic/getldaperror_nil_result_code

Conversation

@TomSellers
Copy link
Contributor

@TomSellers TomSellers commented Feb 19, 2026

This PR addresses a panic in the following line that occurs if response.Children[0].Value is nil.

resultCode := uint16(response.Children[0].Value.(int64))

This panic was original discovered when fuzzing our own code.

The PR:

  • Adds check for nil to avoid the panic
  • Refactors the tests so that a test corpus is created that can be used for both tests and a fuzzer. This will reduce the boilerplate code needed when certain types of tests are added in the future.
  • Adds a test for the issue fixed above
  • Adds a fuzzer that is seeded by the corpus
  • Updates the make fuzz command in Makefile to remove fuzzers that no longer exist and add the new one.

Misc other changes:

  • Fixes an issue in Makefile that prevented podman from being used.
  • Fixes an issue in TestExtendedRequest_FastBind where the test would error if the test target wasn't available. Since the error didn't stop the test a panic would be triggered when defer called conn.Close() on a nil conn.

Running the fuzzer with podman:

# prep env to pass pre-fuzz test run
podman machine start
make local-server

# run fuzzer
make fuzz

# to run w/o time constraint
(cd v3 && go test -v -fuzz  ^FuzzGetLDAPError$)

# teardown
stop-local-server
podman machine stop

conn, err := DialURL(ldapServer)
if err != nil {
t.Error(err)
t.Fatal(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now matches the logic in TestExtendedRequest_WhoAmI above and ensures that the test doesn't panic due to calling conn.Close() on a nil conn if the test LDAP server isn't available.

default: fmt vet lint build test

CONTAINER_CMD := $(shell command -v docker 2>/dev/null || shell command -v podman 2>/dev/null)
CONTAINER_CMD := $(shell (command -v docker 2>/dev/null || command -v podman 2>/dev/null))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the prior code, the second shell was not being treated as a make keyword. This resulted in the podman check not working. The change here is to group the shell statements and pass them to shell.

}

// TestNilPacket tests that nil packets don't cause a panic.
func TestNilPacket(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere below: All removed tests have been added to generateGetLDAPErrorCorpus.

@TomSellers
Copy link
Contributor Author

Added a fix and test coverage for another panic shook out by the fuzzer added in this PR. I expected a panic here but didn't know how to replicate the invalid data.

@cpuschma cpuschma added bug enhancement go Pull requests that update go code labels Feb 21, 2026
Copy link
Member

@cpuschma cpuschma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR and your work! Especially the fuzzing part is a nice addition

@TomSellers
Copy link
Contributor Author

Thanks for the approval and comments @cpuschma
Do you have a sense for when this would be landed and released?

@cpuschma cpuschma merged commit 3bbbfb1 into go-ldap:master Mar 1, 2026
4 checks passed
@cpuschma
Copy link
Member

cpuschma commented Mar 1, 2026

I'm planning to create a new release within approx. next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug enhancement go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants