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.

Sign up for GitHub

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

Jump to bottom

Fix ptsname() for big-endian architectures (again) #51

Merged
merged 2 commits into from
Apr 8, 2021
Merged

Fix ptsname() for big-endian architectures (again) #51

merged 2 commits into from
Apr 8, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Apr 6, 2021 •
edited
Loading

This reverts commit f1b333f (Use Ioctl{SetPointerInt,GetInt} from
golang.org/x/sys/unix on linux), changing the code to what it was after
commit dbd69c5. This fixes a blocker bug on s390 [1].

The original description from commit dbd69c5 follows:

On big-endian architectures unix.IoctlGetInt() leads to a wrong result
because a 32 bit value is stored into a 64 bit buffer. When dereferencing
the result is left shifted by 32. Without this patch ptsname() returns
a wrong path from the second pty onwards.
To protect syscalls against re-arranging by the GC the conversion from
unsafe.Pointer to uintptr must occur in the Syscall expression itself.
See the documentation of the unsafe package for details.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1941815

This is a regression in v1.0.1, so ideally we need a new 1.0.2 release after merging this.

@kolyshkin
Fix ptsname() for big-endian architectures (again)
a261251 
This reverts commit f1b333f (Use Ioctl{SetPointerInt,GetInt} from
golang.org/x/sys/unix on linux), changing the code to what it was after
commit dbd69c5. This fixes a blocker bug on s390.

The original description from commit dbd69c5 follows:

    On big-endian architectures unix.IoctlGetInt() leads to a wrong result
    because a 32 bit value is stored into a 64 bit buffer. When dereferencing
    the result is left shifted by 32. Without this patch ptsname() returns
    a wrong path from the second pty onwards.
    To protect syscalls against re-arranging by the GC the conversion from
    unsafe.Pointer to uintptr must occur in the Syscall expression itself.
    See the documentation of the unsafe package for details.

Cc: Peter Morjan <[email protected]>
Cc: Tobias Klauser <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

@estesp @tklauser @pmorjan PTAL 🙏

@kolyshkin
Copy link
Contributor Author

It would be cool to have this tested on s390, alas with Travis gone I don't know how.

@kolyshkin kolyshkin mentioned this pull request Apr 6, 2021
Closed
tklauser
tklauser approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for messing this up and thanks for the fix.

@Prashanth684
Copy link

was able to test this patch with the unit tests on an s390x system and it works.

Issue before testing with this patch:

[psundara@rock-kvmlp-1 console]$ go test -v ./...
=== RUN   TestEpollConsole
    console_linux_test.go:38: open /dev/pts/4294967296: no such file or directory
--- FAIL: TestEpollConsole (0.00s)
=== RUN   TestWinSize
--- PASS: TestWinSize (0.00s)
=== RUN   TestConsolePty
    console_test.go:66: open /dev/pts/4294967296: no such file or directory
--- FAIL: TestConsolePty (0.00s)
FAIL
FAIL	github.com/containerd/console	0.008s
FAIL

After applying patch:

[psundara@rock-kvmlp-1 console]$ go test -v ./...
=== RUN   TestEpollConsole
--- PASS: TestEpollConsole (0.04s)
=== RUN   TestWinSize
--- PASS: TestWinSize (0.00s)
=== RUN   TestConsolePty
--- PASS: TestConsolePty (0.00s)
PASS
ok  	github.com/containerd/console	0.050s

@AkihiroSuda
Copy link
Member

Could you add comment lines in the source to explain the original issue, LGTM then

@kolyshkin
tc_linux.go: add a warning to not change the code
073d165 
... with a pointer to a detailed explanation.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Could you add comment lines in the source to explain the original issue, LGTM then

Added a second commit. The full explanation is long, so I just warn and refer to an earlier commit which explains it.

PTAL @AkihiroSuda @estesp @dmcgowan

@kolyshkin
Copy link
Contributor Author

@estesp @dims @mikebrow @fuweid @mxpv PTAL 🙏

@kolyshkin
Copy link
Contributor Author

Ideally we need an 1.0.2 release after merging this, as this bug is a regression in 1.0.1.

cpuguy83
cpuguy83 approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mikebrow
mikebrow approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael crosbymichael merged commit 65c9061 into containerd:master Apr 8, 2021
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Apr 8, 2021
@kolyshkin
vendor: bump containerd/console to 1.0.2-pre
6981db7 
This is to include containerd/console#51

Fixes: opencontainers#2896

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin mentioned this pull request Apr 8, 2021
Merged
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Apr 8, 2021
@kolyshkin
vendor: bump containerd/console to 1.0.2-pre
d9ea176 
This is to include containerd/console#51

Fixes: opencontainers#2896

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Apr 12, 2021
@kolyshkin
vendor: bump containerd/console to 1.0.2
b7c315a 
This is to include containerd/console#51.

Fixes: opencontainers#2896

Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin added a commit to kolyshkin/runc-1 that referenced this pull request Apr 15, 2021
@kolyshkin
vendor: bump containerd/console to 1.0.2
c7dc0cb 
This is to include containerd/console#51.

Fixes: opencontainers/runc#2896

Signed-off-by: Kir Kolyshkin <[email protected]>
This was referenced Apr 15, 2021
Merged
Closed
@kolyshkin kolyshkin mentioned this pull request Jul 23, 2021
Closed
anthonyfok added a commit to anthonyfok/go-internal that referenced this pull request Feb 16, 2024
@anthonyfok
testscript: fix ptyName() returning /dev/pts/4294967296 on s390x
86ed44b 
Use uint32 instead of uint (64-bit in Go on s390x) to store the return
value of the TIOCGPTN syscall.  This is to avoid the 32-bit value from
being stored into a 64-bit buffer and get left-shifted by 32 when
dereferencing, turning what should be /dev/pts/1 to /dev/pts/4294967296
on big-endian architectures such as s390x.

Special thanks to the explanation and a similar bug fix provided at
containerd/console#51
@anthonyfok anthonyfok mentioned this pull request Feb 16, 2024
Merged
mvdan pushed a commit to rogpeppe/go-internal that referenced this pull request Feb 16, 2024
@anthonyfok @mvdan
testscript: fix ptyName() returning /dev/pts/4294967296 on s390x
8e66eb9 
Use uint32 instead of uint (64-bit in Go on s390x) to store the return
value of the TIOCGPTN syscall.  This is to avoid the 32-bit value from
being stored into a 64-bit buffer and get left-shifted by 32 when
dereferencing, turning what should be /dev/pts/1 to /dev/pts/4294967296
on big-endian architectures such as s390x.

Special thanks to the explanation and a similar bug fix provided at
containerd/console#51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikebrow mikebrow mikebrow approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@crosbymichael crosbymichael crosbymichael approved these changes

@tklauser tklauser tklauser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kolyshkin @Prashanth684 @AkihiroSuda @tklauser @crosbymichael @cpuguy83 @mikebrow