golang.org/x/sys and unsafe usage

[Reto]

Hi,
I'm a bit confused as to how the golang.org/x/sys package works.

From the documentation of unsafe.Pointer:
>(4) Conversion of a Pointer to a uintptr when calling syscall.Syscall.

> The Syscall functions in package syscall pass their uintptr arguments
> directly to the operating system, which then may, depending on the details
> of the call, reinterpret some of them as pointers.
> That is, the system call implementation is implicitly converting certain
> arguments back from uintptr to pointer.

> If a pointer argument must be converted to uintptr for use as an argument,
> that conversion must appear in the call expression itself

Now, as far as I can tell this forces non stdlib packages to adhere to exactly that.
As far as I can tell x/sys is just a common namespace for the go authors, but
as far as the compiler itself is concerned, that's a normal module not the stdlib.

Or is this a wrong assumption?

Reason I ask is because the code in x/sys clearly violates that rule.

in unix/ioctl.go there's

```
// IoctlSetPointerInt performs an ioctl operation which sets an
// integer value on fd, using the specified request number. The ioctl
// argument is called with a pointer to the integer value, rather than
// passing the integer value directly.
func IoctlSetPointerInt(fd int, req uint, value int) error {
v := int32(value)
return ioctl(fd, req, uintptr(unsafe.Pointer(&v)))
}
```

and the declaration of ioctl in zsyskall_linux.go:

```
func ioctl(fd int, req uint, arg uintptr) (err error) {
_, _, e1 := Syscall(SYS_IOCTL, uintptr(fd), uintptr(req), uintptr(arg))
if e1 != 0 {
err = errnoErr(e1)
}
return
}
```

Now, for starters ioctl includes a pointless conversion of a uintptr to a uintptr,
for the arg parameter can anyone tell me why?

Second (and this is my actual question), isn't that in violation of the unsafe
constraints cited above?

IoctlSetPointerInt clearly converts a unsafe.Pointer to a uintptr and *doesn't*
directly call syscall.Syscall.

Why is this valid?

Thanks in advance.

Regards,
Reto

[Axel Wagner]

Hi,

not an expert, but.

On Sat, Feb 27, 2021 at 11:19 PM Reto <re...@labrat.space> wrote:

Now, as far as I can tell this forces non stdlib packages to adhere to exactly that.
As far as I can tell x/sys is just a common namespace for the go authors, but
as far as the compiler itself is concerned, that's a normal module not the stdlib. 

Or is this a wrong assumption?

I think you are correct in that the compiler does not give `x/sys` special treatment.

However, `x/sys` is still somewhat special, in that, as its maintained by the Go team, they can make sure that `x/sys` stays up to date with regards to implementation details. That is, the documentation of the `unsafe` rules is more strict than it needs to be currently, to reserve the right to change the implementation in the future. And `x/sys` can use a more lenient interpretation, because if that implementation changes, it will be changed in lockstep.

Furthermore, I'm not sure that the compiler gives `syscall` special treatment either. At least a cursory grep through `cmd/compile` for "syscall" seems to give few hits. The most relevant seems

https://github.com/golang/go/blob/release-branch.go1.16/src/cmd/compile/internal/gc/esc.go#L386

which notably isn't restricted to `syscall` - it applies to any function, AIUI. This could basically mean that while the rule *states* "passing to syscall.Syscall", it actually exempts any call, because the special casing has not been done yet. But a future version *might*, so the rule still is more specific.

So, in short:

1. yes, technically `x/sys` is not `syscall`, but you should probably treat them as the same thing, functionally. Maybe the `unsafe` docs should reflect that.

2. the rules are more restricted than what the implementation currently allows.

3. `x/sys` can follow the rules-as-implemented, because if the implementation changes, so will `x/sys` - stdlib or not.

 

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/20210227221836.kgkn4xskg4xe7gsk%40feather.localdomain.

[Axel Wagner]

Oh, forgot to mention: The code I linked to has been moved or removed in master, vs. go 1.16. So the time of "when the implementation changes" might be "now" :)

[Ian Lance Taylor]

On Sat, Feb 27, 2021 at 5:12 PM 'Axel Wagner' via golang-nuts
<golan...@googlegroups.com> wrote:
>
> On Sat, Feb 27, 2021 at 11:19 PM Reto <re...@labrat.space> wrote:
>>
>> Now, as far as I can tell this forces non stdlib packages to adhere to exactly that.
>> As far as I can tell x/sys is just a common namespace for the go authors, but
>> as far as the compiler itself is concerned, that's a normal module not the stdlib.
>>
>>
>> Or is this a wrong assumption?
>
>
> I think you are correct in that the compiler does not give `x/sys` special treatment.
>
> However, `x/sys` is still somewhat special, in that, as its maintained by the Go team, they can make sure that `x/sys` stays up to date with regards to implementation details. That is, the documentation of the `unsafe` rules is more strict than it needs to be currently, to reserve the right to change the implementation in the future. And `x/sys` can use a more lenient interpretation, because if that implementation changes, it will be changed in lockstep.
>
> Furthermore, I'm not sure that the compiler gives `syscall` special treatment either. At least a cursory grep through `cmd/compile` for "syscall" seems to give few hits. The most relevant seems
> https://github.com/golang/go/blob/release-branch.go1.16/src/cmd/compile/internal/gc/esc.go#L386
> which notably isn't restricted to `syscall` - it applies to any function, AIUI. This could basically mean that while the rule *states* "passing to syscall.Syscall", it actually exempts any call, because the special casing has not been done yet. But a future version *might*, so the rule still is more specific.

Although rule 4 in the documentation of the unsafe package mentions
syscall.Syscall, the implementation isn't restricted to the syscall
package. It applies to any function that is written in assembly: that
is, a function that is declared in Go without a body.

We should perhaps tweak the docs to make that the rule.

Or, we could observe that golang.org/x/sys.Syscall actually just
forwards to syscall.Syscall, so they are in effect the same function.

>> Reason I ask is because the code in x/sys clearly violates that rule.
>>
>> in unix/ioctl.go there's
>>
>> ```
>> // IoctlSetPointerInt performs an ioctl operation which sets an
>> // integer value on fd, using the specified request number. The ioctl
>> // argument is called with a pointer to the integer value, rather than
>> // passing the integer value directly.
>> func IoctlSetPointerInt(fd int, req uint, value int) error {
>> v := int32(value)
>> return ioctl(fd, req, uintptr(unsafe.Pointer(&v)))
>> }
>> ```
>>
>> and the declaration of ioctl in zsyskall_linux.go:
>>
>> ```
>> func ioctl(fd int, req uint, arg uintptr) (err error) {
>> _, _, e1 := Syscall(SYS_IOCTL, uintptr(fd), uintptr(req), uintptr(arg))
>> if e1 != 0 {
>> err = errnoErr(e1)
>> }
>> return
>> }
>> ```

Now that you point this out, I'm actually not sure that this is OK. I
don't know what keeps v pinned during this call.

>> Now, for starters ioctl includes a pointless conversion of a uintptr to a uintptr,
>> for the arg parameter can anyone tell me why?

The functions in z*.go in x/sys are generated (by mksyscall.go). It's
easier to always write a conversion to uintptr then to try to decide
whether it is required or not.

>> Second (and this is my actual question), isn't that in violation of the unsafe
>> constraints cited above?
>>
>> IoctlSetPointerInt clearly converts a unsafe.Pointer to a uintptr and *doesn't*
>> directly call syscall.Syscall.
>>
>> Why is this valid?

I'm not sure it is.

Ian

[Reto]

Thanks to both of you.

On Sat, Feb 27, 2021 at 06:56:04PM -0800, Ian Lance Taylor wrote:
> Although rule 4 in the documentation of the unsafe package mentions
> syscall.Syscall, the implementation isn't restricted to the syscall
> package. It applies to any function that is written in assembly: that
> is, a function that is declared in Go without a body.
> We should perhaps tweak the docs to make that the rule.

I see, I was assuming that the assembly function of syscall.Syscall does some
(even darker) dark magic than normal assembly to guarantee that.
So a doc update would indeed be helpful.

> Now that you point this out, I'm actually not sure that this is OK. I
> don't know what keeps v pinned during this call.

> >> IoctlSetPointerInt clearly converts a unsafe.Pointer to a uintptr and *doesn't*
> >> directly call syscall.Syscall.
> >>
> >> Why is this valid?

> I'm not sure it is.

Hm, do you want me to open up a bug on GitHub?

The reason I originally asked is that I have a third party module that does
just the same and while writing the patch to actually adhere to the unsafe
constraints, I started reading the stdlib and it's kinda hard to argue for a
patch upstream if the go stdlib uses the exact same pattern ;)

Greetings,
Reto

[Ian Lance Taylor]

On Sun, Feb 28, 2021 at 1:06 AM Reto <re...@labrat.space> wrote:
>
> On Sat, Feb 27, 2021 at 06:56:04PM -0800, Ian Lance Taylor wrote:
> > Although rule 4 in the documentation of the unsafe package mentions
> > syscall.Syscall, the implementation isn't restricted to the syscall
> > package. It applies to any function that is written in assembly: that
> > is, a function that is declared in Go without a body.
> > We should perhaps tweak the docs to make that the rule.
>
> I see, I was assuming that the assembly function of syscall.Syscall does some
> (even darker) dark magic than normal assembly to guarantee that.
> So a doc update would indeed be helpful.
>
> > Now that you point this out, I'm actually not sure that this is OK. I
> > don't know what keeps v pinned during this call.
>
> > >> IoctlSetPointerInt clearly converts a unsafe.Pointer to a uintptr and *doesn't*
> > >> directly call syscall.Syscall.
> > >>
> > >> Why is this valid?
>
> > I'm not sure it is.
>
> Hm, do you want me to open up a bug on GitHub?

Sure. Thanks. We should probably resolve this one way or another.

Ian