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

Use typeguard in CI testing; fix many typing issues #1655

Closed
wants to merge 6 commits into from

Conversation

bradlarsen
Copy link
Contributor

@bradlarsen bradlarsen commented Apr 6, 2020

THIS IS A DRAFT PULL REQUEST; NOT READY YET

Fixes #1584.

@bradlarsen bradlarsen force-pushed the typeguard-pytest branch 4 times, most recently from d3617f7 to d4d344e Compare April 9, 2020 21:29
@bradlarsen bradlarsen force-pushed the typeguard-pytest branch 3 times, most recently from a51d759 to 31d1089 Compare April 16, 2020 22:37
@ehennenfent
Copy link
Contributor

One potential option for this is to copy ci.yml to typeguard.yml and then replace the first few lines of the file with the following:

name: Typeguard Checks
on:
  # Delete the following line before merging:
  pull_request:
  schedule:
    # Run every day at 1 AM
    - cron:  '0 1 * * *'

Then you can revert ci.yml to its original state. I think this may be a good compromise between not having any runtime type checking and having the tests for each PR take several hours.

Brad Larsen added 6 commits April 17, 2020 16:36
When a syscall is made from an emulated binary, Manticore uses the
syscall number to look up the appropriate Python method that models that
syscall, and then uses Python introspection to massage arguments to that
syscall model function as deemed appropriate.

Previously, the deprecated `inspect.getfullargspec` method was used to
determine the number of arguments to the model function, and whether or
not it takes varargs.

However, in addition to being deprecated, `inspect.getfullargspec` does
not peer through wrapper functions (which can be introduced, for
example, through decorator use).  This inability to peer through wrapper
functions was causing Manticore's syscall emulation to break when the
Typeguard plugin for pytest is used -- that plugin implicitly adds a
`@typeguard.check_types` decorator on every function & method in the
`manticore` package.

Now, we use the non-deprecated `inspect.signature` API to get the needed
information.  This API _does_ peer through wrapper functions, and it
also allows us to get rid of conditional logic around the inclusion of
`self` in bound methods.
Previously, open file descriptors could be mapped to any of the `File`,
`Directory`, `SocketDesc`, `Socket`, and `ProcSelfMaps` classes, which
didn't share a common supertype, and which didn't have consistent
interfaces.

Now, each of these classes implements the `FdLike` abstract base class,
which specifies a number of methods.

This overhaul additionally fixes several issues with the Linux support
code:

- A program that tried to ioctl on a socket would crash Manticore
- In several places, the error code returned by a failing syscall was
  incorrect
- Many file descriptor operations could crash Manticore when used on an
  already-closed descriptor
- Manticore state serialization / deserialization was incorrect for file
  descriptor types other than File or Socket
- In certain cases, `sys_open` / `sys_openat` could crash Manticore when
  trying to log a debug message
- In certain cases, `sys_readv` could crash Manticore, instead of
  correctly returning a negative int value
- `sys_sendfile` was broken due to a variable name typo
- In several places in Manticore’s Linux support code, incorrect type
  hints were present
`arg` is previously used with a string type, earlier in the method,
and where changed, was used with an int type.
@bradlarsen
Copy link
Contributor Author

The initial pull request here had a lot of changes in it, and I ended up splitting it into several smaller pieces, getting those merged into master (e.g. #1672 and #1673), and rebasing and force-pushing what was left here.

The changes left in this pull request that haven't been incorporated are primarily about actually enabling the typeguard pytest plugin when testing.

It would be good to revisit this one day! But other priorities take precedence now.

@bradlarsen bradlarsen closed this Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorporate typeguard pytest plugin
2 participants