cavaliergopher / grab

A download manager package for Go
BSD 3-Clause "New" or "Revised" License
1.39k stars 150 forks source link

nil pointer dereference @ response.go:81 #4

Closed seniorGolang closed 8 years ago

seniorGolang commented 8 years ago

After update:

panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x4 pc=0xc6974]

goroutine 34 [running]: panic(0x346eb0, 0x1070a038) /root/.gvm/gos/go1.6/src/runtime/panic.go:464 +0x330 sync/atomic.loadUint64(0x1080a1d4, 0x0, 0x0) /root/.gvm/gos/go1.6/src/sync/atomic/64bit_arm.go:10 +0x54 github.com/cavaliercoder/grab.(_Response).BytesTransferred(0x1080a180, 0x4, 0x386878) /root/.gvm/pkgsets/go1.6/global/src/github.com/cavaliercoder/grab/response.go:81 +0x40 github.com/cavaliercoder/grab.(_Client).do(0x107b79c0, 0x107f80f0, 0x0, 0x0, 0x0) /root/.gvm/pkgsets/go1.6/global/src/github.com/cavaliercoder/grab/client.go:222 +0x3c0 github.com/cavaliercoder/grab.(_Client).DoAsync.func1(0x107b79c0, 0x107f80f0, 0x10802380) /root/.gvm/pkgsets/go1.6/global/src/github.com/cavaliercoder/grab/client.go:94 +0x24 created by github.com/cavaliercoder/grab.(_Client).DoAsync /root/.gvm/pkgsets/go1.6/global/src/github.com/cavaliercoder/grab/client.go:102 +0x60

cavaliercoder commented 8 years ago

I'm having trouble reproducing this problem. Could you please provide some additional context on how to reproduce this problem?

The only pointer referenced in Response.BytesTransferred is c, which is a pointer to the Response itself, so the response is possibly be nil.

The previous call on the stack, client.go:222, calls resp.BytesTransferred() but it initialized resp itself and I don't see anywhere it could have been replace by nil in that function.

It is possible the nil pointer dereference actually occurred in the stdlib /root/.gvm/gos/go1.6/src/sync/atomic/64bit_arm.go:10. I see you are using Go 1.6. Could you please try and reproduce the issue with 1.5?

seniorGolang commented 8 years ago

This problem is present under the ARM. Example for Raspberry PI.

panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x4 pc=0xc4130]

goroutine 36 [running]: sync/atomic.loadUint64(0x106d8294, 0x0, 0x0) /root/.gvm/gos/go1.5.3/src/sync/atomic/64bit_arm.go:10 +0x54 github.com/cavaliercoder/grab.(_Response).BytesTransferred(0x106d8240, 0x4, 0x32ea50) /root/.gvm/pkgsets/go1.5.3/global/src/github.com/cavaliercoder/grab/response.go:81 +0x40 github.com/cavaliercoder/grab.(_Client).do(0x106709c0, 0x106d40f0, 0x10666540, 0x0, 0x0) /root/.gvm/pkgsets/go1.5.3/global/src/github.com/cavaliercoder/grab/client.go:222 +0x3c0 github.com/cavaliercoder/grab.(_Client).DoAsync.func1(0x106709c0, 0x106d40f0, 0x106c44c0) /root/.gvm/pkgsets/go1.5.3/global/src/github.com/cavaliercoder/grab/client.go:94 +0x24 created by github.com/cavaliercoder/grab.(_Client).DoAsync /root/.gvm/pkgsets/go1.5.3/global/src/github.com/cavaliercoder/grab/client.go:102 +0x60

cavaliercoder commented 8 years ago

That's why I can't reproduce it! There seems to be a known issue with alignment on ARM.

Please see:

I'm not entirely sure how to manipulate alignment in Go... do you have any advice?

luxas commented 8 years ago

I think you should move doneFlag to the top of the struct

cavaliercoder commented 8 years ago

Could you please test this? I don't have access to any ARM processors to confirm the solution with. First PR wins.

Adding one Raspberry Pi to cart...

seniorGolang commented 8 years ago

Move bytesTransferred to the top of the struct Response solve the problem.

cavaliercoder commented 8 years ago

Replicated the issue on Win10 with Go v1.5 and v.16 by setting GOARCH=386 (as per #5)

Elite commented 8 years ago

So do we have a fix?

cavaliercoder commented 8 years ago

When I have a moment, I'm going to experiment with using mutexes instead of the atomic updates. I don't think that rearranging the alignment of the Response struct is the right fix as it will be difficult to lay it out in a clear ordered fashion, but more importantly, future changes to Response could break it again inadvertently.

ohenepee commented 8 years ago

Got the same error on Windows XP Pro SP3

So I'm anticipating that this problem has something to do with non-64-bit Windows OS builds