-
Notifications
You must be signed in to change notification settings - Fork 211
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.
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
Import null pointer information from PDG into static analysis #1086
base: master
Are you sure you want to change the base?
Conversation
ac9abd6
to
0a55190
Compare
I still need to test this on some actual code (maybe lighttpd?) |
0a55190
to
1f4aa09
Compare
if g.is_null { | ||
// TODO: is this enough? | ||
perms.remove(PermissionSet::NON_NULL); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spernsteiner If I'm understanding #1088 correctly, the other thing we need to do here is add NON_NULL
to updates_forbidden
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roughly, yes. Though we might want to experiment a bit - for example, it might be easier to understand the results if we set updates_forbidden
only on function signatures, or only on named variables and not on temporaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there can be two nodes with the same dest
(either in different Graph
s or in the same one), representing two different observations of that dest
during the execution. We want to set NON_NULL
in perms
(and in updates_forbidden
) only if all observations with the same dest
show it containing a non-null pointer. So we need some additional logic beyond just doing perms.insert(NON_NULL); updates_forbidden.insert(NON_NULL);
here.
1f4aa09
to
2b30b48
Compare
Is this waiting on anything, other than needing a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. The brokenness in visit_place
is surprising but I agree that the current behavior is wrong as described; see inline comment.
07b6823
to
17d72f0
Compare
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.
Remove the NON_NULL permission from all nodes in the null graph from the PDG.
2d86abb
to
b044ed5
Compare
Introduce a ProjectionTree data structure in the PDG construction algorithm which keeps track of assignments into projections of locals. This lets us reconstruct the correct source of some events where the direct source is missing.
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;
b044ed5
to
f0cadd4
Compare
No description provided.