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

x/sys/unix: Listxattr on FreeBSD loses namespace info #54357

Closed
calmh opened this issue Aug 9, 2022 · 10 comments
Closed

x/sys/unix: Listxattr on FreeBSD loses namespace info #54357

calmh opened this issue Aug 9, 2022 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-FreeBSD
Milestone
Unreleased

Comments

@calmh
Copy link
Contributor

calmh commented Aug 9, 2022 •
edited

File extended attributes on FreeBSD (and NetBSD) have a namespace which is either user or system. This is represented by an enum in the syscall API. In the Go package, it's represented by prefixing the name with user. or system., like how it works in Linux. When listing extended attributes (at the syscall level) you need to specify the namespace. The Go package lists both the system and user namespaces and merges the results, but doesn't add any prefix so the returned list of attribute names isn't usable -- we don't know the namespace each came from.

A test case:

package fs_test

import (
	"golang.org/x/sys/unix"
	"os"
	"testing"
)

func TestFreeBSDXattr(t *testing.T) {
	fd, err := os.Create("testfile")
	if err != nil {
		t.Fatal(err)
	}
	fd.Close()

	if err := unix.Lsetxattr("testfile", "user.foo", []byte("value 1"), 0); err != nil {
		t.Fatal(err)
	}
	if err := unix.Lsetxattr("testfile", "system.bar", []byte("value 2"), 0); err != nil {
		t.Fatal(err)
	}

	buf := make([]byte, 1024)
	size, err := unix.Llistxattr("testfile", buf)
	if err != nil {
		t.Fatal(err)
	}
	buf = buf[:size]

	t.Logf("%q", buf)
}

which returns the following list of attributes (when run as a superuser who can set system namespace attributes):

=== RUN   TestFreeBSDXattr
    freebsd_xattr_test.go:30: "\x03foo\x03bar"
--- PASS: TestFreeBSDXattr (0.00s)

(that's length, name, length, name so "foo", "bar")

Actually trying to unix.Lgetxattr "foo" or "bar" returns ENOATTR, as these do not exist without a namespace.

Probably Llistxattr (and Listxattr) should synthesize the namespace prefix in the listing.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 9, 2022
@gopherbot gopherbot added this to the Unreleased milestone Aug 9, 2022
@dmgk dmgk added the OS-FreeBSD label Aug 9, 2022
@dmgk
Copy link
Member

dmgk commented Aug 9, 2022

cc @golang/freebsd

@dmgk
Copy link
Member

dmgk commented Aug 10, 2022

Modifying returned data in this way would probably break some existing code somewhere. Also, when called with zero sized buffer, unix.Listxattr() and friends return required buffer size, which would be too small if these function start adding namespace prefixes.

Perhaps we could add a set of new functions (something like ListxattrNS, FlistxattrNS and LlistxattrNS) that will accept an additional namespace parameter and will return only attributes scoped to that namespace.

@calmh
Copy link
Contributor Author

calmh commented Aug 10, 2022

Yeah... The only way I see existing code working at the moment is if it assumes that only user namespace attributes are returned, and then prefixes them by itself. But yeah, modifying the returned data certainly has pitfalls.

@paulzhol
Copy link
Member

There's a TestXattr test which explicitly strips just the user. prefix on FreeBSD.
https://github.com/golang/sys/blob/1c4a2a72c664c42691e45fe3a31510ec5ba13cfb/unix/xattr_test.go#L60-L63
@tklauser do you remember the reasoning behind it?

@paulzhol
Copy link
Member

Another data point. The FUSE implementation does prepend each attribute name with "namespace."
https://github.com/freebsd/freebsd-src/blob/514a191356c160ed8feb123138b92f3725e0fefc/sys/fs/fuse/fuse_vnops.c#L2746-L2755
https://github.com/freebsd/freebsd-src/blob/514a191356c160ed8feb123138b92f3725e0fefc/sys/fs/fuse/fuse_vnops.c#L2796-L2804

@paulzhol
Copy link
Member

I did some further digging.. there's more to it than just the namespace prefixes. The actual buffer formats differ.
NetBSD implements both the FreeBSD format (length prepended, attribute name), and the Linux format (NULL terminated strings) when encoding the attribute names into the buffer:

extattr_list_file (FreeBSD)
https://github.com/NetBSD/src/blob/ff7a4938d96318e1f23dbf1eca8dfdb7881d5c90/sys/kern/vfs_xattr.c#L708

llistxattr (Linux)
https://github.com/NetBSD/src/blob/ff7a4938d96318e1f23dbf1eca8dfdb7881d5c90/sys/kern/vfs_xattr.c#L1049

Both call extattr_list_vp with or without a flag EXTATTR_LIST_LENPREFIX which is handled directly by the VFS code:
https://github.com/NetBSD/src/blob/03827c661ad6ef834853ae37414efa9106760446/sys/ufs/ufs/ufs_extattr.c#L1230-L1246

I don't think we should be doing this kind of translation on the Go side. I had some really nasty corner cases doing something similar with the direntries for the FreeBSD 11 <-> 12 64bit inode translation shim.

I think we should just provide the full FreeBSD interface and that's it.

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 10, 2022
@gopherbot
Copy link

Change https://go.dev/cl/430215 mentions this issue: unix: add namespaced versions of Listxattr/Flistxattr/Llistxattr on *BSD

@dmgk
Copy link
Member

dmgk commented Sep 11, 2022

@calmh Would the approach in the above CL be helpful for your use case?

@calmh
Copy link
Contributor Author

calmh commented Sep 11, 2022

Yes, being able to specify the namespace solves my use case. 👍

However, I still think the existing functions should be either changed or perhaps deprecated, as the data they return can't be usefully interpreted, as we don't know from which namespace the returned attribute names come.

gopherbot pushed a commit to golang/sys that referenced this issue Sep 13, 2022
@dmgk @gopherbot
unix: add namespaced versions of Listxattr/Flistxattr/Llistxattr on *BSD
63ea559 
Current extended attributes API mixes together attributes from different
namespaces without a way to separate them. Add versions of *listxattr
functions that return attributes only from the specified namespace.

For golang/go#54357

Change-Id: I605d677dd2f2a17541bc02f3146a7509c69269c9
Reviewed-on: https://go-review.googlesource.com/c/sys/+/430215
Reviewed-by: Yuval Pavel Zholkover <paulzhol@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Dmitri Goutnik <dgoutnik@gmail.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Jenny Rakoczy <jenny@golang.org>
Reviewed-by: Jenny Rakoczy <jenny@golang.org>
@dmgk
Copy link
Member

dmgk commented Sep 16, 2022

I believe this issue was resolved by the above commit, closing.

@dmgk dmgk closed this as completed Sep 16, 2022
@golang golang locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-FreeBSD
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@calmh @paulzhol @dmgk @gopherbot @thanm