Skip to content
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

Workaround for OSS-Fuzz std::format detection #3120

Closed
wants to merge 3 commits into from

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Jan 14, 2025

No description provided.

@kmilos kmilos added the CI Issues related with CI jobs label Jan 14, 2025
@kmilos kmilos marked this pull request as ready for review January 14, 2025 10:49
@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

Why not test for std::format directly?

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 14, 2025

From the CheckCXXSymbolExists docs:

If the symbol is a type, enum value, or C++ template it will not be recognized: consider using the CheckTypeSize or CheckSourceCompiles module instead.

It looks like it might not always work w/ non-templated std:vformat either:

This command is unreliable when <symbol> is (potentially) an overloaded function.

Switching to CheckSourceCompiles would be just as verbose, if not more effort...

This way the check/workaround is the same in CMake and source, and IMHO easily identifiable and removable in the future when OSS-Fuzz image is updated.

In any case, let's first confirm this approach works w/ -DCMAKE_CXX_STANDARD=20? @kevinbackhouse

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

@@ -56,7 +56,7 @@ deps += cpp.find_library('ws2_32', required: host_machine.system() == 'windows')
 deps += cpp.find_library('psapi', required: host_machine.system() == 'windows')
 deps += cpp.find_library('procstat', required: host_machine.system() == 'freebsd')
 
-if not cpp.has_header_symbol('format', '__cpp_lib_format')
+if not cpp.has_header_symbol('format', 'std::format')
   deps += dependency('fmt')
 endif
 

on meson works. It's actually shorter...

@kevinbackhouse
Copy link
Collaborator

Wow! Is this the answer to the OSS-Fuzz build failure?

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

Yeah even

env CXX=clang++ CXXFLAGS=-stdlib=libc++ meson -Dcpp_std=c++20 a

succeeds.

edit: oh but it fails to compile as the code ends up using fmt.

@kevinbackhouse
Copy link
Collaborator

I'm testing it now.

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

@kmilos what do you think of this alternate solution?

neheb@3f7fbda

@kevinbackhouse
Copy link
Collaborator

It works! Awesome work @kmilos!

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 14, 2025

Sure, the other proposal looks good. Just wasn't sure it'd work going by CMake docs...

@kevinbackhouse
Copy link
Collaborator

@neheb's: is your solution a permanent fix? I.e. we won't need to remember to revert it at some stage in the future when OSS-Fuzz gets fixed? If so, that sounds even better.

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

oh this is hilarious. So it works under meson but not cmake:

 error: address of overloaded function 'forma    t' does not match required type 'int'
 35             7 |   return ((int*)(&std::format))[argc];
 34               |                   ^~~~~~~~~~~
 33         /usr/bin/../include/c++/v1/__format/format_functions.h:476:1: note: candidate template ignored: could not match 'string (format_string<_Args...>,     _Args &&...)' (aka 'basic_string<char> (basic_format_string<char, type_identity_t<type-parameter-0-0>...>, _Args &&...)') against 'int'
 32           476 | format(format_string<_Args...> __fmt, _Args&&... __args) {
 31               | ^
 30         /usr/bin/../include/c++/v1/__format/format_functions.h:483:1: note: candidate template ignored: could not match 'wstring (wformat_string<_Args...>    , _Args &&...)' (aka 'basic_string<wchar_t> (basic_format_string<wchar_t, type_identity_t<type-parameter-0-0>...>, _Args &&...)') against 'int'
 29           483 | format(wformat_string<_Args...> __fmt, _Args&&... __args) {
 28               | ^
 27         /usr/bin/../include/c++/v1/__format/format_functions.h:609:1: note: candidate template ignored: could not match 'string (locale, format_string<_Ar    gs...>, _Args &&...)' (aka 'basic_string<char> (std::locale, basic_format_string<char, type_identity_t<type-parameter-0-0>...>, _Args &&...)') against 'in    t'
 26           609 | format(locale __loc, format_string<_Args...> __fmt, _Args&&... __args) {
 25               | ^
 24         /usr/bin/../include/c++/v1/__format/format_functions.h:616:1: note: candidate template ignored: could not match 'wstring (locale, wformat_string<_    Args...>, _Args &&...)' (aka 'basic_string<wchar_t> (std::locale, basic_format_string<wchar_t, type_identity_t<type-parameter-0-0>...>, _Args &&...)') aga    inst 'int'
 23           616 | format(locale __loc, wformat_string<_Args...> __fmt, _Args&&... __args) {
 22               | ^
 21         1 error generated.
 20         ninja: build stopped: subcommand failed.

That has to be a joke.

@kevinbackhouse
Copy link
Collaborator

I've squashed the commits on my testing branch so that it's almost ready to submit: kevinbackhouse/oss-fuzz#12

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

OK. Finally fixed CMake: https://github.com/neheb/exiv2/commit/d8867845918cc2e7e75e9ee9bbfd57994db73c00.patch

My solution is more generic. I can't say I like matching against compilers.

@kevinbackhouse
Copy link
Collaborator

For future reference this is the link to the discussions that we had about the OSS-Fuzz build failure (which started after #2769 was merged): #3108

I also asked for help on the OSS-Fuzz repo but nobody has replied so far.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 14, 2025

I'll close this PR then in favour of the other, more generic solution.

@kmilos kmilos closed this Jan 14, 2025
@kevinbackhouse
Copy link
Collaborator

I'm testing @neheb's version now

@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

Looks like my version was missing

--- a/src/image_int.cpp
+++ b/src/image_int.cpp
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
+#include "config.h"
 #include "image_int.hpp"
 
 #include <cstdarg>

Added and force pushed.

@neheb
Copy link
Collaborator

neheb commented Jan 15, 2025

For reference, I'll note that you cannot mix and match libc or libc++. OSS-Fuzz builds with -stdlib=libc++ (LLVM) when the OS uses libstdc++ (GNU). That's the whole reason for the errors with fmt.

This also happens with system INIReader, which doesn't seem to be used.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 15, 2025

you cannot mix and match libc or libc++. OSS-Fuzz builds with -stdlib=libc++ (LLVM) when the OS uses libstdc++ (GNU)

... when trying to link static libs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues related with CI jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants