Skip to content

fix issue 84 #85

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

Merged
merged 5 commits into from
Apr 1, 2021
Merged

fix issue 84 #85

merged 5 commits into from
Apr 1, 2021

Conversation

achille-roussel
Copy link
Contributor

Fixes #84

This PR modifies the functions in the ascii package to prevent pointers being advanced beyond the end of the memory areas they started in.

Technically, we were never dereferencing pointers with addresses beyond the allocated areas, but Go makes it invalid to simply have pointers that address unallocated memory. To my understanding, this is done to prevent exposing out-of-bounds pointers to the GC, which could mistakenly create references to objects that shouldn't have any, or potentially cause runtime errors if the GC finds pointers that aren't mapping to any allocated areas.

I believe that an alternative solution could have been to use //go:nosplit on the functions, which should prevent these "out-of-bounds" pointers from being seen by the GC, tho I haven't found enough literature on the subject to feel confident enough in that assumption.

Thanks @aaron42net for reporting the issue!

Copy link
Contributor

@chriso chriso left a comment

Choose a reason for hiding this comment

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

Can we incorporate -race into our test commands so we catch these issues early?

Perhaps something like this:

diff --git i/Makefile w/Makefile
index 09c0db2..f70b7a0 100644
--- i/Makefile
+++ w/Makefile
@@ -11,13 +11,8 @@ go-fuzz-corpus := ${GOPATH}/src/github.com/dvyukov/go-fuzz-corpus
 go-fuzz-dep := ${GOPATH}/src/github.com/dvyukov/go-fuzz/go-fuzz-dep
 
 test:
-   go test -v -cover ./ascii
-   go test -v -cover ./json
-   go test -v -cover ./proto
-   go test -v -cover -tags go1.15 ./json
-   go test -v -cover ./iso8601
-   go run ./json/bugs/issue11/main.go
-   go run ./json/bugs/issue18/main.go
+   go test -v -race -cover ./...
+   go test -v -race -cover -tags go1.17 ./json
 
 $(benchstat):
    GO111MODULE=off go get -u golang.org/x/perf/cmd/benchstat

If we use the same main_test.go pattern in the issue11/issue18 dirs then they'll be run automatically by go test ./...

@achille-roussel
Copy link
Contributor Author

@chriso I brought your suggestions in, but kept each test separated so we can run them in parallel with make -j (running with -race takes way more time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"checkptr: pointer arithmetic result points to invalid allocation" caused by #81
2 participants