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

Jump to bottom

syscall: Getdirentries on freebsd 11 is broken #31416

Closed
randall77 opened this issue Apr 11, 2019 · 8 comments
Closed

syscall: Getdirentries on freebsd 11 is broken #31416

randall77 opened this issue Apr 11, 2019 · 8 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-FreeBSD Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.19

Comments

@randall77
Copy link
Contributor

Getdirentries on freebsd 11 will silently drop files from the directory listing.

It converts old, smaller records into new, bigger records. It tries to account for the size increase by calling the syscall with a smaller buffer (1/4 the size). But it also has a lower bound on the minimum size, so the syscall buffer may in fact be no smaller than the original buffer. As a result, when copying from the syscall buffer to the output buffer, there may not be room for all the listed files. The code silently drops the files that don't fit.

The patch below will at least panic in that situation. The freebsd port needs to be fixed somehow. The new test TestDirentRepeat tickles this bug.

diff --git a/src/syscall/syscall_freebsd.go b/src/syscall/syscall_freebsd.go
index 87a27b1ff7..8fdb9cb144 100644
--- a/src/syscall/syscall_freebsd.go
+++ b/src/syscall/syscall_freebsd.go
@@ -392,6 +392,11 @@ func convertFromDirents11(buf []byte, old []byte) int {
                dstPos += int(dstDirent.Reclen)
                srcPos += int(srcDirent.Reclen)
        }
+       if srcPos < len(old) {
+               // We have old entries, but can't fit them in the new entry buffer.
+               // Bad!
+               panic("leftover entries")
+       }
 
        return dstPos
 }

@paulzhol

Split off from #31403
@bcmills
Copy link
Contributor

bcmills commented Apr 11, 2019

The freebsd builders are still red, so if this can't be fixed soon, please at least send a CL to skip the test on the affected builders.

@bcmills bcmills added Soon This needs to be done soon. (regressions, serious bugs, outages) Testing An issue that has been verified to require only test changes, not just a test failure. labels Apr 11, 2019
@bcmills bcmills changed the title Getdirentries on freebsd 11 is broken syscall: Getdirentries on freebsd 11 is broken Apr 11, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 11, 2019
@paulzhol paulzhol self-assigned this Apr 11, 2019
@gopherbot
Copy link

Change https://golang.org/cl/171719 mentions this issue: syscall: FreeBSD Getdirentries additional buf size validation

@gopherbot
Copy link

Change https://golang.org/cl/171818 mentions this issue: syscall: skip DirentRepeat test on freebsd

gopherbot pushed a commit that referenced this issue Apr 16, 2019
@randall77
syscall: skip DirentRepeat test on freebsd
8285c85 
Dirent doesn't work properly. Diable the test for now.

Update #31416

Change-Id: I34a8045598a9c303dcc754ce04da3c124f122d1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/171818
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bcmills bcmills removed the Soon This needs to be done soon. (regressions, serious bugs, outages) label Sep 10, 2019
@ayang64 ayang64 mentioned this issue Jan 6, 2022
Closed
@ayang64
Copy link
Member

ayang64 commented Jan 7, 2022

Since FreeBSD 11.x was removed from our list of builds, can we close this issue and remove the skipped portion of the test? I couldn't reproduce under FreeBSD 14.

@bcmills bcmills modified the milestones: Backlog, Go1.19 Jan 7, 2022
@bcmills bcmills added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jan 7, 2022
@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2022

Since the tree is frozen, a change to unskip the test at this point wouldn't get very many runs before the release, which would make it more likely that we miss a test regression if it turns out not to be fixed after all.

Let's aim to remove the skip when the tree opens for 1.19, so that we can have the whole 1.19 cycle to verify that it is actually fixed.

@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

Change https://go.dev/cl/393618 mentions this issue: syscall: unskip TestDirentRepeat on freebsd

gopherbot pushed a commit that referenced this issue Mar 18, 2022
@tklauser
syscall: unskip TestDirentRepeat on freebsd
3d19e8d 
TestDirentRepeat fails on FreeBSD 11, but seems to pass on newer
versions. Go 1.18 is the last release to support FreeBSD 11 per
https://golang.org/doc/go1.18#freebsd and there are no FreeBSD 11
builders anymore. Thus unskip TestDirentRepeat to verify the issue is
indeed fixed on FreeBSD 12 and later.

For #31416

Change-Id: I189ef06719ff830ffe2e402c74a75874c9e5b97b
Reviewed-on: https://go-review.googlesource.com/c/go/+/393618
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@tklauser
Copy link
Member

There don't seem to be any reports about TestDirentRepeat failing on any of the freebsd builder since https://go.dev/cl/393618 re-enabling the test was submitted 2 months ago. Closing this issue.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-FreeBSD Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
6 participants
@tklauser @ayang64 @paulzhol @bcmills @randall77 @gopherbot