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

cmd/cgo: missing pointer check for struct passed through a union type #15942

Closed
bcmills opened this issue Jun 2, 2016 · 9 comments

Comments

@bcmills
Copy link
Member

commented Jun 2, 2016

I was thinking some more about the examples in #14210. Here's a more realistic false-negative example that does "follow the rules", but still manages to leak a Go pointer into C undetected. It's not unlike what you might find in a program using, say, the SDL API.

It doesn't do any iffy casting in C, and its only iffy casting in Go is due to language support - this is exactly the code you end up writing if you have to deal with a C API with unions.

package main

/*
typedef enum {
  DIRECT,
  INDIRECT,
} kind;

typedef struct {
  kind k;
} header;

typedef struct {
  header hdr;
  int payload;
} direct;

typedef struct {
  header hdr;
  int *payload;
} indirect;

typedef union {
  header hdr;
  direct d;
  indirect i;
} either;

int get_payload(either *e) {
  switch (e->hdr.k) {
  case DIRECT:
    return e->d.payload;
  case INDIRECT:
    return *(e->i.payload);
  default:
    return -1;
  }
}
*/
import "C"

import (
    "fmt"
    "unsafe"
)

func main() {
    var escapee C.int = 42
    i := C.indirect{
        hdr:     C.header{k: C.INDIRECT},
        payload: &escapee,
    }

    escaped := C.get_payload((*C.either)(unsafe.Pointer(&i)))
    fmt.Println(escaped)
}

With go version go1.6.1 linux/amd64, this program outputs 42 and exits with code 0. It should instead panic with a cgo pointer violation.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

This looks impossible to fix. I'm open to suggestions.

As soon as the Go code imports unsafe, there are many cases that we can not detect. The pointer checking code was designed under the assumption that the Go code does not import unsafe.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jun 2, 2016
@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

I think this is just a bad interaction between the fix for #14387 and the fact that we use [n]byte to represent "opaque" C unions.

One fix would involve remembering details about C unions when we generate their opaque types: for example, we could check the corresponding Go types of each member of the union and record (somewhere?) whether any of the members may contain pointers.

An even simpler approximation would be to merely record the set of Go types that represent C unions and make (*Package).hasPointer return true for those types. (That would introduce some spurious checks for unions that do not contain pointers, but would otherwise make handling for unions closer to what we already do for void*.)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

The current code is careful to never erroneously check a non-pointer value as though it were a pointer, as that can cause false positives and, worse, unpredictable behavior when using PIE with ASLR. We might be able to use your suggestion if we recorded where a pointer might be in the C type, and also looked at the Go code to see whether the field was in fact initialized with an actual pointer. That could in principle catch errors like your original report, though of course it would hardly be foolproof.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

In this example, we do know that the value is a pointer: it's a *C.either, not a C.either. We should use essentially the same logic here as we would for unsafe.Pointer.

Changing

typedef union {
  header hdr;
  direct d;
  indirect i;
} either;

int get_payload(either *e) {
  switch (e->hdr.k) {
  case DIRECT:
    return e->d.payload;
  case INDIRECT:
    return *(e->i.payload);
  default:
    return -1;
  }
}

to

typedef void either;

int get_payload(either *e) {
  switch (((header*)e)->k) {
  case DIRECT:
    return ((direct*)e)->payload;
  case INDIRECT:
    return *(((indirect*)e)->payload);
  default:
    return -1;
  }
}

produces a program with identical semantics that does correctly trigger the cgo pointer check, without any change at all in the user-supplied Go code.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

We know that the argument to the function is a pointer, but passing a pointer is OK. What we don't know is whether that pointer points to a pointer. In the original example, it obviously does, because i.payload is a Go pointer. But in general we don't know whether the *C.either points to memory that contains a Go pointer. If it this happens to be the DIRECT case, we must avoid reporting a false positive if the d.payload field is an int that happens coincidentally to have the same value as a Go pointer.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

Wouldn't the runtime use the heap mask bits to determine which fields are pointers? I still don't see how handling a pointer-to-union is fundamentally any different than handling void*, which we already do for this case.

I guess it might be different because the runtime assumes that it knows the structure of types other than unsafe.Pointer? (Could we pass another parameter to runtime.cgoCheckPointer to say "you may think you know what type this thing is but really you don't"?)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

Ah, I see what you mean. The unsafe.Pointer handling is problematic in general because it doesn't obey the restrictions on field/slice pointers and therefore produces false positives as in #14210. But now I finally get it: if we pass a pointer to a C union, and if we can figure out that a C union might contain a pointer, then it would be reasonable to treat the union pointer as an unsafe.Pointer.

I'll change the milestone to 1.8.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.8, Unplanned Jun 2, 2016
@ianlancetaylor ianlancetaylor self-assigned this Jun 2, 2016
@quentinmit quentinmit added the NeedsFix label Oct 5, 2016
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

@ianlancetaylor, is this an easy fix?

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Nov 2, 2016
@gopherbot

This comment has been minimized.

Copy link

commented Nov 14, 2016

CL https://golang.org/cl/33237 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.