Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FuzzRPCClientPartialLength test (MSSQL Server) #25658

Merged
merged 1 commit into from
May 15, 2023

Conversation

gabrielcorado
Copy link
Contributor

The test failed with:

$ go test -run=^$ ./lib/srv/db/sqlserver/protocol -fuzz=FuzzRPCClientPartialLength
fuzz: elapsed: 0s, gathering baseline coverage: 0/59 completed
failure while testing seed corpus entry: FuzzRPCClientPartialLength/b5768bce0ade2442
fuzz: elapsed: 2s, gathering baseline coverage: 0/59 completed
--- FAIL: FuzzRPCClientPartialLength (2.12s)
    --- FAIL: FuzzRPCClientPartialLength (0.00s)
        fuzz_test.go:62:
                Error Trace:    teleport/teleport/lib/srv/db/sqlserver/protocol/fuzz_test.go:62
                                                        /opt/homebrew/Cellar/go/1.20.2/libexec/src/reflect/value.go:586
                                                        /opt/homebrew/Cellar/go/1.20.2/libexec/src/reflect/value.go:370
                                                        /opt/homebrew/Cellar/go/1.20.2/libexec/src/testing/fuzz.go:335
                Error:          Received unexpected error:
                                failed to convert packet to SQL packet: runtime error: invalid memory address or nil pointer dereference
                Test:           FuzzRPCClientPartialLength

FAIL
exit status 1
FAIL    github.com/gravitational/teleport/lib/srv/db/sqlserver/protocol 2.761s

Thanks, @Tener, for pointing it out!

The problem was related to the packet fixture used on this test, it had two errors:

  • Packet length was not properly encoded and limited the packet size.
  • The packet did not contain all the data if the length was not divisible by the chunks.

Both were addressed in this PR.

Note: No production code changes.

@gabrielcorado gabrielcorado self-assigned this May 4, 2023
@github-actions github-actions bot added database-access Database access related issues and PRs size/sm labels May 4, 2023
@github-actions github-actions bot requested review from Joerger and nklaassen May 4, 2023 19:39
Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

Good stuff 👍

We may want to add FuzzRPCClientPartialLength to fuzz/oss-fuzz-build.sh so that it gets covered by OSS-Fuzz project.

The changes are rather straightforward, but if you want to test them you should do:

> git clone --depth=1 https://github.com/google/oss-fuzz.git
> cd oss-fuzz
> python3 infra/helper.py build_fuzzers teleport ~/path/to/repo/teleport

Note:

  • the build process leaves some changes to the repo under ~/path/to/repo/teleport.
  • on M1 you should make Docker use Rosetta emulation or the build will take hours.

@gabrielcorado gabrielcorado added this pull request to the merge queue May 15, 2023
Merged via the queue into master with commit a80497d May 15, 2023
@gabrielcorado gabrielcorado deleted the gabrielcorado/sqlserver-fuzz-test-fix branch May 15, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-access Database access related issues and PRs size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants