Fix several undefined behaviors #1645

mmisono · 2020-11-22T19:08:51Z

UBSan (Undefined Behavior Sanitizer) found several errors when running bpftrace_test. This commits fix them.
Please see the commit messages for the details.

Checklist

Language changes are updated in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

danobi · 2020-11-23T17:45:10Z

src/ast/semantic_analyser.cpp

-        long value = static_cast<Integer &>(size_arg).n;
-        if (value < 0)
+        auto integer = dynamic_cast<Integer *>(&size_arg);
+        if (integer)


I think this check was wrong to begin with. We should only be downcasting and checking the value after we know it's a literal, right? Otherwise we could be reading junk. So in a is_final_pass() branch and after the check_arg(). Same for the buf case. I believe that would solve this static_cast issue as well.

That makes sense. But I found in the case of buf(), the length (buffer_size) is used in other places, and we cannot check the length in the final pass.. So I decided to use dynamic_cast here. 735b944 please let me know if there is a better way.

Can't buf() just use max_buffer_size until the final pass? Then on the final pass it can do all the error checks and calculations.

I first tried it and failed. Then I realized that AssignMap did not update the map size. I fixed it and now I think it's OK: bfded77

Is update_assign_map_type() fixing a separate issue? I'm not seeing why it's related.

without it, https://github.com/iovisor/bpftrace/blob/7ca62cb0aca9d6125d8a96fdc0238f7e07354097/src/ast/semantic_analyser.cpp#L2067 is failed because it thinks map size is max_buffer_size, but expr_size become smaller in the final pass.

danobi · 2020-11-23T20:51:12Z

src/ast/semantic_analyser.cpp

@@ -604,7 +604,7 @@ void SemanticAnalyser::visit(Call &call)
        }
      }

-      if (call.vargs->size() == 2)
+      if (is_final_pass() && call.vargs->size() == 2)


I think you have to check the type before the downcast otherwise you don't know what you're casting

Suggested change

if (is_final_pass() && call.vargs->size() == 2)

if (is_final_pass() && check_arg(..) && call.vargs->size() == 2)

And then delete the check_arg() below

I see. I missed the check_arg() below. thanks.

UBSan detected the following error. ``` % sudo /src/bpftrace -e 'BEGIN { @ = -9223372036854775808 }' src/parser.yy:297:81: runtime error: signed integer overflow: -1 * -9223372036854775808 cannot be represented in type 'long' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/parser.yy:297:81 in Attaching 1 probe... ^C @: -9223372036854775808 ``` -9223372036854775808 is LONG_MIN, but when creating ast::Integer, we multiply the value by -1, and this is the cause of the error. To avoid this error, we can manually calculate negative number like the following: ``` (long)(~(unsigned long)($2) + 1) ``` Note that a minus LONG_MIN is LONG_MIN. ``` % ./src/bpftrace -e 'BEGIN { $x = -9223372036854775808; printf("%ld\n", -$x); } ' Attaching 1 probe... -9223372036854775808 ^C ``` To cope with this, we need a runtime overflow checker.

mmisono · 2020-11-26T21:57:19Z

hmm.. still there are some dowancast issues

% sudo ./src/bpftrace -e "kprobe:f { str(arg0, arg1); }" 
/disk/work/bpftrace2/src/ast/semantic_analyser.cpp:634:22: runtime error: downcast of address 0x611000002d40 which does not point to an object of type 'bpftrace::ast::Integer'
0x611000002d40: note: object is of type 'bpftrace::ast::Builtin'
 11 00 00 10  58 17 3a 01 00 00 00 00  00 00 00 00 00 00 00 00  01 00 00 00 16 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'bpftrace::ast::Builtin

mmisono · 2020-11-26T22:02:13Z

the problem is Type::integer does not necessarily mean that is ast::Integer.

mmisono · 2020-11-26T22:24:58Z

ah, sorry I forgot about is_literal... now the problem is almost solved

UBSan reports the following error when running bpftrace_test. ``` /work/bpftrace2/src/ast/semantic_analyser.cpp:662:22: runtime error: downcast of address 0x7fffe80cced0 which does not point to an object of type 'bpftrace::ast::Integer' 0x7fffe80cced0: note: object is of type 'bpftrace::ast::String' 00 00 00 00 a8 fa 4b 01 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 16 00 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'bpftrace::ast::String' ``` the problem test is ``` test("kprobe:f { str(arg0, \"hello\"); }", 10) ``` and the program tries to downcast "hello" to ast::Integer. To fix this, do static cast only in the final pss. In the final pass we know it's a integer literal by check_arg().

UBSan reports the following error. ``` % sudo ./src/bpftrace -e 'BEGIN { @ = lhist(pid, 0, 100, 1); }' /work/bpftrace2/src/ast/codegen_llvm.cpp:462:26: runtime error: downcast of address 0x611000002d40 which does not point to an object of type 'bpftrace::ast::Integer' 0x611000002d40: note: object is of type 'bpftrace::ast::Builtin' 08 00 80 7e a8 a6 39 01 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 0d 00 00 00 00 00 00 00 ^~~~~~~~~~~~~~~~~~~~~~~ vptr for 'bpftrace::ast::Builtin' ``` There is no need to static cast to Integer, so simply remove them.

danobi requested changes Nov 23, 2020

View reviewed changes

mmisono force-pushed the fix_undefined_behavior branch from a61be2f to a4cd32b Compare November 23, 2020 20:12

danobi reviewed Nov 23, 2020

View reviewed changes

mmisono force-pushed the fix_undefined_behavior branch from a4cd32b to b6e5ecb Compare November 23, 2020 22:37

mmisono force-pushed the fix_undefined_behavior branch from b6e5ecb to fa2c2ab Compare November 26, 2020 21:33

mmisono added 3 commits November 27, 2020 07:35

Update changelog

1557f74

mmisono force-pushed the fix_undefined_behavior branch from fa2c2ab to 1557f74 Compare November 26, 2020 22:37

mmisono requested a review from danobi November 26, 2020 22:48

danobi approved these changes Nov 30, 2020

View reviewed changes

mmisono merged commit b6ef34b into bpftrace:master Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several undefined behaviors #1645

Fix several undefined behaviors #1645

mmisono commented Nov 22, 2020 •

edited

Loading

danobi Nov 23, 2020 •

edited

Loading

mmisono Nov 23, 2020

danobi Nov 23, 2020

mmisono Nov 23, 2020

danobi Nov 23, 2020

mmisono Nov 23, 2020

danobi Nov 23, 2020

mmisono Nov 23, 2020

mmisono commented Nov 26, 2020

mmisono commented Nov 26, 2020

mmisono commented Nov 26, 2020

	if (is_final_pass() && call.vargs->size() == 2)
	if (is_final_pass() && check_arg(..) && call.vargs->size() == 2)

Fix several undefined behaviors #1645

Fix several undefined behaviors #1645

Conversation

mmisono commented Nov 22, 2020 • edited Loading

Checklist

danobi Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

mmisono Nov 23, 2020

Choose a reason for hiding this comment

danobi Nov 23, 2020

Choose a reason for hiding this comment

mmisono Nov 23, 2020

Choose a reason for hiding this comment

danobi Nov 23, 2020

Choose a reason for hiding this comment

mmisono Nov 23, 2020

Choose a reason for hiding this comment

danobi Nov 23, 2020

Choose a reason for hiding this comment

mmisono Nov 23, 2020

Choose a reason for hiding this comment

mmisono commented Nov 26, 2020

mmisono commented Nov 26, 2020

mmisono commented Nov 26, 2020

mmisono commented Nov 22, 2020 •

edited

Loading

danobi Nov 23, 2020 •

edited

Loading