Merge branch 'conditionally-noreturn' into 'main'

Fix a bug that could cause false-negative fallthrough CFG edges

Closes #634

See merge request rewriting/ddisasm!1255

Author: jdorn-gt

CHANGELOG.md

@@ -4,6 +4,7 @@
     - Fix several issues that could result in missing symbolic expressions
     - Improve resolution of TLS-related symbolic expression
 * Fix bug that could lead to functional errors due to false-positive symbolic operands or data.
+* Fix bug that could drop fall-through edges from calls to conditionally no-return functions
 
 # 1.9.2
 

examples/asm_examples/ex_noreturn_conditional/Makefile

@@ -0,0 +1,14 @@
+.PHONY: all clean check
+all: out.txt
+
+out.txt: ex
+	@./$^ > $@
+ex: ex_original.s
+	gcc $^ -no-pie -o $@
+
+clean:
+	rm -f ex out.txt
+	rm -fr ex.unstripped ex.s *.old*  dl_files *.gtirb
+check:
+	./ex > /tmp/res.txt
+	@ diff out.txt /tmp/res.txt && echo TEST OK

examples/asm_examples/ex_noreturn_conditional/ex_original.s

@@ -0,0 +1,128 @@
+
+.intel_syntax noprefix
+.extern printf
+.extern exit
+
+.section .rodata
+msg_main1:
+  .string "main: calling conditional_sinkcall(5)\n"
+msg_main2:
+  .string "main: calling conditional_sinkcall(2)\n"
+msg_main3:
+  .string "main: calling conditional_sinkcall(0) -- should not return\n"
+msg_after:
+  .string "main: AFTER conditional_sinkcall(0) (unreachable)\n"
+
+msg_conditional_sinkcall_enter:
+  .string "conditional_sinkcall: entered with x=%d\n"
+msg_conditional_sinkcall_ret:
+  .string "conditional_sinkcall: returning normally\n"
+msg_fatal:
+  .string "fatal_error: exiting program\n"
+
+.section .text
+
+# ---------------------------------------
+# void fatal_error()
+# prints message then exits (noreturn)
+# ---------------------------------------
+.type fatal_error,@function
+fatal_error:
+    lea rdi, msg_fatal[rip]
+    xor eax, eax
+    call printf
+
+    xor edi, edi
+    call exit
+
+    # unreachable safeguard
+    hlt
+
+
+# ---------------------------------------
+# void conditional_sinkcall(int x)
+# argument in edi
+# ---------------------------------------
+.type conditional_sinkcall,@function
+conditional_sinkcall:
+    push rbp
+    mov rbp, rsp
+    sub rsp, 16
+
+    mov [rbp-4], edi # save argument x
+
+    # log entry
+    mov esi, edi
+    lea rdi, msg_conditional_sinkcall_enter[rip]
+    xor eax, eax
+    call printf
+
+    mov edi, [rbp-4] # restore x
+    test edi, edi
+    je .error_path
+
+    lea rdi, msg_conditional_sinkcall_ret[rip]
+    xor eax, eax
+    call printf
+
+    leave
+    ret
+
+.error_path:
+    leave
+    call fatal_error # no return
+    hlt
+
+
+# ---------------------------------------
+# main()
+# ---------------------------------------
+.global main
+.type main,@function
+main:
+    push rbp
+    mov rbp, rsp
+    sub rsp, 16
+
+    lea rdi, msg_main1[rip]
+    xor eax, eax
+    call printf
+
+call_1:
+    mov edi, 5
+    call conditional_sinkcall
+
+    lea rdi, msg_main2[rip]
+    xor eax, eax
+    call printf
+
+call_2:
+    mov edi, 2
+    call conditional_sinkcall
+
+    nop
+    nop
+    nop
+    nop
+
+    lea rdi, msg_main3[rip]
+    xor eax, eax
+    call printf
+
+    mov edi, 0
+    # 0: sink call: does not return
+    call conditional_sinkcall
+
+    nop
+    nop
+    nop
+
+.type fall_through_func,@function
+fall_through_func:
+    # unreachable if analysis works
+    lea rdi, msg_after[rip]
+    xor eax, eax
+    call printf
+
+    xor edi, edi
+    call exit

src/datalog/cfg.dl

@@ -237,7 +237,7 @@ resolved_transfer_to_symbol(EA,Function, "branch"):-
 cfg_edge(Src,Dest,Conditional,"false","fallthrough"):-
     refined_block_control_instruction(Src,EA),
     may_fallthrough(EA,Dest),
-    !no_return_call_propagated(EA),
+    !no_return_call_propagated(EA,_),
     !nop_block(Src),
     code_in_refined_block(Dest,Dest),
     // Do not cross a section boundary

src/datalog/code_inference.dl

@@ -179,6 +179,26 @@ likely_fallthrough(From,To):-
         conditional_jump(From)
     ).
 
+/**
+A block `Block` falls through a NOP/padding sled to the next real block `NextBlock`.
+`From` is the source instruction of the edge leading to `NextBlock`.
+*/
+.decl fallthrough_over_padding(Block:address, From:address, NextBlock:address)
+
+fallthrough_over_padding(Block, From, NextBlock):-
+    block_last_instruction(Block, BlockLast),
+    may_fallthrough(BlockLast, NextBlock),
+    !candidate_block_is_padding(NextBlock),
+    From = BlockLast.
+
+fallthrough_over_padding(Block, From, NextBlock):-
+    block_last_instruction(Block, Inst),
+    may_fallthrough(Inst, PadBlock),
+    candidate_block_is_padding(PadBlock),
+    block_last_instruction(PadBlock, PadLast),
+    may_fallthrough(PadLast, NextBlock),
+    From = PadLast.
+
 //////////////////////////////////////////////////////////////
 // This is a small refinement for discarding immediates as targets
 // in some obvious cases. This is specially useful for PIE code where

src/datalog/code_inference_postprocess.dl

@@ -226,3 +226,22 @@ padding_prefix_end(EA,Block):-
     !arch.is_nop(EA),
     next(PrevEA,EA),
     padding_prefix(PrevEA,Block).
+
+/**
+A refined block `Block` contains at least one `return` instruction.
+*/
+.decl refined_block_contains_return(Block:address)
+
+refined_block_contains_return(Block):-
+    refined_block(Block),
+    code_in_refined_block(EA,Block),
+    arch.return(EA).
+
+/**
+A function `Func` contains at least one `return` instruction.
+*/
+.decl refined_block_contains_return_in_func(Func:address)
+
+refined_block_contains_return_in_func(Func) :-
+    function_inference.in_function(Block, Func),
+    refined_block_contains_return(Block).

src/datalog/noreturn.dl

@@ -90,7 +90,7 @@ segment_target_range(Beg,End,MaxTgt):-
         // propagate if it is a jump to a no-return proc.
         direct_or_pcrel_jump(BlockEnd,InterTarget),
         inter_procedural_edge(BlockEnd,_),
-        no_return_block(InterTarget),
+        no_return_block(InterTarget,_),
         MaxTgt = PrevMaxTgt
     ),
     MaxTgt >= Beg.
@@ -112,43 +112,64 @@ that are not known to be no-return.
 The only situation where it would generate false-positive noreturns is if a
 "known noreturn" library function does, in fact, return.
 */
-.decl no_return_block(EA:address)
+.decl no_return_block(EA:address,Reason:symbol)
 
 // If we have a self-contained segment and the
 // last block does not fallthrough, all the blocks
 // in the segment cannot return
 // If the fallthrough in the last block turns
 // out to be a no-return call, the same applies
-no_return_block(Block):-
+no_return_block(Block,"no-fallthrough"):-
     self_contained_segment(Beg,End),
     block_boundaries(LastBlock,_,End),
     block_last_instruction(LastBlock,BlockEnd),
     (
         !may_fallthrough(BlockEnd,_);
-        no_return_call_propagated(BlockEnd)
+        no_return_call_propagated(BlockEnd,_)
     ),
     block_boundaries(Block,Beg,EndBlock),
     EndBlock <= End.
 
-// A function is called, and the call falls through interprocedurally.
-// The function is likely noreturn.
-// The edge after this call site should already be eliminated because it's
-// interprocedural, but this rule ensures other calls to the same function
-// don't introduce problematic edges.
-no_return_block(Func):-
+/**
+A function `Func` is called at instruction `Call` and the call may fall through
+to the next instruction interprocedurally.
+NOP or padding instructions may follow the call.
+*/
+.decl call_may_fallthrough_inter(Call:address,Func:address)
+
+call_may_fallthrough_inter(Call,Func):-
     direct_call(Call,Func),
-    may_fallthrough(Call,Fallthrough),
-    (
-        !candidate_block_is_padding(Fallthrough),
-        Next = Fallthrough,
-        From = Call
-        ;
-        candidate_block_is_padding(Fallthrough),
-        block_last_instruction(Fallthrough,From),
-        may_fallthrough(From,Next)
-    ),
+    block_last_instruction(CallBlock,Call),
+    fallthrough_over_padding(CallBlock,From,Next),
     inter_procedural_edge(From,Next).
 
+/**
+Represents the next initial function following `Func`
+(from `function_entry_initial`, before use-def and value analysis).
+`NextFunc` is the immediate successor of `Func`.
+*/
+.decl next_function_entry_initial(Func:address,NextFunc:address)
+
+next_function_entry_initial(Func, NextFunc) :-
+    function_inference.function_entry_initial(Func),
+    NextFunc = min F : {
+        function_inference.function_entry_initial(F),
+        F > Func
+    }.
+
+/**
+An initial function (`function_entry_initial`, prior to use-def
+and value analysis) contains one or more `return` instructions.
+This is a heuristic and may generate false positives.
+*/
+.decl initial_function_containing_return(Func:address,ReturnEA:address)
+
+initial_function_containing_return(Func,EA):-
+    next_function_entry_initial(Func, NextFunc),
+    EA >= Func,
+    EA < NextFunc,
+    arch.return(EA).
+
 /**
 Calls to known no return functions or their PLT blocks.
 */
@@ -164,19 +185,77 @@ no_return_call_refined(EA):-
     no_return_function(Pattern),
     match(Pattern,Function).
 
-no_return_block(Block):-
+no_return_block(Block,"no_return_call_refined"):-
     no_return_call_refined(BlockEnd),
     block_last_instruction(Block,BlockEnd).
 
+/**
+A call `Call` targets a function `CallTarget` that has another call-site
+which falls through interprocedurally.
+*/
+.decl call_target_has_other_fallthrough_inter(Call:address,CallTarget:address)
+
+call_target_has_other_fallthrough_inter(Call,CallTarget):-
+    call_may_fallthrough_inter(OtherCall,CallTarget),
+    direct_call(Call,CallTarget),
+    OtherCall != Call.
+
 /**
 Calls to noreturn blocks.
 */
-.decl no_return_call_propagated(EA:address)
+.decl no_return_call_propagated(EA:address,Reason:symbol)
 
-no_return_call_propagated(EA):-
+no_return_call_propagated(EA,"no_return_call_refined"):-
     no_return_call_refined(EA).
 
-no_return_call_propagated(EA):-
+no_return_call_propagated(EA,"direct_call to no_return_block"):-
     direct_call(EA,Block),
-    no_return_block(Block),
+    no_return_block(Block,_),
     !pc_load_call(EA,Block).
+
+// A function is called, and the call falls through interprocedurally.
+// The function is likely noreturn.
+// The edge after this call site should already be eliminated because
+// it's interprocedural.
+no_return_call_propagated(Call,"interprocedural fallthrough"):-
+    direct_call(Call,CallTarget),
+    call_may_fallthrough_inter(Call,CallTarget),
+    !no_return_call_refined(Call),
+    !pc_load_call(Call,CallTarget).
+
+// Any function target that has call-site falling through interprocedurally
+// is likely noreturn. This rule ensures other calls to the same function
+// don't introduce problematic edges.
+// However, a function can be conditionally noreturn (i.e., it may return along
+// some paths but not others).
+// To avoid incorrectly removing legitimate fallthrough CFG edges, this rule
+// conservatively checks whether `Next` is a `possible_target`, and if not,
+// we do not classify the function call as noreturn function call.
+no_return_call_propagated(
+    Call,
+    "call possiblly_no_return_func and fallthrough-next is a possible target"
+):-
+    call_target_has_other_fallthrough_inter(Call,CallTarget),
+    !call_may_fallthrough_inter(Call,CallTarget),
+    !pc_load_call(Call,CallTarget),
+    !no_return_call_refined(Call),
+    block_last_instruction(CallBlock,Call),
+    fallthrough_over_padding(CallBlock,_,Next),
+    (
+        // No return instruction appears in the target function (internal).
+        !initial_function_containing_return(CallTarget,_), UNUSED(Next),
+        !plt_block(CallTarget,_)
+        ;
+        // If the target is external, there is not enough information to
+        // determine whether it is conditionally non-returning.
+        // As a heuristic, classify it as a no-return call only when
+        // Next is a possible target.
+        plt_block(CallTarget,_),
+        possible_target(Next)
+        ;
+        // The call target function may contain a return, which may
+        // indicate that it is a conditionally non-returning function.
+        // Further check whether Next is a possible target.
+        initial_function_containing_return(CallTarget,_),
+        possible_target(Next)
+    ).