Page MenuHomePhabricator

Add an instruction to avoid cgo compilation error from Go 1.9.4
ClosedPublic

Authored by rhysd on Feb 8 2018, 6:17 AM.

Details

Summary

Because of CVE-2018-6574, some compiler options and linker options are restricted to prevent arbitrary code execution.

https://github.com/golang/go/issues/23672

By this change, building a Go code with LLVM Go bindings causes a compilation error as follows.

go build llvm.org/llvm/bindings/go/llvm: invalid flag in #cgo LDFLAGS: -Wl,-headerpad_max_install_names

llvm-go tool generates cgo LDFLAGS directive from llvm-config --ldflags and it contains -Wl,option options. But -Wl,option is banned by default. To avoid this problem, we need to set $CGO_LDFLAGS_ALLOW environment variable to notify a compiler that the flags should be allowed.

$ export CGO_LDFLAGS_ALLOW='-Wl,(-search_paths_first|-headerpad_max_install_names)'

I added an instruction about how to avoid the compilation error and build the code properly.

Diff Detail

Repository
rL LLVM

Event Timeline

rhysd created this revision.Feb 8 2018, 6:17 AM
echristo accepted this revision.Feb 11 2018, 11:32 AM
This revision is now accepted and ready to land.Feb 11 2018, 11:32 AM

(If you need someone to commit this let me know)

@echristo Thank you for your review!

(If you need someone to commit this let me know)

Actually I requested some people to review this patch based on git log. So I'm happy with a review from one of them.

@echristo Thank you for your review!

(If you need someone to commit this let me know)

Actually I requested some people to review this patch based on git log. So I'm happy with a review from one of them.

I've also approved it... so you can commit if you'd like. If you don't have commit access I'll do it for you.

Is this a candidate for the 6.0.0 release?

Is this a candidate for the 6.0.0 release?

Yes, it should be brought in there too.

rhysd added a comment.Feb 13 2018, 3:14 PM

@echristo

I've also approved it... so you can commit if you'd like. If you don't have commit access I'll do it for you.

I'm not a committer of LLVM project. So could you please commit this patch?

Another thought here is getting that flag put into the default set that's allowed in go?

rhysd added a comment.EditedFeb 13 2018, 6:37 PM

Another thought here is getting that flag put into the default set that's allowed in go?

Not allowed. The white list must be set by user for now by the environment variable.

Improving the default white list of options is now ongoing. But I'm not sure whether or not -Wl,-search_paths_first and -Wl,-headerpad_max_install_names will be in the list.

https://github.com/golang/go/issues/23749

hans added a subscriber: hans.Feb 19 2018, 7:28 AM

If we want this for 6.0, it needs to land real soon.
Eric, do you have concerns or do you want to commit it?

Another thought here is getting that flag put into the default set that's allowed in go?

Not allowed. The white list must be set by user for now by the environment variable.

Improving the default white list of options is now ongoing. But I'm not sure whether or not -Wl,-search_paths_first and -Wl,-headerpad_max_install_names will be in the list.

https://github.com/golang/go/issues/23749

It looks like they've been set into the default set here: https://go-review.googlesource.com/c/go/+/94676

Do we still need this?

-eric

rhysd added a comment.Feb 21 2018, 6:56 PM

It looks like they've been set into the default set here: https://go-review.googlesource.com/c/go/+/94676

yeah, because I suggested that they are listed in the whitelist in the issue thread.

For Go 1.10, we no longer need this. But I want to ensure that the whitelist fix is also included in 1.9.5.
After that, I think we no longer need this completely.

rhysd added a comment.Feb 21 2018, 7:02 PM

If we want this for 6.0, it needs to land real soon.
Eric, do you have concerns or do you want to commit it?

Ah, I missed to catch this point.
The latest Go version (1.10) fixed this. So I don't think this patch is urgent and can skip 6.0.

I spoke with Ian about it and he said the whitelist fix is also targeted at 1.9.5.

That said, come to think of it, this is just documentation and is fairly useful so I'll commit anyhow in a bit and ask Hans to backport.

Thanks!

Committed thusly:

echristo@athyra ~/s/llvm> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
M bindings/go/README.txt
Committed r325946

echristo closed this revision.Feb 23 2018, 12:14 PM

Eric, thank you for your confirming. I understood.

hans added a comment.Feb 26 2018, 2:06 AM

Committed thusly:

echristo@athyra ~/s/llvm> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...

	M	bindings/go/README.txt

Committed r325946

Merged to 6.0.0 in r326076.