Add environment variables expansion capability

[paulsemel]

This is needed to get some path for Chrome that will depend on BUILD_DIR.

[jonathanmetzman]

Could you explain what this is needed for better?

[paulsemel]

Sure!

In order for chrome to get some data, Chrome internal path service system needs to know where to find that. Since we're packaging fuzzers differently, the data is not in its usual place. However, since this is a common thing, there is already support for overriding the default path with the environment variable CR_SOURCE_ROOT. Basically, what we'd do is CR_SOURCE_ROOT=%BUILD_DIR%/src_root and that would make it work again. Currently it's breaking all our https based fuzzers.

I think this change is harmless, because we're passing the environment variables to the fuzzers anyways, so fuzzers would be able to know the content of them when launching.

WDYT? Does it make sense to you?

[jonathanmetzman]

@ochang does this sound fine to you?

[paulsemel]

Hello there @jonathanmetzman @oliverchang!

I'd need this to get in in order to fix our fuzzers! WDYT about this?

[oliverchang]

Sure!

In order for chrome to get some data, Chrome internal path service system needs to know where to find that. Since we're packaging fuzzers differently, the data is not in its usual place. However, since this is a common thing, there is already support for overriding the default path with the environment variable CR_SOURCE_ROOT. Basically, what we'd do is CR_SOURCE_ROOT=%BUILD_DIR%/src_root and that would make it work again. Currently it's breaking all our https based fuzzers.

I think this change is harmless, because we're passing the environment variables to the fuzzers anyways, so fuzzers would be able to know the content of them when launching.

WDYT? Does it make sense to you?

Still lacking some context here :)

Would you mind explaining in a bit more detail? Is this required for a particular blackbox fuzzer? or libFuzzer fuzzers?

If this is a breakage when things worked before, what changed?

[paulsemel]

Sure! In order for chrome to get some data, Chrome internal path service system needs to know where to find that. Since we're packaging fuzzers differently, the data is not in its usual place. However, since this is a common thing, there is already support for overriding the default path with the environment variable CR_SOURCE_ROOT. Basically, what we'd do is CR_SOURCE_ROOT=%BUILD_DIR%/src_root and that would make it work again. Currently it's breaking all our https based fuzzers. I think this change is harmless, because we're passing the environment variables to the fuzzers anyways, so fuzzers would be able to know the content of them when launching. WDYT? Does it make sense to you?

Still lacking some context here :)

Would you mind explaining in a bit more detail? Is this required for a particular blackbox fuzzer? or libFuzzer fuzzers?

If this is a breakage when things worked before, what changed?

Chrome needs to know the path of some root directory that basically contains test data. However, on other architectures, this path is something like ../../path, which is not the case on ClusterFuzz of course (src_root). To let chrome know about this, we can set an environment variable with the full path of this directory. To do this, we need to know where ClusterFuzz actually unpacked our directory (the BUILD_DIR directory for instance), so that we can set the env var to CR_SOURCE_ROOT=%BUILD_DIR%/src_root so that Chrome can access the files correctly.

This is for now needed by our InProcessFuzzer fuzzers (libfuzzer and centipede), but might be needed by others in the future.

No breakage at all, never worked before!

Does it make sense to you?

[paulsemel]

Thanks for the context! I still do have some concerns with this approach:

• This is dependent on expand() being called at the right time, which makes it hard to reason about.

I mean, we are calling it right before execution, which I think should be the only valid moment to call it. Maybe we could stop exposing the expand method and modify the existing copy function? That way, you only expand the one you are manipulating:

def copy(expand=False):
    env = os.environ.copy()
    if expand:
        _expand(env)
    return env

• It's a destructive, irreversible transformation on the environment.

True. But again, with the proposal above, that's different, we only modify the copy we're making. We could also only allow certain variables to be expanded, but I don't think that really matters tbh.

• The implementation introduces a bit of complexity.

Is there any possibility that CR_SOURCE_ROOT can be calculated at runtime by the fuzzer in some other way, e.g. based on the cwd() + argv[0] ?

That would work with the fuzzer for which we currently need this. However, I am unsure we'll always be able to control command line arguments in the future depending on how we write fuzzers in chrome, so I am not a big fan of this solution. The environment variable has the advantage of being future proof from that point of view, which is probably the reason why it is implemented that way in Chrome.

If there's no other way to do this, it may be still better to just hardcode the logic to set CR_SOURCE_ROOT in build_manager.py as a chrome-specific hardcode.

That would work. However, I have two concerns with this:

• This is again not very future proof. I think it is not impossible that we need other env var to be dependent on the BUILD_DIR in the future, for some reasons. So maybe one solution is to only allow expanding the *_DIR env variables?
• I agree this adds complexity. However, maybe if we narrow down what we can expand here it would remove some and be easier to merge this? For instance, there already is code in ClusterFuzz that does var expansion, so having only some var that we can expand is already an accepted complexity (https://github.com/google/clusterfuzz/blob/7325e7d2d7e4a30ce2a599cbb1f1e5cceac8643c/src/clusterfuzz/_internal/bot/testcase_manager.py#L1039-L1050) (and maybe I could merge the code that does it?).

We'd have something like:

// The only accepted expansion would be the vars listed in `vars`
def expand(env, vars)

Then, we can essentially call this only when setting BUILD_DIR.

WDYT?

[oliverchang]

Thanks for the context! I still do have some concerns with this approach:

• This is dependent on expand() being called at the right time, which makes it hard to reason about.

I mean, we are calling it right before execution, which I think should be the only valid moment to call it. Maybe we could stop exposing the expand method and modify the existing copy function? That way, you only expand the one you are manipulating:

Then this creates a strange divide between env vars acessed by ClusterFuzz vs child processes. Also, new_process is not the only place we create new processes.

Re future proofing, I would prefer we keep things simple for now and follow YAGNI. This is the first we've seen this need in all the years ClusterFuzz has existed. If this comes back in the future, then I think that would be a strong argument to consider your current approach again.

[jonathanmetzman]

I think we have decided against this so please reopen if I'm wrong.