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

Fix undefined behavior when parsing boolean options in DBSCAN benchmark #804

Merged
merged 1 commit into from
Dec 28, 2022

Conversation

dalg24
Copy link
Contributor

@dalg24 dalg24 commented Dec 28, 2022

I stumbled onto these while looking into #64 and running UBSan locally

<ArborX>/benchmarks/dbscan/dbscan.cpp:71:53: runtime error: member call on address 0x600001afc0a0 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001afc0a0: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  b0 f3 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior <ArborX>/benchmarks/dbscan/dbscan.cpp:71:53 in
<Boost_ROOT>/include/boost/program_options/value_semantic.hpp:201:13: runtime error: member access within address 0x600001afc0a0 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001afc0a0: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  b0 f3 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior <Boost_ROOT>/include/boost/program_options/value_semantic.hpp:201:13 in
<Boost_ROOT>/include/boost/program_options/value_semantic.hpp:202:13: runtime error: member access within address 0x600001afc0a0 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001afc0a0: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  b0 f3 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior <Boost_ROOT>/include/boost/program_options/value_semantic.hpp:202:13 in
<ArborX>/benchmarks/dbscan/dbscan.cpp:82:73: runtime error: member call on address 0x600001af8640 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001af8640: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  14 f4 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior <ArborX>/benchmarks/dbscan/dbscan.cpp:82:73 in
<ArborX>/benchmarks/dbscan/dbscan.cpp:83:55: runtime error: member call on address 0x600001af4000 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001af4000: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  15 f4 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior <ArborX>/benchmarks/dbscan/dbscan.cpp:83:55 in
<ArborX>/benchmarks/dbscan/dbscan.cpp:84:53: runtime error: member call on address 0x600001af40a0 which does not point to an object of type 'boost::program_options::typed_value<bool>'
0x600001af40a0: note: object is of type 'boost::program_options::typed_value<bool, char>'
 00 00 00 00  08 dc 38 03 01 00 00 00  78 dc 38 03 01 00 00 00  16 f4 44 6f 01 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'boost::program_options::typed_value<bool, char>'

The doc
clearly states

This will cause the value to default to false and it will become true if the switch is found

and the API says

but the created value_semantic won't accept any explicit value

@aprokop
Copy link
Contributor

aprokop commented Dec 28, 2022

I don't see API saying that if the option is not present, it is guaranteed to be false. So, do we need to initialize it prior to parsing to guarantee that?

@dalg24
Copy link
Contributor Author

dalg24 commented Dec 28, 2022

I don't see API saying that if the option is not present, it is guaranteed to be false. So, do we need to initialize it prior to parsing to guarantee that?

The doc says "will cause the value to default to false". Tools are pretty good at catching reads from uninitialized values so I am not too concerned but I can do

--- a/benchmarks/dbscan/dbscan.hpp
+++ b/benchmarks/dbscan/dbscan.hpp
@@ -17,7 +17,7 @@ namespace ArborXBenchmark
 struct Parameters
 {
   std::string algorithm;
-  bool binary;
+  bool binary = false;
   int cluster_min_size;
   int core_min_size;
   float eps;
@@ -27,9 +27,9 @@ struct Parameters
   int max_num_points;
   int n;
   int num_samples;
-  bool variable_density;
-  bool verbose;
-  bool verify;
+  bool variable_density = false;
+  bool verbose = false;
+  bool verify = false;
 };

 template <int DIM>

if you really want me to

@aprokop
Copy link
Contributor

aprokop commented Dec 28, 2022

The doc says "will cause the value to default to false".

Where did you see that? I've looking around, and I see some mentions about implicit values, but can't find any confirmation.

@dalg24
Copy link
Contributor Author

dalg24 commented Dec 28, 2022

Follow the first link I posted and look above the 2nd code snippet

@dalg24 dalg24 merged commit 4b17922 into arborx:master Dec 28, 2022
@dalg24 dalg24 deleted the bool_switch_default_value branch December 28, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants