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

(c2rust-analyze) Add almost all libc KnownFns used in lighttpd_rust_amalgamated #998

Merged
merged 13 commits into from
Jul 27, 2023

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jul 19, 2023

This adds known_fns! annotations for all libc fns used in lighttpd_rust_amalgamated with a few exceptions:

  • vararg fns aren't yet supported (support for some of them is a WIP)
  • libc fns that are not in the libc crate for some reason (will find them)
  • fns with fn ptr args, as I'm not certain how to handle them yet
  • non-libc fns used by lighttpd_rust_amalgamated (pcre2 fns)

I've left TODOs for these, as well as for some other ptr perms I wasn't certain about and would like to have reviewed.

I've also outlined in #865 (comment) how we might support some of these exceptions.

…gamated` to `all_known_fns`, un-annotated.

There are a lot of `fn`s, so this just adds them unannotated so far.
Since `known_fns!` checks the number of perms matches the number of pointers,
new `fn`s with pointers are commented out for now.

A few `libc` `fn` names are included as comments
as they are for some reason not in Rust's `libc` crate, but are a part of `libc`.

The other foreign `fn`s in `lighttpd_rust_amalgamated` are `pcre2` `fn`s, not `libc` ones.
…ble, with a few exceptions:

* Return types that are declared as `*mut` but are not supposed to be written
  as they may be in statically allocated memory that will be overwritten.
* Vararg `fn`s.
* `libc` `fn`s that are not in the `libc` crate for some reason.
* `fn`s with `fn` ptr args.
* Non-`libc` `fn`s used by `lighttpd_rust_amalgamated` (`pcre2` `fn`s).
… statically allocated memory that aren't meant to be modified, mark them as `READ`, not `READ | WRITE`. They are also commented as such.
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.

Looks good overall.

Comment on lines +643 to +646
fn calloc(
nobj: size_t,
size: size_t,
) -> *mut c_void: [READ | WRITE | OFFSET_ADD | FREE];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the return type here should actually have OFFSET_SUB in addition to OFFSET_ADD. It's legal and reasonable to do things like calloc(...).offset(1).offset(-1), and it seems difficult to specify something like "you can do OFFSET_SUB on this pointer, but only if you do an OFFSET_ADD first".

(Though really calloc should probably be special-cased along with malloc, realloc, etc at some point.)

String arguments should stick with just OFFSET_ADD to be more permissive (the details of OFFSET_ADD vs OFFSET_SUB vs both doesn't matter here anyway, since it's all the same once it's been cast to a raw pointer). Returned strings and arrays should also get OFFSET_SUB, though.

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 think the return type here should actually have OFFSET_SUB in addition to OFFSET_ADD. It's legal and reasonable to do things like calloc(...).offset(1).offset(-1), and it seems difficult to specify something like "you can do OFFSET_SUB on this pointer, but only if you do an OFFSET_ADD first".

Why couldn't we have something like this for offset?

fn offset<T>(self: *const T: [OFFSET_ADD], count: isize) -> *const T: [OFFSET_ADD | OFFSET_SUB];

(Though really calloc should probably be special-cased along with malloc, realloc, etc at some point.)

I think it already is. I just added all of the libc fns used by lighttpd_rust_amalgamated for consistency, even if they are already builtins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In dc4c467, I made the fns that internally do .offsets return OFFSET_SUB, which I think is clearly correct (e.x. for strchr). I'm still not sure about the others, though. From what I understood, if I pointer has only OFFSET_ADD, it can be made into a slice. But if it has OFFSET_SUB, too, then it can't become a slice. When a fn returns a pointer to the beginning of memory that could be a slice, why would we add OFFSET_SUB?

c2rust-analyze/src/known_fn.rs Show resolved Hide resolved
c2rust-analyze/src/known_fn.rs Show resolved Hide resolved
c2rust-analyze/src/known_fn.rs Outdated Show resolved Hide resolved
c2rust-analyze/src/known_fn.rs Outdated Show resolved Hide resolved
…y say `VALID` for future greppability if/when we add `PermissionSet::VALID`.
@kkysen kkysen requested a review from spernsteiner July 26, 2023 20:22
@kkysen kkysen merged commit 339629c into master Jul 27, 2023
9 checks passed
@kkysen kkysen deleted the kkysen/analyze-known-fn-decls branch July 27, 2023 21:37
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.

(c2rust-analyze) Hardcode ptr permissions and effects of known std and libc functions
2 participants