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/go: build cache does not check #included headers for changes #24355

Open
FlorianUekermann opened this Issue Mar 12, 2018 · 14 comments

Comments

Projects
None yet
9 participants
@FlorianUekermann
Contributor

FlorianUekermann commented Mar 12, 2018

go 1.10 linux/amd64

The go build and go test commands don't rebuild packages if included cgo files changed. I guess a solution would be to run the preprocessor first or disable caching for packages that use cgo altogether.

@andybons andybons added this to the Go1.11 milestone Mar 13, 2018

@andybons andybons changed the title from cgo: false build cache hits if cgo dependencies changed to cmd/cgo: false build cache hits if cgo dependencies changed Mar 13, 2018

@andybons

This comment has been minimized.

Member

andybons commented Mar 13, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 27, 2018

Can you show us a small standalone example? I'm not clear on what you mean by "included cgo files". Do you mean explicitly files included using #include?

@ianlancetaylor ianlancetaylor changed the title from cmd/cgo: false build cache hits if cgo dependencies changed to cmd/go: false build cache hits if cgo dependencies changed Mar 27, 2018

@FlorianUekermann

This comment has been minimized.

Contributor

FlorianUekermann commented Mar 27, 2018

Do you mean explicitly files included using #include?

Yes.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 27, 2018

One possibility would be to use -MD when running the C compiler, and add the listed files to the hash.

@FlorianUekermann

This comment has been minimized.

Contributor

FlorianUekermann commented Mar 27, 2018

use -MD when running the C compiler, and add the listed files to the hash.

Just to make sure I'm following. Did you mean add the contents of all listed files or just the list?

I had running with -E and hashing the output in mind, but I guess your idea may be more efficient (is it?).

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 27, 2018

I meant the contents of the listed files, as in cache.FileHash in cmd/go/internal/cache/hash.go. The idea is to be able to use the cache to detect whether we can skip running the compiler. If we use -E then we have to run the compiler anyhow to see whether the cache is up to date.

@Xim8

This comment has been minimized.

Xim8 commented Apr 9, 2018

This is also affecting me in a more general sense that with 1.10 there is no way to model the build dependency to statically linked libraries anymore. If the library changes, the cached files are still reused and I silently end up using an older version of the library unless I do go clean -cache -i <package> before the build. With go versions previous to 1.10 I had cmake touching my cgo wrappers to have them rebuilt.

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 18, 2018

I think this is basically working as expected.
If you change the underlying C code, or you change the compiler,
then you have to rebuild with -a. I'll leave it open in case there is
a simple fix but I don't think there is.

@rsc rsc changed the title from cmd/go: false build cache hits if cgo dependencies changed to cmd/go: build cache does not check #included headers for changes Apr 18, 2018

@navytux

This comment has been minimized.

Contributor

navytux commented Apr 18, 2018

There is a way to make it work with the help of gcc -MD & friends:

(hello.h)

#define WORLDNUM        3

(hello.c)

#include <stdio.h>
#include "hello.h"

int main() {
        printf("Hello world (%d)!\n", WORLDNUM);
}
$ gcc -c -MMD -MT cdeps hello.c
$ cat hello.d 
cdeps: hello.c hello.h
$ gcc -c -MD -MT cdeps hello.c 
$ cat hello.d 
cdeps: hello.c /usr/include/stdc-predef.h /usr/include/stdio.h \
 /usr/include/x86_64-linux-gnu/bits/libc-header-start.h \
 /usr/include/features.h /usr/include/x86_64-linux-gnu/sys/cdefs.h \
 /usr/include/x86_64-linux-gnu/bits/wordsize.h \
 /usr/include/x86_64-linux-gnu/bits/long-double.h \
 /usr/include/x86_64-linux-gnu/gnu/stubs.h \
 /usr/include/x86_64-linux-gnu/gnu/stubs-64.h \
 /usr/lib/gcc/x86_64-linux-gnu/7/include/stddef.h \
 /usr/include/x86_64-linux-gnu/bits/types.h \
 /usr/include/x86_64-linux-gnu/bits/typesizes.h \
 /usr/include/x86_64-linux-gnu/bits/types/__FILE.h \
 /usr/include/x86_64-linux-gnu/bits/types/FILE.h \
 /usr/include/x86_64-linux-gnu/bits/libio.h \
 /usr/include/x86_64-linux-gnu/bits/_G_config.h \
 /usr/include/x86_64-linux-gnu/bits/types/__mbstate_t.h \
 /usr/lib/gcc/x86_64-linux-gnu/7/include/stdarg.h \
 /usr/include/x86_64-linux-gnu/bits/stdio_lim.h \
 /usr/include/x86_64-linux-gnu/bits/sys_errlist.h hello.h

This way if there is something like

// #include "mycode.cinc"
import "C"

Cgo could see the dependency on mycode.cinc and other files mycode.cinc includes.

@FlorianUekermann

This comment has been minimized.

Contributor

FlorianUekermann commented Apr 18, 2018

Sorry guys, I misclicked. Didn't mean to close. What @ianlancetaylor and @navytux suggest seems like a good fix to me.

@karalabe

This comment has been minimized.

Contributor

karalabe commented Jul 3, 2018

I think this is basically working as expected.
If you change the underlying C code, or you change the compiler,
then you have to rebuild with -a.

While true, there's a hidden security subtlety here. Lets suppose there's a crypto library called go-crypto, which internally wraps the c-crypto project (random names). The devs of c-crypto find a fatal flaw, fix it and notify go-crypto, who update their vendored C code and issue a new release too.

I - as a user of the go-crypto library - see this and do a go get -u to fetch the new code, sleeping easy that I'm all protected. Except Go didn't bother to actually recompile anything because only the C code changes, so my binary is still vulnerable, even though I built it with the new code.

This same issue will happen arbitrarily high a dependency chain, where anyone forgetting to rebuild with -a could potentially be vulnerable.


Btw, I'm not saying I know how to fix this or whether it's even fixable. I just wanted to add a bit of weight behind this issue.

@FlorianUekermann

This comment has been minimized.

Contributor

FlorianUekermann commented Jul 3, 2018

Just a quick note because nobody has mentioned solutions to the more general issue @rsc pointed out:

If you change the underlying C code, or you change the compiler, then you have to rebuild with -a.

This is going to cause confusing issues in practice. I doubt that everyone is aware of all packages that use C in some sub-dependency. Similarly a lot of people won't always know whether the compiler got updated recently.

As @karalabe points out this is a potential security risk. But it is also a general usability problem, as it may very well break builds or even the resulting binaries.

These problems seem pretty similar to the issues ccache and zapcc face. I don't know where this is documented for zapcc, but ccache has a few pointers here: https://ccache.samba.org/manual/latest.html#_common_hashed_information

In general I don't see much harm in hashing a little more of the environment ($CC -MD, relevant environment variables, $CC -v or the binary itself). I'm starting to doubt that this will ever be perfect, but a couple of safeguards could save a lot of people a lot of time and confusion.

@vcaesar

This comment has been minimized.

vcaesar commented Aug 14, 2018

I think so too, a couple of safeguards could save a lot of people a lot of time and confusion.
If you change the underlying C code, or you change the compiler, then you have to rebuild with -a. This is going to cause confusing issues in practice.

After many people update the package code, they don't even know that the cache of go1.10 caused the bug to not be fixed.

@seebs

This comment has been minimized.

Contributor

seebs commented Sep 30, 2018

As a naive user, I got bitten by this, but at least it was really obvious: there was a bug in the glfw package's C code (caught by newer compiler, which issued a warning). so i changed the code, ran go build... same error. it was not at all obvious why it was giving me an error that couldn't possibly refer to any existing file on the disk, but apparently "-a" would have helped... but that's extremely non-obvious, and there's no reason that i should have to rebuild other unrelated packages to hint "actually, this package has a thing that has changed".

my actual quick workaround: a blank line in the .go file including the affected .c file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment