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 "checkptr: pointer arithmetic result points to invalid allocation" panic with race flag #97

Merged
merged 1 commit into from
May 2, 2024

Conversation

khasanovbi
Copy link
Contributor

When we run tests with race flag the program crashes with the following panic:

fatal error: checkptr: pointer arithmetic result points to invalid allocation

goroutine 1 gp=0xc0000061c0 m=0 mp=0x5cffe0 [running]:
runtime.throw({0x515d10?, 0x46abf9?})
        /usr/lib/go-1.22/src/runtime/panic.go:1023 +0x5c fp=0xc000067c70 sp=0xc000067c40 pc=0x4380dc
runtime.checkptrArithmetic(0x0?, {0x0, 0x0, 0x8?})
        /usr/lib/go-1.22/src/runtime/checkptr.go:69 +0x9c fp=0xc000067ca0 sp=0xc000067c70 pc=0x40b5bc
github.com/lestrrat-go/libxml2/clib.XMLDocumentString.func1(0x253bcd0, 0xc000056028, 0xc00001413c, 0xc000067d08, 0x0)
        /home/bulat/go/pkg/mod/github.com/lestrrat-go/libxml2@v0.0.0-20231124114421-99c71026c2f5/clib/clib.go:1547 +0x4a fp=0xc000067ce0 sp=0xc000067ca0 pc=0x4a93aa
github.com/lestrrat-go/libxml2/clib.XMLDocumentString({0x5321c0, 0xc000014120}, {0x0, 0x0}, 0x0)
        /home/bulat/go/pkg/mod/github.com/lestrrat-go/libxml2@v0.0.0-20231124114421-99c71026c2f5/clib/clib.go:1547 +0x1cf fp=0xc000067ea0 sp=0xc000067ce0 pc=0x4a914f
github.com/lestrrat-go/libxml2/dom.(*Document).String(0xc000014120)
        /home/bulat/go/pkg/mod/github.com/lestrrat-go/libxml2@v0.0.0-20231124114421-99c71026c2f5/dom/node_document.go:260 +0x47 fp=0xc000067ee8 sp=0xc000067ea0 pc=0x4ab647
main.main()

Example:

package main

import (
	"fmt"

	"github.com/lestrrat-go/libxml2"
)

const document = `
<CreateBucketConfiguration> 
	<LocationConstraint>us-east-1</LocationConstraint> 
</CreateBucketConfiguration>
`

func main() {
	document, err := libxml2.Parse([]byte(document))
	if err != nil {
		panic(err)
	}

	fmt.Println(document.String())
}

From docs:
Conversion of a uintptr back to Pointer is not valid in general. It's only allowed in the same expression.

I split the PR into 2 commits:

  1. minimal edits so as not to change the ABI
  2. replacing uintptr with unsafe.Pointer throughout the project.

I would like to merge both, but the first one is enough for me).

Copy link

github-actions bot commented May 2, 2024

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 2, 2024
@lestrrat
Copy link
Collaborator

lestrrat commented May 2, 2024

Sorry, this slipped past me. Checking.

@lestrrat lestrrat removed the Stale label May 2, 2024
@lestrrat
Copy link
Collaborator

lestrrat commented May 2, 2024

@khasanovbi I think your second commit changes the public API and thus will break other people's code.
Even if we are to accept such a change in the future, I don't think I can merge the PR as it is right now. Sorry.

Please change the PR accordingly.

As a side note I think we'd have to actually cut a release if we are going to change the public API after years of it not being modified.
(disclaimer: I'm a bit reluctant to babysit this project seriously ATM, feel free to prod me, but I can't promise anything right now)

@khasanovbi
Copy link
Contributor Author

@lestrrat Maybe I'm wrong, but I think hardly anyone uses Pointer() methods externally.
What could pointers be used for outside the library?
But rolled back the changes to safe ones anyway.
I can make a second commit in a separate PR, in case there is an opportunity to update the library in the future.

@lestrrat
Copy link
Collaborator

lestrrat commented May 2, 2024

What could pointers be used for outside the library?

@khasanovbi For starters, xmlsec :) (more specifically, here)

But even without this particular example, I think my point is that for any given API, there's always bound to be an unexpected user. You tend to only notice them once you make breaking changes and they scream really loud at you. Obviously I can't prove that this is the case for this particular library, but that has been my experience with FOSS over the last 20+ years :)

@lestrrat lestrrat merged commit b65cd51 into lestrrat-go:master May 2, 2024
6 checks passed
@lestrrat
Copy link
Collaborator

lestrrat commented May 2, 2024

@khasanovbi thanks, merged!

@khasanovbi khasanovbi deleted the fix_pointers_in_race branch May 2, 2024 13:04
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.

None yet

2 participants