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

premature garbage collection on armv7 not respecting runtime.KeepAlive? #28710

Closed
thinkski opened this issue Nov 10, 2018 · 2 comments
Closed

premature garbage collection on armv7 not respecting runtime.KeepAlive? #28710

thinkski opened this issue Nov 10, 2018 · 2 comments

Comments

@thinkski
Copy link

@thinkski thinkski commented Nov 10, 2018

What did you do?

Wrote a small program to issue an ioctl system call, passing a pointer to a struct to the syscall via uintptr(unsafe.Pointer(&pf)). Cross-compiled to armv7:

GOARM=7 GOARCH=arm GOOS=linux go build demo.go

Here is the full code: https://play.golang.org/p/elPL-IUS989
As is, it works fine. However, removing the fmt.Println(&pf) on line 46, causes a segmentation fault on the target (Raspberry Pi 3B+, for those that wish to replicate):

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x0]

goroutine 1 [running]:
runtime: unexpected return pc for main.main called from 0x0
stack: frame={sp:0x14267c4, fp:0x14267e8} stack=[0x1426000,0x1426800)
01426744:  0142672c  000337e4 <runtime.panicmem+84>  0142677c  000a3314
01426754:  0144401c  00000004  00000004  0003619c <runtime.main+228>
01426764:  0006234c <syscall.Syscall+104>  014267c4  014000e0  00000002
01426774:  01444000  014000f4  00000000  0008d8e0
01426784:  0140c0a8  00000000  00000000  000477c4 <runtime.sigpanic+236>
01426794:  0008ffb8  0010ae20  00077024 <main.main+152>  014000e0
014267a4:  00000001  00000000  00000000  00000000
014267b4:  014000e0  00000000  00000000  00077024 <main.main+152>
014267c4: <00000000  00000000  00000000  00000000
014267d4:  00000000  00000000  00000000  00000000
014267e4:  00000000 >00000000  00000000  00000000
014267f4:  00000000  00000000  00000000

What did you expect to see?

Expected to see no segmentation fault without the fmt.Println() on line 46.

What did you see instead?

Without the fmt.Println() on line 46, a segmentation fault occurs, perhaps due to garbage collection of the struct instance before the syscall is executed? However, runtime.KeepAlive(&pf) should prevent garbage collection before the syscall completes.

System details

go version go1.11.2 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/chris/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/chris/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.11.2 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.11.2
uname -v: Darwin Kernel Version 17.7.0: Wed Oct 10 23:06:14 PDT 2018; root:xnu-4570.71.13~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13.6
BuildVersion:	17G3025
lldb --version: lldb-1000.11.38.2
  Swift-4.2
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 10, 2018

It's not a problem with runtime.KeepAlive. You don't need to call runtime.KeepAlive for pointers passed to unix.Syscall via an explicit conversion to uintptr in the call to unix.Syscall itself, as you are doing.

The problem seems more likely to be related to the fact that your definition of v4l2_pix_format is too small. When I look at the GNU/Linux header files, I see additional flags, xx_enc, quantization, and xfer_func fields that you aren't mentioning in your definition. When you remove the fmt.Println statement, the struct does not escape, so you are clobbering the stack. With the fmt.Println statement, the struct escapes, so although you are clobbering the heap nothing happens since the program exits soon after.

Closing since I don't think this is a problem with Go. Please comment if you disagree.

@thinkski

This comment has been minimized.

Copy link
Author

@thinkski thinkski commented Nov 11, 2018

@ianlancetaylor Thank you for taking a look at this. The /usr/include/linux/videodev2.h file on the target does not include these fields in the struct:

/usr/include/linux/videodev2.h:

...
struct v4l2_format {
        __u32    type;
        union {
                struct v4l2_pix_format          pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
                struct v4l2_pix_format_mplane   pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
                struct v4l2_window              win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
                struct v4l2_vbi_format          vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
                struct v4l2_sliced_vbi_format   sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
                struct v4l2_sdr_format          sdr;     /* V4L2_BUF_TYPE_SDR_CAPTURE */
                __u8    raw_data[200];                   /* user-defined */
        } fmt;
};
...
struct v4l2_pix_format {
        __u32                   width;
        __u32                   height;
        __u32                   pixelformat;
        __u32                   field;          /* enum v4l2_field */
        __u32                   bytesperline;   /* for padding, zero if unused */
        __u32                   sizeimage;
        __u32                   colorspace;     /* enum v4l2_colorspace */
        __u32                   priv;           /* private data, depends on pixelformat */
};
...

Target's uname -a:

Linux raspberrypi 4.14.72-v7+ #1146 SMP Wed Sep 26 16:58:28 BST 2018 armv7l GNU/Linux

Nevertheless, tried adding the additional fields to the Go struct and ran into the same issue:
https://play.golang.org/p/CZJuH2k1k1i

However, adding an additional padding [37]uint32 on top of that did the trick (36 did not). Looks like it's like you say, the stack is getting clobbered -- verified with -gcflags '-m' that 46:26: setPixelFormat &pf does not escape. The sizeof(struct v4l2_format) is 204 bytes, so perhaps the space in the union is assumed by the v4l2 driver to be available for use as temporary storage space. Unfortunately, the driver is closed source. Would you recommend using cgo in this case?

Thanks again.

@golang golang locked and limited conversation to collaborators Nov 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.