Nil pointer panic that should be impossible

[Ethan Reesor]

From this CI job:

panic: runtime error: invalid memory address or nil pointer dereference

[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x51d8b7]

goroutine 1589381 [running]:

strings.EqualFold({0xc000beec20?, 0x0?}, {0x0?, 0xacace7?})

     /usr/local/go/src/strings/strings.go:1111 +0x37

gitlab.com/accumulatenetwork/accumulate/pkg/url.(*URL).Equal(0xc000a74e40?, 0xc00094c540)

     /builds/accumulatenetwork/accumulate/pkg/url/url.go:472 +0x10c

This is in a docker container based on the go:1.22 image, so the panic appears to be happening here:

func EqualFold(s, t string) bool {

// ASCII fast path

i := 0

for ; i < len(s) && i < len(t); i++ {

sr := s[i]

tr := t[i] // <-- line 1111

(*URL).Equal:

func (u *URL) Equal(v *URL) bool {

if u == v {

return true

}

if u == nil || v == nil {

return false

}

return strings.EqualFold(u.String(), v.String())

}

(*URL).String:

func (u *URL) String() string {

if u.memoize.str != "" {

return u.memoize.str

}

u.memoize.str = u.format(nil, true)

return u.memoize.str

}

(*URL).format:

func (u *URL) format(txid []byte, encode bool) string {

var buf strings.Builder

// ... write to the builder

return buf.String()

}

How is this possible? Based on `addr=0x0` in the panic I think this is a nil pointer panic, as opposed to some other kind of segfault. The only way I can reproduce panic-on-string-index is with `(*reflect.StringHeader)(unsafe.Pointer(&s)).Data = 0`, but I don't see how that can be happening here. I'm saving the string but I'm not doing anything weird with it. And the string header is a value type so code that manipulates the returned string shouldn't modify the original. And I'm definitely not doing any kind of unsafe string manipulation like that in my code, anywhere. The only reference to unsafe anywhere in my code is for parameters for calling GetDiskFreeSpaceExW (Windows kernel32.dll call).

[Axel Wagner]

Have you tried running the code with the race detector enabled? I suspect that you are concurrently modifying `u.memoize.str` by calling `u.String()` from multiple goroutines. And the non-zero length of the string header written by one goroutine becomes visible to the other one, before the modification to the data pointer.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/d6f6bb75-45e9-4a38-9bbd-d332e7f3e57cn%40googlegroups.com.

[王李荣]

hi Axel,

is not modifying `u.memoize.str` thread-safe?  the len and the data point should become visible at same time?

[Axel Wagner]

On Thu, Mar 21, 2024 at 2:48 PM 王李荣 <wanglir...@gmail.com> wrote:

hi Axel,

is not modifying `u.memoize.str` thread-safe?  the len and the data point should become visible at same time?

What makes you think that? To be clear, there are no benign data races. Even a data-race on a variable smaller than a word is still a data-race, unless you do it holding a lock or using atomic instructions. But strings are *larger* than single words.

To demonstrate that the effect I am talking about is real, look at this code: https://go.dev/play/p/LzRq9-OH-Xb

 

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/31f77ff2-cf11-4b3e-9b14-874b6cc41da3n%40googlegroups.com.

[Ethan Reesor]

I hadn't used the race detector before. I do see a race warning for (*URL).String() among an embarrassing number of other results. I'm going to update (*URL).String() to use atomic.Pointer to remove the race.

Thanks,

Ethan

You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/Dgy0fyb4Shw/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfG8v0qO_NP4PipEBL%3Dd_Ase9ntWi4EL1dQE_6ubeZQnww%40mail.gmail.com.

[Lirong Wang]

Wow, i am from other language and i thought `string` is immutable or something like that, so thread-safe for this operation. learned something new!!! Thanks

[Axel Wagner]

TBQH the word "mutable" doesn't make a lot of sense in Go (even though the spec also calls strings "immutable").

Arguably, *all* values in Go are immutable. It's just that all *pointers* in Go allow to modify the referenced variables - and some types allow you to get a pointer to a shared variable, which strings don't.

That is, a `[]byte` is immutable - you have to write `x = append(x, v)` specifically because `append` creates a new slice value and overwrites the variable `x` with it.

However, a `[]byte` refers to an underlying array and `&b[0]` allows you to obtain a pointer to that underlying array. So a `[]byte` represents a reference and that reference allows to mutate the referenced storage location. The same goes for a `*T`, a `map[K]V`, or a `type S struct{ X int; P *int }` - `S` itself is immutable, but `S.X` is a reference to some potentially shared variable.

A `string` meanwhile, does not allow you to obtain a pointer to the underlying storage and that's what makes it "immutable". And that does indeed mean that if you pass a `string` value around, that can't lead to data races, while passing a `[]byte` around *might*.

But for this case, it doesn't really matter whether or not the field is a `string` or a `[]byte` or an `int`: Because the "mutable" type is the `*URL`. Which represents a reference to some underlying `URL` variable, that you can then mutate. The race happens because you have a method on a pointer that mutates a field - *regardless* of the type of that field.

I don't know if that helps, it's a bit subtle.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/65400cce-b60c-4bb0-97a7-a963c6621098n%40googlegroups.com.

[Brian Candler]

And as Axel's own reproducer shows, even having two threads reading and writing the same *variable* which points to a string can result in indeterminate behaviour, since a string is really a struct containing two parts (a pointer and a len).  You're not mutating the string itself, but you are updating a variable at the same time as it's being read.

In this regard, Go is no more thread-safe than C or C++, unless you make use of the concurrency features it provides (i.e. channels) instead of concurrently reading and writing the same variables.

[Axel Wagner]

On Mon, Mar 25, 2024 at 3:47 PM 'Brian Candler' via golang-nuts <golan...@googlegroups.com> wrote:

And as Axel's own reproducer shows, even having two threads reading and writing the same *variable* which points to a string can result in indeterminate behaviour, since a string is really a struct containing two parts (a pointer and a len).

I want to caution against this reasoning. The size of a variable is irrelevant. A data race on an `int` is still a data race.

It's the concurrent modification of a variable (in the sense of the spec, i.e. every field or array element is its own variable) that is the problem - not its type.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/a0294b1b-ce7a-4b68-b462-d63db8d58ce6n%40googlegroups.com.

[Axel Wagner]

Or to be perhaps more precise: This view can be useful to understand the observed behaviour: A string is two words which are updated non-atomically, hence one read might observe the write of the length separately from the write of the data.

But to determine the *correctness* of the program, any concurrent read/write of a variable must be synchronised, regardless of type or size.

[Brian Candler]

Absolutely!