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.

Sign up for GitHub

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

Jump to bottom

Rust: More tests for rust/deadcode #17923

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+304 −118
Open

Rust: More tests for rust/deadcode #17923

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf0e100
Rust: Move some test cases together.
geoffw0 Nov 4, 2024
a8b1cb3
Rust: Make it so that all of the test functions may return.
geoffw0 Nov 6, 2024
fbfdd57
Rust: Additional test cases for rust/dead-code.
geoffw0 Nov 4, 2024
3805d0f
Merge branch 'main' into unreachable4
geoffw0 Nov 8, 2024
c7112ef
Rust: Accept consistency check changes.
geoffw0 Nov 8, 2024
727a7d4
Merge branch 'main' into unreachable4
geoffw0 Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions rust/ql/test/query-tests/unusedentities/UnreachableCode.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,21 @@
| unreachable.rs:39:9:39:23 | ExprStmt | This code is never reached. |
| unreachable.rs:46:9:46:23 | ExprStmt | This code is never reached. |
| unreachable.rs:67:5:67:19 | ExprStmt | This code is never reached. |
| unreachable.rs:114:13:114:20 | MacroExpr | This code is never reached. |
| unreachable.rs:123:13:123:20 | MacroExpr | This code is never reached. |
| unreachable.rs:149:5:149:19 | ExprStmt | This code is never reached. |
| unreachable.rs:156:9:156:23 | ExprStmt | This code is never reached. |
| unreachable.rs:165:13:165:27 | ExprStmt | This code is never reached. |
| unreachable.rs:171:9:171:23 | ExprStmt | This code is never reached. |
| unreachable.rs:177:13:177:27 | ExprStmt | This code is never reached. |
| unreachable.rs:185:13:185:27 | ExprStmt | This code is never reached. |
| unreachable.rs:188:5:188:19 | ExprStmt | This code is never reached. |
| unreachable.rs:212:9:212:23 | ExprStmt | This code is never reached. |
| unreachable.rs:228:9:228:23 | ExprStmt | This code is never reached. |
| unreachable.rs:134:13:134:20 | MacroExpr | This code is never reached. |
| unreachable.rs:143:13:143:20 | MacroExpr | This code is never reached. |
| unreachable.rs:166:9:166:23 | ExprStmt | This code is never reached. |
| unreachable.rs:171:9:171:17 | MacroExpr | This code is never reached. |
| unreachable.rs:177:9:177:26 | MacroExpr | This code is never reached. |
| unreachable.rs:206:9:206:23 | ExprStmt | This code is never reached. |
| unreachable.rs:231:13:231:27 | ExprStmt | This code is never reached. |
| unreachable.rs:241:13:241:27 | ExprStmt | This code is never reached. |
| unreachable.rs:247:9:247:23 | ExprStmt | This code is never reached. |
| unreachable.rs:254:17:254:31 | ExprStmt | This code is never reached. |
| unreachable.rs:264:17:264:31 | ExprStmt | This code is never reached. |
| unreachable.rs:267:9:267:23 | ExprStmt | This code is never reached. |
| unreachable.rs:303:9:303:23 | ExprStmt | This code is never reached. |
| unreachable.rs:332:9:332:23 | ExprStmt | This code is never reached. |
| unreachable.rs:348:9:348:23 | ExprStmt | This code is never reached. |
| unreachable.rs:370:9:370:23 | ExprStmt | This code is never reached. |
| unreachable.rs:375:5:375:10 | ExprStmt | This code is never reached. |
| unreachable.rs:382:5:382:10 | ExprStmt | This code is never reached. |
2 changes: 2 additions & 0 deletions rust/ql/test/query-tests/unusedentities/UnusedValue.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@
| more.rs:44:9:44:14 | a_ptr4 | Variable $@ is assigned a value that is never used. | more.rs:44:9:44:14 | a_ptr4 | a_ptr4 |
| more.rs:59:9:59:13 | d_ptr | Variable $@ is assigned a value that is never used. | more.rs:59:9:59:13 | d_ptr | d_ptr |
| more.rs:65:9:65:17 | f_ptr | Variable $@ is assigned a value that is never used. | more.rs:65:13:65:17 | f_ptr | f_ptr |
| unreachable.rs:292:9:292:15 | for_ten | Variable $@ is assigned a value that is never used. | unreachable.rs:292:9:292:15 | for_ten | for_ten |
| unreachable.rs:299:9:299:16 | for_ever | Variable $@ is assigned a value that is never used. | unreachable.rs:299:9:299:16 | for_ever | for_ever |
11 changes: 7 additions & 4 deletions rust/ql/test/query-tests/unusedentities/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,14 +481,17 @@ fn main() {
folds_and_closures();

unreachable_if_1();
// unreachable_panic();
unreachable_if_2();
unreachable_if_3();
unreachable_panic();
_ = unreachable_bail();
unreachable_match();
// unreachable_loop();
unreachable_loop();
unreachable_loop_async();
unreachable_paren();
unreachable_let_1();
unreachable_let_2();
unreachable_if_2();
unreachable_if_3();
unreachable_attributes();

macros();
}
206 changes: 168 additions & 38 deletions rust/ql/test/query-tests/unusedentities/unreachable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,26 @@ pub fn unreachable_if_1() {
do_something(); // $ Alert[rust/dead-code]
}

pub fn unreachable_if_2() {
if cond() {
do_something();
return;
} else {
do_something();
}

do_something();
}

pub fn unreachable_if_3() {
if !cond() {
do_something();
return;
}

do_something();
}

pub fn unreachable_panic() {
if cond() {
do_something();
Expand Down Expand Up @@ -127,33 +147,89 @@ pub fn unreachable_panic() {
}
}

pub fn unreachable_match() {
match get_a_number() {
1 => {
return;
}
_ => {
do_something();
}
macro_rules! bail_1 {
() => {
return Err(String::from("message"))
};
}

macro_rules! bail_2 {
($message:literal) => {
return Err(String::from($message))
};
}

pub fn unreachable_bail() -> Result<i32, String> {
if cond() {
do_something();
return Err(String::from("message"));
do_something(); // $ Alert[rust/dead-code]
}

if cond() {
do_something();
bail_1!(); // $ SPURIOUS: Alert[rust/dead-code]
do_something(); // $ MISSING: Alert[rust/dead-code]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue with the workarounds for PostOrderTree and macro nodes. I'll figure out a way to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue to track this, I know what to do but I have other priorities.

}

if cond() {
do_something();
bail_2!("message"); // $ SPURIOUS: Alert[rust/dead-code]
}
do_something();

match get_a_number() {
1 => {
return;
Ok(1)
}

pub fn unreachable_match() {
if cond() {
match get_a_number() {
1 => {
return;
}
_ => {
do_something();
}
}
_ => {
return;
do_something();
}

if cond() {
match get_a_number() {
1 => {
return;
}
_ => {
return;
}
}
do_something(); // $ Alert[rust/dead-code]
}

if cond() {
_ = match get_a_number() {
1 => "One",
_ => "Some"
};
do_something();
}

if cond() {
_ = Some(match get_a_number() {
1 => "One",
_ => "Some"
});
do_something();
}
do_something(); // $ Alert[rust/dead-code]
}

pub fn unreachable_loop() {
loop {
do_something();
break;
do_something(); // $ Alert[rust/dead-code]
if cond() {
loop {
do_something();
break;
do_something(); // $ Alert[rust/dead-code]
}
}

if cond() {
Expand All @@ -171,23 +247,67 @@ pub fn unreachable_loop() {
do_something(); // $ Alert[rust/dead-code]
}

for _ in 1..10 {
if cond() {
continue;
do_something(); // $ Alert[rust/dead-code]
if cond() {
for _ in 1..10 {
if cond() {
continue;
do_something(); // $ Alert[rust/dead-code]
}
do_something();
}
do_something();
}

loop {
if cond() {
return;
do_something(); // $ Alert[rust/dead-code]
if cond() {
loop {
if cond() {
return;
do_something(); // $ Alert[rust/dead-code]
}
}
do_something(); // $ Alert[rust/dead-code]
do_something();
do_something();
}
do_something(); // $ Alert[rust/dead-code]

if cond() {
fn do_nothing() { };
fn loop_forever() { loop {} };
fn take_a_fn(_: fn() -> ()) {
};
fn call_a_fn(f: fn() -> ()) {
f();
};

take_a_fn( do_nothing );
call_a_fn( do_nothing );
take_a_fn( loop_forever );
call_a_fn( loop_forever );
do_something(); // $ MISSING: Alert[rust/dead-code]
}
}

async fn do_something_async() {}

pub async fn unreachable_loop_async() {
let for_ten = async { // $ SPURIOUS: Alert[rust/unused-value]
for _ in 1..10 {
do_something_async().await;
}
do_something();
};

let for_ever = async { // $ SPURIOUS: Alert[rust/unused-value]
loop {
do_something_async().await;
}
do_something(); // $ Alert[rust/dead-code]
};

do_something();
for_ten.await;
do_something();
for_ever.await;
do_something(); // $ MISSING: Alert[rust/dead-code]
}

pub fn unreachable_paren() {
Expand Down Expand Up @@ -232,22 +352,32 @@ pub fn unreachable_let_2() {
do_something();
}

pub fn unreachable_if_2() {
if cond() {
#[cfg(not(foo))]
pub fn unreachable_attributes() {
// `#[cfg` and `cfg!` checks can go either way, we should not assume this
// function or either branch below is unreachable.
if cfg!(bar) {
do_something();
return;
} else {
do_something();
}

do_something();
}
#[doc="This is a doc comment declared through an attribute."]

pub fn unreachable_if_3() {
if !cond() {
if (true) {
do_something();
return;
} else {
do_something(); // $ Alert[rust/dead-code]
}

do_something();
}

const _: () = {
_ = 1; // $ SPURIOUS: Alert[rust/dead-code]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a child of a Const (Const > BlockExpr > StmtList > ExprStmt > AssignmentExpr); I'm not sure if we want to exclude these from the query, or add them to the CFG so that they're considered reachable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One argument for including them in the CFG is that const code could contain unreachable code. If we exclude constant things, then we will not be able to detect this and other things (unused variables, etc.). An argument in the other direction is that code evaluated at compile time is unlikely to contain security issues. Or maybe it can?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like const code is less likely to contain security issues, but still could. Certainly for data flow - for example the value of a security relevant constant (e.g. enabling or disabling a feature) could be computed in const code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what steps we need to take to include them in the CFG? Is it just a case of adding the entry points somewhere???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue to track this, but I could do with some pointers (or someone else to take on the issue, if that is easier).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll be looking at that together as well as the async problem 👍

};

const _: () = {
const fn foo() {
_ = 1;
};
foo(); // $ SPURIOUS: Alert[rust/dead-code]
};
Loading