lestrrat-go / libxml2

Interface to libxml2, with DOM interface
MIT License
230 stars 55 forks source link

Fix "checkptr: pointer arithmetic result points to invalid allocation" panic with race flag #97

Closed khasanovbi closed 6 months ago

khasanovbi commented 6 months ago

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).

github-actions[bot] commented 6 months ago

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.

lestrrat commented 6 months ago

Sorry, this slipped past me. Checking.

lestrrat commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@khasanovbi thanks, merged!