CPP: IR weirdness with unexpected result for `getUnconvertedResultExpression`

[intrigus-lgtm]

(Kind of related to #21241 and #21240)

While testing my query, I noticed that I had a duplicate result for line 28 (if (&x)).
(Only visible when looking at the .expected file of my test output)
The IR supposedly had two address-of instruction at that line.
I don't understand why and looking at the IR the only difference between line 28 and 29 is that there is one CompareNE instruction in the one case and one CompareEQ instruction in the other case (as expected).

| twice!NonSSA: NonSSA: 8:ResultCopy | AddressOfNullCheck.c:28:9
 | twice!NonSSA: NonSSA: 7:ValCondCondCompare | AddressOfNullCheck.c:28:9
 | twice!NonSSA: NonSSA: 15:ResultCopy | AddressOfNullCheck.c:29:10

IR graph dump query

/**
 * @name ff
 * @description aa
 * @kind graph
 * @id aaaaa
 * @tags security
 */

import cpp
import semmle.code.cpp.ir.IR
import semmle.code.cpp.ir.PrintIR
import semmle.code.cpp.Declaration

private class Foo extends PrintIRConfiguration {
  override predicate shouldPrintDeclaration(Declaration decl) {
    decl.(Function).getName() = "test_direct_bool_context"
  }
}

Dot file

digraph {
  compound=true;
  subgraph cluster_0 {
    0[shape=point, width=0];
    "label"="void test_direct_bool_context()"; 
    
    subgraph cluster_1 {
      1[shape=point, width=0];
      "label"="Block 0"; 
      
      2["label"="v24_1(void) = EnterFunction : "; ];
      3["label"="m24_2(unknown) = AliasedDefinition : "; ];
      4["label"="m24_3(unknown) = InitializeNonLocal : "; ];
      5["label"="m24_4(unknown) = Chi : total:m24_2, partial:m24_3"; ];
      6["label"="r25_1(glval<int>) = VariableAddress[x] : "; ];
      7["label"="m25_2(int) = Uninitialized[x] : &:r25_1"; ];
      8["label"="r28_1(glval<int>) = VariableAddress[x] : "; ];
      9["label"="r28_2(int *) = CopyValue : r28_1"; ];
      10["label"="r28_3(int *) = Constant[0] : "; ];
      11["label"="r28_4(bool) = CompareNE : r28_2, r28_3"; ];
      12["label"="v28_5(void) = ConditionalBranch : r28_4"; ];
    }
    subgraph cluster_13 {
      13[shape=point, width=0];
      "label"="Block 1"; 
      
      14["label"="v28_6(void) = NoOp : "; ];
    }
    subgraph cluster_15 {
      15[shape=point, width=0];
      "label"="Block 2"; 
      
      16["label"="r29_1(glval<int>) = VariableAddress[x] : "; ];
      17["label"="r29_2(int *) = CopyValue : r29_1"; ];
      18["label"="r29_3(int *) = Constant[0] : "; ];
      19["label"="r29_4(bool) = CompareEQ : r29_2, r29_3"; ];
      20["label"="v29_5(void) = ConditionalBranch : r29_4"; ];
    }
    subgraph cluster_21 {
      21[shape=point, width=0];
      "label"="Block 3"; 
      
      22["label"="v29_6(void) = NoOp : "; ];
    }
    subgraph cluster_23 {
      23[shape=point, width=0];
      "label"="Block 4"; 
      
      24["label"="v30_1(void) = NoOp : "; ];
      25["label"="v24_5(void) = ReturnVoid : "; ];
      26["label"="v24_6(void) = AliasedUse : m24_3"; ];
      27["label"="v24_7(void) = ExitFunction : "; ];
    }
  }
  13 -> 15["label"="Goto"; "ltail"="cluster_13"; "lhead"="cluster_15"; ];
  21 -> 23["label"="Goto"; "ltail"="cluster_21"; "lhead"="cluster_23"; ];
  1 -> 15["label"="False"; "ltail"="cluster_1"; "lhead"="cluster_15"; ];
  15 -> 23["label"="False"; "ltail"="cluster_15"; "lhead"="cluster_23"; ];
  1 -> 13["label"="True"; "ltail"="cluster_1"; "lhead"="cluster_13"; ];
  15 -> 21["label"="True"; "ltail"="cluster_15"; "lhead"="cluster_21"; ];
}

Query

/**
 * @name Suspicious check on address-of expression
 * @description Taking the address of a valid object always produces a non-NULL
 *              pointer, so checking the result against NULL is either redundant
 *              or indicates a bug where a different check was intended.
 * @kind problem
 * @problem.severity warning
 * @precision high
 * @id asymmetric-research/suspicious-address-of-null-check
 * @tags reliability
 *       correctness
 *       readability
 */

import cpp
import semmle.code.cpp.controlflow.Guards
/**
 * Technically, we'd ignore cases where we do something like &foo->first_field (where foo is a pointer)
 * this is because if foo is NULL, the expression could actually evaluate to NULL.
 * However, it doesn't seem likely that anyone would write code like that intentionally
 * and it's also undefined behavior to dereference a NULL pointer anyway.
 */
class ValidAddressOfExpr extends AddressOfExpr { }


from Instruction source
where
  source.getUnconvertedResultExpression() instanceof ValidAddressOfExpr and
    source.getLocation().getStartLine() = [28, 29]
select source, "twice!" + source.getUniqueId()

Testcase

#define NULL ((void*)0)

struct foo {
    int a;
    long b;
};
typedef struct foo foo_t;

int global_var;
int global_array[10];

// Test direct comparisons with local variables
void test_direct_comparison_local(void) {
    int x;

    // Direct comparison of address-of with NULL - should alert
    if (&x == NULL) {}       // $ Alert
    if (&x != NULL) {}       // $ Alert
    if (NULL == &x) {}       // $ Alert
    if (0 == &x) {}          // $ Alert
}

// Test direct boolean context with local variables
void test_direct_bool_context(void) {
    int x;

    // Direct use in boolean context - should alert
    if (&x) {}               // $ Alert DUPLICATE HERE
    if (!&x) {}              // $ Alert
}

[redsun82]

👋 @intrigus-lgtm thanks for the detailed report! I looked into this and can confirm it's a bug in the IR construction layer.

As far as I can tell, the issue is in getInstructionConvertedResultExpression in IRConstruction.qll. For if (&x), two different instructions end up mapped to the same AddressOfExpr:

1. CopyValue (ResultCopy): Generated by TranslatedResultCopy because AddressOfExpr satisfies exprNeedsCopyIfNotLoaded — this produces the "result" instruction for the expression.

2. CompareNE (ValueConditionCompare): Generated by TranslatedValueCondition for the implicit boolean conversion in C. The second disjunct in getInstructionConvertedResultExpression explicitly maps this instruction back to the original expression (to match C++ behavior where there's an explicit BoolConversion).

Both instructions get getUnconvertedResultExpression() = &x, causing the duplicate.

For something like if (!&x) there's no duplicate because the NotExpr sits between the AddressOfExpr and the boolean condition context — the CompareNE maps to the NotExpr, not the AddressOfExpr.

I'll relay this internally.

[MathiasVP]

No bugs here, (un)fortunately. This is working as intended. Let me explain the relevant background:

CodeQL databases created from C compilation have no integer-to-boolean conversions in the AST. So if you have a DB generated from a C compilation this function:

void f(void) {
    int x;
    if (&x) {}
}

there the fully converted expression of the conditional will be the AddressOfExpr - and nothing more. However, if you generate a DB from the same code, but compiled with a C++ compiler, you will see an integer-to-Boolean conversion as the fully converted expression of the conditional.

This distinction between C and C++ is what the code @redsun82 linked to above is related to.

In both cases, the IR we generate for the conditional will look something like:

r4_1(glval<int>) = VariableAddress[x] :
r4_2(int *)      = CopyValue          : r4_1
r4_3(int *)      = Constant[0]        :
r4_4(bool)       = CompareNE          : r4_2, r4_3
v4_5(void)       = ConditionalBranch  : r4_4

The CompareNE (i.e., the “not equal to 0” comparison) represents the integer-to-Boolean conversion. In C, the second disjunct in

codeql/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll

Lines 175 to 190 in 6fbf727

	// Consider the snippet `if(x) { ... }` where `x` is an integer.
	// In C++ there is a `BoolConversion` conversion on `x` which generates a
	// `CompareNEInstruction` whose `getInstructionConvertedResultExpression`
	// is the `BoolConversion` (by the logic in the disjunct above). Thus,
	// calling `getInstructionUnconvertedResultExpression` on the
	// `CompareNEInstruction` gives `x` in C++ code.
	// However, in C there is no such conversion to return. So instead we have
	// to map the result of `getInstructionConvertedResultExpression` on the
	// `CompareNEInstruction` to `x` manually. This ensures that calling
	// `getInstructionUnconvertedResultExpression` on the `CompareNEInstruction`
	// gives `x` in both the C case and C++ case.
	exists(TranslatedValueCondition translatedValueCondition |
	translatedValueCondition = getTranslatedCondition(result) and
	translatedValueCondition.shouldGenerateCompareNE() and
	instruction = translatedValueCondition.getInstruction(ValueConditionCompareTag())
	)

explicitly maps the CompareNE to &x (since there's no conversion in the AST).

In C++ there is an integer-to-Boolean, so

codeql/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll

Lines 166 to 173 in 6fbf727

	exists(TranslatedExpr translatedExpr |
	translatedExpr = getTranslatedExpr(result) and
	instruction = translatedExpr.getResult() and
	// Only associate `instruction` with this expression if the translated
	// expression actually produced the instruction; not if it merely
	// forwarded the result of another translated expression.
	instruction = translatedExpr.getInstruction(_)
	)

(i.e., the first disjunct) maps the CompareNE to the Boolean conversion expression.

The end result is that, both in C/C++, when asking for the instruction which computes the unconverted expression (i.e., &x) you get both the CompareNE and the CopyValue. In C, the converted expression computed by CompareNE is &x (and the unconverted result of that is &x), and in C++ the converted expression computed by CompareNE is (Bool) ... (and the unconverted result of that is &x).