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

syscall: unimplemented EpollWait fails to return an error on android/arm64 #35479

Closed
jakubgs opened this issue Nov 9, 2019 · 9 comments
Closed

syscall: unimplemented EpollWait fails to return an error on android/arm64 #35479

jakubgs opened this issue Nov 9, 2019 · 9 comments
Milestone

Comments

@jakubgs
Copy link

@jakubgs jakubgs commented Nov 9, 2019

What version of Go are you using?

$ go version
go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

Yes. Also tested it with go1.13.1.

What operating system and processor architecture are you using?

I built the binary for Android with GOARCH=arm64 on Linux with GOHOSTARCH=amd64.

What did you do?

I've built this test application that does TCP pinging using the syscall package, and found out that the EpollWait call returns an EpollEvent with only Events attribute set. The Fd attribute is always set to 0 when run on arm64.

Here's the code:
https://github.com/status-im/infra-utils/blob/113e4e50/tcp-ping/main.go

I ran it on my android device(OnePlus 6 with Android 9) with the following results:

OnePlus6:/data # ./tcp-ping-arm64                                                                                                    
TCP_PING:2019/11/09 15:43:52 PINGING: 8.8.8.8:53
TCP_PING:2019/11/09 15:43:52 Socket FD: 3
TCP_PING:2019/11/09 15:43:53 Epoll FD: 4
TCP_PING:2019/11/09 15:43:53 EpollWait #: 1
TCP_PING:2019/11/09 15:43:53 SO_ERROR: 0
TCP_PING:2019/11/09 15:43:53 EpollEvent: {4 0 0}
TCP_PING:2019/11/09 15:43:53 SUCCESS

As you can see the EpollEvent returned by EpollWait has the 2nd attribute(Fd) set to 0.

I have fixed it by switching from using syscall package to x/sys/unix:
https://github.com/status-im/infra-utils/blob/ec20ad39/tcp-ping/main.go

What did you expect to see?

I expected the event returned from EpollWait to include the number of the file descriptor for which the event was triggered.

What did you see instead?

File descriptor being set to 0, making the EpollWait call useless when used with multiple file descriptors.

Notes

I understand that the syscall was frozen as of Go 1.3 and that it won't be fixed. My intention in creating this issue is maybe helping someone else avoid hours of research by finding this issue.

I was also curious about why exactly the value for the file descriptor is always 0.

@bcmills bcmills changed the title EpollWait returns EpollEvent without set file descriptor on Android syscall: EpollWait returns EpollEvent without set file descriptor on Android Nov 10, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 10, 2019

Your program is missing a check of the err variable bound from the call (see also #23142).

If you check the error, I suspect you will find that that system call is not implemented at all (#25813).

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 10, 2019

Duplicate of #25813

@bcmills bcmills marked this as a duplicate of #23142 Nov 10, 2019
@bcmills bcmills closed this Nov 10, 2019
@bcmills bcmills marked this as a duplicate of #25813 Nov 10, 2019
@jakubgs

This comment has been minimized.

Copy link
Author

@jakubgs jakubgs commented Nov 10, 2019

Thanks for the explanation, appreciate it.

The syscall package should not be used.

Oh, now that I look at it there is a deprecation warning:

Deprecated: this package is locked down. Callers should use the corresponding package in the golang.org/x/sys repository instead.
https://golang.org/pkg/syscall/

But I must have missed it every single time I was browsing that page. Maybe making it bold or moving it to the top of the description would be a good idea.

@jakubgs

This comment has been minimized.

Copy link
Author

@jakubgs jakubgs commented Nov 10, 2019

Your program is missing a check of the err variable bound from the call.

That was just a proof-of-concept code, I have other examples that do check it. I just verified this on my simple test code by adding a check for error from sycall.EpollWait() and there is no error. If it's not implemented it should return an error and not a partially filled out event.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 11, 2019

If you have a concrete example that demonstrates a nil error in conjunction with missing data, that aspect of it is probably worth fixing, at least.

@jakubgs

This comment has been minimized.

Copy link
Author

@jakubgs jakubgs commented Nov 11, 2019

Well, yes, I modified that test program: status-im/infra-utils@28aea97 & status-im/infra-utils@68a3244
And ran it on arm64:

 $ make go-arm64
GOARCH=arm64 go build -o tcp-ping-arm64 main.go

 $ adb push tcp-ping-arm64 /sdcard/
tcp-ping-arm64: 1 file pushed. 27.8 MB/s (3132491 bytes in 0.108s)

 $ adb shell
OnePlus6:/ $ su
OnePlus6:/ # cd /data
OnePlus6:/data # mv /sdcard/tcp-ping-arm64 ./
OnePlus6:/data # chmod +x tcp-ping-arm64
OnePlus6:/data # ./tcp-ping-arm64
TCP_PING:2019/11/11 14:29:14 PINGING: 8.8.8.8:53
TCP_PING:2019/11/11 14:29:14 Socket FD: 3
TCP_PING:2019/11/11 14:29:14 Epoll FD: 4
TCP_PING:2019/11/11 14:29:14 EpollWait err: <nil>
TCP_PING:2019/11/11 14:29:14 EpollWait #: 1
TCP_PING:2019/11/11 14:29:14 SO_ERROR: 0
TCP_PING:2019/11/11 14:29:14 EpollEvent: {4 0 3 0}
TCP_PING:2019/11/11 14:29:14 SUCCESS

See as EpollWait err is <nil> and EpollEvent has 2nd field set to 0. It's clearly broken.

@bcmills bcmills changed the title syscall: EpollWait returns EpollEvent without set file descriptor on Android syscall: unimplemented EpollWait fails to return an error on android/arm64 Nov 11, 2019
@bcmills bcmills reopened this Nov 11, 2019
@bcmills bcmills added this to the Backlog milestone Nov 11, 2019
@tklauser

This comment has been minimized.

Copy link
Member

@tklauser tklauser commented Nov 12, 2019

The implementation of syscall.EpollWait is exactly the same as unix.EpollWait. I'd rather suspect a problem with the definition of EpollEvent on arm64 missing padding before its Fd member. This was fixed in x/sys/unix (https://golang.org/cl/21971) but not in syscall. I'll send a CL.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2019

Change https://golang.org/cl/206838 mentions this issue: syscall: fix epoll_event padding on linux/arm64

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2019

Change https://golang.org/cl/206858 mentions this issue: unix: test returned fd in TestEpoll

@gopherbot gopherbot closed this in 405a2f2 Nov 12, 2019
gopherbot pushed a commit to golang/sys that referenced this issue Nov 12, 2019
Check the fd returned in EpollEvent to detect potential padding issues.

Also, fail the test if the number of events mismatches.

Updates golang/go#35479

Change-Id: I39f856ca2c336e849876a33acffb70b82aa83c3f
Reviewed-on: https://go-review.googlesource.com/c/sys/+/206858
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.