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.

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

Support intermediate casts in calls. #1022

Merged
merged 3 commits into from
Sep 18, 2023
Merged

Conversation

aneksteind
Copy link
Contributor

@aneksteind aneksteind commented Aug 29, 2023

In lighttpd there are intermediate casts where the final cast is an argument to memcpy or memset. Prior to this, only one cast to *libc::c_void was exempted, the final one. Now, all casts are skipped/allowed as long as the final cast serves as input to a function like memcpy or free or realloc. Also, any non-skipped casts into *libc::c_void are now allowed to support cast ancestry that crosses function call boundaries, as the intra-body analysis starting backwards from memset and memcpy does not capture these cases.

The contents of algo_md5.rs match exactly the transpiled output from lighttpd with no alterations made.

@aneksteind aneksteind force-pushed the lighttpd.cast.intermediate branch from b9cfa49 to c36931f Compare August 29, 2023 17:30
In lighttpd there are intermediate casts
where the final cast is an argument to memcpy.
Prior to this, only one cast was exempted, the final
one. Now, all casts are skipped/allowed as long as
the final cast serves as input to a function like
`memcpy` or `free` or `realloc`.
@aneksteind aneksteind force-pushed the lighttpd.cast.intermediate branch from 95ed4b4 to 8d57764 Compare August 29, 2023 20:15
}
Some(false) => {
if is_c_void_ptr(self.acx.tcx(), to_lty.ty) {
// allow casts to c_void
Copy link
Contributor Author

@aneksteind aneksteind Aug 29, 2023

Choose a reason for hiding this comment

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

@spernsteiner this feels off to me, but the logic to exempt ancestor casts to *libc::c_void got complicated when considering function call boundaries. Is there anything wrong with making exceptions for casts to *libc::c_void?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems okay for now. I'm hoping most of the void* cast logic will be superseded by the new pointee type analysis I'm working on.

@spernsteiner
Copy link
Collaborator

any non-skipped casts into *libc::c_void are now allowed to support cast ancestry that crosses function call boundaries

I'm not sure I understand what you mean here. Is this about MD5_Update, where the _input pointer is just passed (through several additional casts) to memcpy? Is the goal to treat MD5_Update(..., input as *const c_void, ...) similar to memcpy(..., input as *const c_void, ...), giving the same special treatment to the input as *const c_void cast?

Base automatically changed from test.lighttpd.md5 to master September 8, 2023 15:02
@aneksteind
Copy link
Contributor Author

aneksteind commented Sep 8, 2023

any non-skipped casts into *libc::c_void are now allowed to support cast ancestry that crosses function call boundaries

I'm not sure I understand what you mean here. Is this about MD5_Update, where the _input pointer is just passed (through several additional casts) to memcpy? Is the goal to treat MD5_Update(..., input as *const c_void, ...) similar to memcpy(..., input as *const c_void, ...), giving the same special treatment to the input as *const c_void cast?

@spernsteiner sort of, in particular we exempt the cast in MD5_Update(..., input as *const c_void, ...) because within MD5_Update the casted result is used in memcpy. But to avoid the analysis across boundaries or adding a new pointer permission that gets propagated backwards like free, any casts into a void pointer are now exempted from transmutability checks

@spernsteiner
Copy link
Collaborator

I took a look at this, but I'm not sure if this approach of extending CVoidCasts is sufficient to enable the rewrites we want. In particular, I think the proper safe rewrite for this code has MD5_Update taking _input: &[u8] and calling copy_from_slice in place of memcpy. I'll put some more thought into this and try to write something up soon.

Copy link
Collaborator

@spernsteiner spernsteiner left a comment

Choose a reason for hiding this comment

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

Let's go ahead with this approach for now, even if some of it may be superseded by future work.

}
Some(false) => {
if is_c_void_ptr(self.acx.tcx(), to_lty.ty) {
// allow casts to c_void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems okay for now. I'm hoping most of the void* cast logic will be superseded by the new pointee type analysis I'm working on.

Comment on lines 259 to 264
pub fn rv_place<'tcx>(rv: &Rvalue<'tcx>) -> Option<Place<'tcx>> {
use Rvalue::*;
match rv {
Use(op) => op.place(),
Repeat(op, _) => op.place(),
Ref(_, _, p) => Some(*p),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of this function? It doesn't seem like a very meaningful operation, since the Places it returns are used in all sorts of different ways depending on what kind of Rvalue was passed. For example, some Rvalues read from rv_place(rv), while others don't.

Looking at the use site below, it seems like this is part of the logic for propagating through multiple levels of casts, as in p as *const c_char as *const c_void. If that's the case, can this be reduced to the Cast case only, where the meaning is clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, now only addressing Use and Cast in a1fbda0

@aneksteind aneksteind merged commit 0ba4903 into master Sep 18, 2023
9 checks passed
@aneksteind aneksteind deleted the lighttpd.cast.intermediate branch September 18, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants