Import null pointer information from PDG into static analysis #1086

ahomescu · 2024-04-25T05:19:35Z

No description provided.

ahomescu · 2024-04-25T05:22:54Z

I still need to test this on some actual code (maybe lighttpd?)

c2rust-analyze/src/analyze.rs

pdg/src/builder.rs

c2rust-analyze/src/analyze.rs

spernsteiner · 2024-06-03T16:18:30Z

Is this waiting on anything, other than needing a cargo fmt to fix the CI?

dynamic_instrumentation/src/instrument.rs

fw-immunant

LGTM overall. The brokenness in visit_place is surprising but I agree that the current behavior is wrong as described; see inline comment.

spernsteiner · 2024-06-17T21:46:31Z

@ahomescu Don't forget to fix the CI. Currently it's failing on the cargo fmt style check.

spernsteiner

Initial comments. I haven't looked at the PDG code yet.

analysis/runtime/src/events.rs

dynamic_instrumentation/src/instrument.rs

spernsteiner

Seems generally reasonable. This approach of collapsing nested projections down to a single byte offset will limit our ability to extract borrowchecker aliasing information from the PDG, if we end up wanting to do that in the future.

Are there any automated tests related to instrumentation or the PDG? I thought we had one or two, but I could be wrong.

pdg/src/builder.rs

spernsteiner · 2024-06-24T23:36:07Z

pdg/src/builder.rs

+        CopyRef => unimplemented!(),
+        AddrOfLocal(ptr, _, size) => {
+            // TODO: is this a local from another function?
+            mapping.size = size.try_into().ok();


Can this conversion ever fail? If not, it seems like you could make the field size: usize instead of Option<usize> and avoid the None case above.

The field would still need to be size: Option<usize> because we have other cases where we get a pointer without a known size. That comes from the None on line 268.

Some of this is happening because we get pointers from native libraries (e.g. glibc) which we have no control over. I think the best we can do right now is say "this is a pointer we know nothing about", and maybe add some clever heuristics later.

Re the conversion: I could do Some(size.try_into().unwrap()), that might be better for debugging?

pdg/src/graph.rs

Record field and index projections for pointers by recording a Project event with a base and target pointer pair.

spernsteiner

Looks reasonable overall. Left a few comments.

analysis/runtime/src/metadata.rs

c2rust-analyze/src/analyze.rs

dynamic_instrumentation/src/instrument.rs

pdg/src/builder.rs

Add an index value to Project events to differentiate between different projections with the same pointer offset, e.g., (*p).x and (*p).x.a if a is the first field of the structure. This is implemented as a HashMap<u64, Vec<usize>> where each element is a unique combination of field projections. The Project key points to an element in this map. The keys are randomly-generated 64-bit keys with very low probability of collision.

The event assignment-based PDG construction steps were giving incorrect results for indirect accesses, so remove them completely.

Track the provenance of pointers with finer granularity by storing the size of every allocation, local, and constant inside the corresponding events. With this information, we can keep track of the boundaries of every object and track whether a projected pointer falls inside the original allocation.

Keep track of the size of every local for the new provenance algorithm.

Add a new arg_ptr() method that allows passing a pointer directly to an event handler. The handler needs to be generic on the pointee type: fn foo_handler<T: ?Sized>(ptr: *const T) { ... } This change allows us to get more information about the pointees from the event handler body, e.g., call std::mem::size_of_val().

Add a new AddrOfSized event and use it to keep track of the sizes of all constant operands for the new provenance algorithm.

Handle Offset nodes with a base pointer of 0 where the offset is non-zero, potentially resulting in a brand new pointer. One such case occurs in mod_cgi from lighttpd: const uintptr_t baseptr = (uintptr_t)env->b->ptr; for (i = 0; i < env->oused; ++i) envp[i] += baseptr;

Mark each PDG graph with a boolean flag that represents whether that graph corresponds to the null pointer or not. The PDG construction algorithm seems to build one unique graph for all null pointers in the entire program.

Add one test where a function argument can be either null or non-null in the recur() function of the analysis/tests/misc example code.

The dynamic instrumentation code inserts additional locals for its own instrumentation code.

Remove the NON_NULL permission from all nodes in the null graph from the PDG.

If the dynamic analysis tells us a pointer is never NULL (i.e. it never shows up in an is_null graph), then force the NON_NULL permission on it using updates_forbidden. This is potentially an unsound transformation because the pointer may actually be NULL on other inputs not covered by the sample ones.

ahomescu requested review from spernsteiner and fw-immunant April 25, 2024 05:19

ahomescu self-assigned this Apr 25, 2024

ahomescu force-pushed the ahomescu/non_null branch from ac9abd6 to 0a55190 Compare April 25, 2024 05:21

kkysen changed the title ~~Import null pointer information from PDF into static analysis~~ Import null pointer information from PDG into static analysis Apr 25, 2024

spernsteiner approved these changes Apr 26, 2024

View reviewed changes

c2rust-analyze/src/analyze.rs Outdated Show resolved Hide resolved

c2rust-analyze/src/analyze.rs Outdated Show resolved Hide resolved

pdg/src/builder.rs Outdated Show resolved Hide resolved

ahomescu force-pushed the ahomescu/non_null branch from 0a55190 to 1f4aa09 Compare April 26, 2024 23:05

ahomescu commented Apr 30, 2024

View reviewed changes

c2rust-analyze/src/analyze.rs Show resolved Hide resolved

ahomescu force-pushed the ahomescu/non_null branch from 1f4aa09 to 2b30b48 Compare May 18, 2024 05:51

fw-immunant reviewed Jun 3, 2024

View reviewed changes

dynamic_instrumentation/src/instrument.rs Outdated Show resolved Hide resolved

fw-immunant approved these changes Jun 3, 2024

View reviewed changes

ahomescu force-pushed the ahomescu/non_null branch 9 times, most recently from f0cadd4 to de844f5 Compare June 13, 2024 07:21

ahomescu force-pushed the ahomescu/non_null branch 4 times, most recently from 3a5ab91 to 710b626 Compare June 15, 2024 05:54

spernsteiner reviewed Jun 24, 2024

View reviewed changes

ahomescu force-pushed the ahomescu/non_null branch from 710b626 to 31c703a Compare July 18, 2024 05:45

ahomescu force-pushed the ahomescu/non_null branch 2 times, most recently from bcfaceb to 2a94d8f Compare August 5, 2024 23:38

Handle nested fields and other projections

c8928fe

Record field and index projections for pointers by recording a Project event with a base and target pointer pair.

ahomescu force-pushed the ahomescu/non_null branch 2 times, most recently from db8c192 to e9b6204 Compare August 14, 2024 06:27

spernsteiner approved these changes Aug 14, 2024

View reviewed changes

analysis/runtime/src/metadata.rs Outdated Show resolved Hide resolved

c2rust-analyze/src/analyze.rs Show resolved Hide resolved

dynamic_instrumentation/src/instrument.rs Outdated Show resolved Hide resolved

pdg/src/builder.rs Outdated Show resolved Hide resolved

ahomescu force-pushed the ahomescu/non_null branch 5 times, most recently from 34ddf68 to b0351bf Compare August 21, 2024 22:51

ahomescu force-pushed the ahomescu/non_null branch from b0351bf to c86dd94 Compare August 27, 2024 00:36

ahomescu added 14 commits August 26, 2024 18:27

Build PDG without tracking assignments

949905b

The event assignment-based PDG construction steps were giving incorrect results for indirect accesses, so remove them completely.

Remove unused CopyRef NodeKind

cbd1983

Add size of locals to AddrOfLocal events

d2578b4

Keep track of the size of every local for the new provenance algorithm.

Add constant size information to events

fcda013

Add a new AddrOfSized event and use it to keep track of the sizes of all constant operands for the new provenance algorithm.

Add is_null flag to PDG graphs

e984216

Mark each PDG graph with a boolean flag that represents whether that graph corresponds to the null pointer or not. The PDG construction algorithm seems to build one unique graph for all null pointers in the entire program.

Add an extra null pointer test to analysis example

dfd848a

Add one test where a function argument can be either null or non-null in the recur() function of the analysis/tests/misc example code.

Log unknown PDG functions instead of panicking

677dbdc

Log and ignore out-of-bounds locals in PDG

3888185

The dynamic instrumentation code inserts additional locals for its own instrumentation code.

Use PDG to remove NON_NULL from pointers in static analysis

afe6541

Remove the NON_NULL permission from all nodes in the null graph from the PDG.

ahomescu force-pushed the ahomescu/non_null branch from c86dd94 to 36ed671 Compare August 27, 2024 01:28

Update reference PDG for analysis/tests/minimal

f898e86

ahomescu force-pushed the ahomescu/non_null branch from 39acd9d to f898e86 Compare August 28, 2024 01:35

ahomescu mentioned this pull request Aug 31, 2024

analyze: Path- or context-sensitive analysis #1118

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import null pointer information from PDG into static analysis #1086

Import null pointer information from PDG into static analysis #1086

ahomescu commented Apr 25, 2024

ahomescu commented Apr 25, 2024

spernsteiner commented Jun 3, 2024

fw-immunant left a comment

spernsteiner commented Jun 17, 2024

spernsteiner left a comment

spernsteiner left a comment

spernsteiner Jun 24, 2024

ahomescu Jul 26, 2024

spernsteiner left a comment

Import null pointer information from PDG into static analysis #1086

Are you sure you want to change the base?

Import null pointer information from PDG into static analysis #1086

Conversation

ahomescu commented Apr 25, 2024

ahomescu commented Apr 25, 2024

spernsteiner commented Jun 3, 2024

fw-immunant left a comment

Choose a reason for hiding this comment

spernsteiner commented Jun 17, 2024

spernsteiner left a comment

Choose a reason for hiding this comment

spernsteiner left a comment

Choose a reason for hiding this comment

spernsteiner Jun 24, 2024

Choose a reason for hiding this comment

ahomescu Jul 26, 2024

Choose a reason for hiding this comment

spernsteiner left a comment

Choose a reason for hiding this comment