Skip to content

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

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
kolyshkin:fix-bi
Apr 8, 2021
Merged

Fix ptsname() for big-endian architectures (again)#51
crosbymichael merged 2 commits intocontainerd:masterfrom
kolyshkin:fix-bi

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 <peter.morjan@de.ibm.com>
Cc: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 6, 2021

@estesp @tklauser @pmorjan PTAL 🙏

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 6, 2021

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

Prashanth684 commented Apr 6, 2021

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

AkihiroSuda commented Apr 7, 2021

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 <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 7, 2021

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

kolyshkin commented Apr 8, 2021

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

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Apr 8, 2021

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 <kolyshkin@gmail.com>
@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 <kolyshkin@gmail.com>
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 <kolyshkin@gmail.com>
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 <kolyshkin@gmail.com>
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

@crosbymichael crosbymichael crosbymichael approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

+2 more reviewers

@tklauser tklauser tklauser approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Reviewers whose approvals may not affect merge requirements

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