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

Allow setting component specific CC, CFLAGS, LDFLAGS from conf file #22

Merged
merged 8 commits into from Aug 5, 2017

Conversation

Projects
None yet
3 participants
@paraschetal
Contributor

paraschetal commented Jul 23, 2017

This will allow setting component specific:

  • CC: In many of the qubes components, CC=gcc has been hardcoded in the Makefiles (example). However, in order to build with other compiler toolchains like clang, with the added advantage of its hardening protection and sanitizer flags along with better static analysis, we should allow setting it from the builder.conf file itself.(example)

  • CFLAGS: This will make it easy to set protection flags while building the components and help fixing issues like this. Also, with CC_component set as clang, we will easily be able to use sanitizer flags while building which will help detecting bugs while testing/fuzzing without having to modify the component's Makefile.

  • LDFLAGS: Setting component specific LDFLAGS from the configuration file itself will help for things like statically linking (which is required by oss-fuzz btw).

While building a couple of components under clang, using a configuration file like this, qubes-builder has been able to already detect an errors like:

...
+ make dom0
(cd gui-daemon; make)
make[1]: Entering directory '/home/user/qubes-src/gui-daemon/gui-daemon'
clang -fstack-protector-strong -I../include/ -g -O2 -Wall -Wextra -Werror -fPIC `pkg-config --cflags libconfig` `pkg-config --cflags libnotify` `pkg-config --cflags libpng` `pkg-config --cflags vchan-xen`   -c -o xside.o xside.c
xside.c:2933:13: error: address of array 'g->vmname' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
    if (!g->vmname) {
        ~~~~^~~~~~
1 error generated.
<builtin>: recipe for target 'xside.o' failed
make[1]: *** [xside.o] Error 1
make[1]: Leaving directory '/home/user/qubes-src/gui-daemon/gui-daemon'
...

So, the end goal here could be to build all components under clang with all hardening flags, correcting all the compiler warnings, and finally checking the security of the built executables by checksec.sh.

Once this is merged, we can proceed with changing the Makefiles of the components. I have already had to make some changes like this on my forks to allow building with clang. Of course, the CC, CFLAGS, LDFLAGS can still be set from the component's Makefile to perhaps override those set from builder.conf, but I don't think we should hardcode them by default since it reduces flexibility and control while building the components.

PS: Building using clang would require it to be installed in the chroot dir. I have therefore added it to qubes-builder-fedora's build-pkgs-base.list. I'm not sure if this is the best way since it is not essential for a regular build using gcc itself.
Also, I had to change python-sh to python2-sh in the DEPENDENCIES of the Makefile of qubes-builder since the components were not being built otherwise on my fedora-25 VM. I am not sure if this change should be made permanent since it might break the build for other distros.

/cc @jpouellet

paraschetal added some commits Jul 23, 2017

@marmarek

This comment has been minimized.

Member

marmarek commented Jul 23, 2017

This, in general, looks like a wrong approach, at least for release builds. Build should be less dependent on build environment, not more. If for nothing else, for being reproducible (QubesOS/qubes-issues#816). This is why we have those chroot environments in the first place, why we carefully filter the environment etc.

So, if at all, this should be allowed only for test/devel builds. Since we don't have "build type" setting (maybe we should? something close to this is INCREMENT_DEVEL_VERSIONS), there should be large warning when this setting is enabled. And then, for flexibility I'd go with a single setting for BUILD_ENV_$(COMPONENT) instead of separate setting for each selected variable (what if you want to override CXX?).

On the other hand - not hardcoding CC in individual Makefiles is a good idea.

@jpouellet

This comment has been minimized.

Contributor

jpouellet commented Jul 23, 2017

I discussed this with @paraschetal on IRC. Perhaps we should have such discussions on qubes-devel in the future.

@marmarek We're on the same page re: wanting things to be reproducible, and avoiding propagating env vars from the calling environment makes sense as part of that. The intention was not to leak env vars from the caller of qubes-builder, but only to explicitly set them via builder.conf. Being able to easily specify compiler flags for components is useful and necessary for things like QubesOS/qubes-issues#2259 and various instrumentation (asan, msan, ubsan, etc.), and afaict does not harm reproducibility as long as they are strictly controlled via builder.conf rather than propagated from outside. Do you see this differently?

@paraschetal Default values (edit: even if empty) should likely be set someplace. Additionally, I think the used values should be dumped in the build-id and/or build-info targets.

@jpouellet

This comment has been minimized.

Contributor

jpouellet commented Jul 23, 2017

And then, for flexibility I'd go with a single setting for BUILD_ENV_$(COMPONENT) instead of separate setting for each selected variable (what if you want to override CXX?).

👍

@marmarek

This comment has been minimized.

Member

marmarek commented Jul 23, 2017

afaict does not harm reproducibility as long as they are strictly controlled via builder.conf rather than propagated from outside. Do you see this differently?

Ideally we'd use upstream tools (pbuilder, mock etc) to build release packages. This means src.rpm, or debian source package should be the only build process input. No extra build environments, extra settings etc. I mean "qubes-gui-vm-3.2.17-1.fc25.x86_64" should be enough to know how to reproduce exactly the same package.
Unfortunately this goal isn't realistic, but for example adding Debian buildinfo file should be enough.

Anyway, for development/test builds it's ok to have more flexibility. And it should be clearly marked that given build is not a release build. build-id and/or build-info targets is a good idea.

As for QubesOS/qubes-issues#2259 - it's more about the thing - not harcoding values in Makefile (in this case overriding those provided by rpmbuild), rather than specifying them in builder.conf.

@paraschetal

This comment has been minimized.

Contributor

paraschetal commented Jul 24, 2017

I agree that using this for release builds might not be a good idea, however for test/devel builds it will definitely help. So having a "build type" setting seems to be a good option to me. How could this be best implemented?

@paraschetal Default values (edit: even if empty) should likely be set someplace.

I have now replaced individual env variables with a generalized one, do you still think setting default values is required (since we do not know which env variables the user is going to set using it)?
Maybe the default values of only the essential variables for the component could be set from the builder itself. But how will we identify such variables? Like some components might require CXX, some CC, and so on.

Additionally, I think the used values should be dumped in the build-id and/or build-info targets.

From the build-id's message The following settings copied to builder.conf will make builder use exactly the same sources, it appears to me that its purpose is to actually help in creating the builder.conf for a reproducible build, and not actually inform about the current build. And, build-info is not displayed always while building components. Like it is not displayed when I run make gui-daemon (it is displayed when make get-sources is run though). So maybe displaying the user set environment variables elsewhere (a separate build-env-info) with a more prominent warning might be better.

### LDFLAGS_`component`
> Default: Value set in Makefile of the component
For example:
ENV_gui-daemon = "\

This comment has been minimized.

@marmarek

marmarek Jul 24, 2017

Member

Many applications (including shell) does not like - in env variable name. This is why in this type of settings we replace - with _ (see BRANCH_, GIT_URL_ etc).

This comment has been minimized.

@marmarek

marmarek Jul 24, 2017

Member

Also, I hope it is still possible to specify multiple flags, like "CFLAGS=-Wall -Werror", i.e. individual values with spaces, right? Probably some escaping will be needed (this example should show how to do it).

@marmarek

This comment has been minimized.

Member

marmarek commented Jul 24, 2017

I agree that using this for release builds might not be a good idea, however for test/devel builds it will definitely help. So having a "build type" setting seems to be a good option to me. How could this be best implemented?

Maybe a setting RELEASE_BUILD? It could either forcefully set some other settings (like disable INCREMENT_DEVEL_VERSIONS, NO_SIGN, ENV_*, enable CHECK_BRANCH etc), or error out when unexpected value is set. Also configs in example-config should have RELEASE_BUILD = 1 with appropriate comment. @jpouellet what do you think?

I have now replaced individual env variables with a generalized one, do you still think setting default values is required (since we do not know which env variables the user is going to set using it)?
Maybe the default values of only the essential variables for the component could be set from the builder itself. But how will we identify such variables? Like some components might require CXX, some CC, and so on.

So maybe displaying the user set environment variables elsewhere (a separate build-env-info) with a more prominent warning might be better.

IMO build-info is ok. It is shown before make qubes. Combined with above RELEASE_BUILD setting should be enough.

@jpouellet

This comment has been minimized.

Contributor

jpouellet commented Jul 24, 2017

Maybe a setting RELEASE_BUILD? It could either forcefully set some other settings (like disable INCREMENT_DEVEL_VERSIONS, NO_SIGN, ENV_*, enable CHECK_BRANCH etc), or error out when unexpected value is set. Also configs in example-config should have RELEASE_BUILD = 1 with appropriate comment. @jpouellet what do you think?

👍 Not sure about NO_SIGN since one may wish to sign separately from the build machine regardless, but concept sounds good to me.

@jpouellet

This comment has been minimized.

Contributor

jpouellet commented Jul 24, 2017

OTOH, if you're building an iso from clean, then it would make sense to sign pkgs. Nevermind. Sounds good.

paraschetal added some commits Aug 5, 2017

@marmarek marmarek merged commit 4203c2c into QubesOS:master Aug 5, 2017

1 check passed

policy/qubesos/code-signing Signed with 0E12409536B3420B
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment