Skip to content

Add experimental stub_syscalls config option#3332

Closed
stevenengler wants to merge 1 commit intoshadow:mainfrom
stevenengler:stub-config
Closed

Add experimental stub_syscalls config option#3332
stevenengler wants to merge 1 commit intoshadow:mainfrom
stevenengler:stub-config

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented May 15, 2024

In #3280 we discussed allowing users to add stubs for features that Shadow is missing. There are a lot of different features that users may want to add stubs for (ex: syscalls, socket options, ioctl commands, misc syscall flags, etc).

This PR adds an experimental stub_syscalls (or --stub-syscalls) configuration option that allows you to list syscall numbers that you wish to add stubs for. This only covers a small part of #3280. I'm thinking that we could cover the rest of #3280 with a second stub_features option or something. But it might make sense to handle syscalls separately with their own stub_syscalls option.

This new option takes syscall numbers instead of names so that users can specify syscalls that shadow doesn't know about. For example if a new Linux version introduces a new syscall and we haven't added it to linux-api yet. It also helps prevent mistakes from misspellings or differences between linux syscall names and libc function names.

experimental.stub_syscalls

Default: []
Type: Array of Integer

List of syscall numbers to add "return 0" stubs for.

If any of the listed syscalls are called by the managed application, and Shadow does not provide
its own implementation of that syscall, Shadow will simply return 0 from the syscall instead of
ENOSYS.

@robgjansen Wdyt? I'm open to alternative designs.

@stevenengler stevenengler added this to the Support missing syscalls milestone May 15, 2024
@stevenengler stevenengler self-assigned this May 15, 2024
@github-actions github-actions bot added Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable Component: Documentation In-repository documentation, under docs/ labels May 15, 2024
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable improvement to me!

One concern is that there are stubs that require more than just return 0, but maybe there are not that many of those so adding manual stubs for them would not be too onerous? This does make me wonder for how many of our missing syscalls does return 0 make sense...

Log the syscalls for each process to individual "strace" files [default: "off"]

--stub-syscalls <syscalls>
List of syscall numbers to add "return 0" stubs for. [default: []]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have any stubs already in Shadow that we would want to remove but then set as default in this configuration option instead (i.e., so that it goes through the new stub logic)? Or maybe we just want to keep those in place, because if the default for --stub-syscalls is x,y,z but the user has a new syscall 666 that they want Shadow to treat as a stub, they would probably get confused and accidentally drop the defaults by writing --stub-syscalls=666 instead of --stub-syscalls=x,y,z,666?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see any existing syscall stubs in Shadow that we'd want to use this for. But we might want to add another config option in the future for adding stubs for socket options, and in that case we probably would want to add defaults for for those socket options.

they would probably get confused and accidentally drop the defaults by writing --stub-syscalls=666 instead of --stub-syscalls=x,y,z,666

Yeah, this is similar to the --args option of tornettools simulate, where adding new args drops the default args. I'm not sure what the solution to this is.

stevenengler added a commit that referenced this pull request Jun 8, 2024
This pulls the `OnceSet` commit out of #3332. It moves the locking
correctness logic to its own type. This also adds changes for some
review comments from #3332.
@stevenengler
Copy link
Copy Markdown
Contributor Author

stevenengler commented Jun 9, 2024

The race condition bug was fixed in #3338 and #3339, and the OnceSet type was added in #3349, so those commits have been removed from this PR.

@stevenengler
Copy link
Copy Markdown
Contributor Author

One concern is that there are stubs that require more than just return 0, but maybe there are not that many of those so adding manual stubs for them would not be too onerous? This does make me wonder for how many of our missing syscalls does return 0 make sense...

I can't think of any syscalls that someone might want to stub which don't return 0. Any syscall I can think of which returns a non-zero value typically needs to do something to arrive at the returned value, and I don't see how returning a constant value would be useful. My thinking is that it would be easiest to just support returning 0 for now, and then if someone has a more complicated use case, we could change the option somehow to support that as well.

@stevenengler
Copy link
Copy Markdown
Contributor Author

I'm closing this for now since I think there are still some open questions, especially if we want to also include stubs for socket options and ioctls and have them be consistent with syscall stubs. I don't have any immediate plans to work on these other stubs, and it would be better to have some cohesive design for them. Anyone can reopen or copy these changes to a new PR if they want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Documentation In-repository documentation, under docs/ Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants