Bad Build Checks check libraries

[jonathanmetzman]

I saw this output on a local branch of skia:

Broken fuzz targets (2):
libEGL.so:
BAD BUILD: /out/libEGL.so seems to have either startup crash or exit:
libGLESv2.so:
BAD BUILD: /out/libGLESv2.so seems to have either startup crash or exit:
22 fuzzers total, 2 seem to be broken (9%).
Check build passed

[Dor1s]

We treat any executable file in the $OUT directory as a fuzz target. That dir is not supposed to have any other executable files. Just remove the executable bit from those shared libraries and any other files that don't actually need the executable bit.

[jonathanmetzman]

I (think I) can mark the .so files as non-executable.
But it is the norm for shared libraries to be executable on Linux. Clang and GCC do this by default.
Feel free to close if you think this is the responsibility of the project, but I think this will probably cause problems in the future. I think it makes more sense to check for LLVMFuzzerTestOneInput to determine if something is a fuzz target.

[Dor1s]

Wow, I din't know that those are marked as executable by default. Would it be possible to put those files into a subdir of $OUT/, as we are using -maxdepth 1 option of find utility?

[jonathanmetzman]

I think I would need to add some parameters at link time so that it would look in the subdirectory.
I'd rather make the libraries non-executable.
But either way, it doesn't really solve the problem in the general case (eg: projects that ship shared libs that aren't skia)

note: it isn't actually needed for them to be marked executable on Linux, but the fact that they are executable by default is an issue.

[inferno-chromium]

checking for LLVMFuzzerTestOneInput should work everywhere, @jonathanmetzman can you add that in the bad build check ? @Dor1s - any reason for not that ?

[Dor1s]

It's still unclear to me why projects need to ship shared libraries, as we explicitly require to use static linking as per https://github.com/google/oss-fuzz/blob/798abca6f4bc42203bcc52684bdd145a0b494eeb/docs/fuzzer_environment.md#runtime-dependencies

[Dor1s]

@inferno-chromium just don't want to add any unnecessary complications for handling the behavior we do discourage and tend to avoid

[jonathanmetzman]

It's still unclear to me why projects need to ship shared libraries, as we explicitly require to use static linking as per

I don't know that SwiftShader needs to be dynamically linked, but I haven't seen any way to build a static version of SwiftShader and FWIW I think Chrome uses it as a shared object, even in non-component builds (maybe so that it can switch between angle and swiftshader?).

I don't actually know why the docs are anti-dynamic linking, as long as projects ship the library in $OUT I don't see what would go wrong (other than this :-).
I can definitely be wrong about this though.

[jonathanmetzman]

Blocking #1563

[Dor1s]

Fair point regarding SwiftShader, I don't know either!

I think in general static builds are faster and more portable, that's why we prefer those.

@oliverchang may have any opinion here as well. I don't want to be a blocker here if I'm the only one who against that check :)

[jonathanmetzman]

I think in general static builds are faster and more portable, that's why we prefer those.

I agree, my uneducated opinion is that static linking is usually preferable.
It's worth pointing out that libGLESv2 is 80 MB. I don't have a real idea of what would happen if we statically linked it (given the link time optimization issues caused by ASAN) but it may save quite a bit of space.

[oliverchang]

I think it makes sense to check for LLVMFuzzerTestOneInput in executables. It's not too complicated and matches CF behaviour.

Even if we discourage shared libraries, not having the LLVMFuzzerTestOneInput check during bad build check is not the correct check for that :)

[jonathanmetzman]

Even if we discourage shared libraries, not having the LLVMFuzzerTestOneInput check during bad build check is not the correct check for that :)

+1

Do you know some of the reasons not to ship DLLs in $OUT? the main one I can think of is messing up clang coverage but I'm sure there is something I am missing.