Skip to content

Add context to strace output when we can't read memory#2821

Merged
robgjansen merged 2 commits intoshadow:mainfrom
stevenengler:readv-writev
Jan 3, 2024
Merged

Add context to strace output when we can't read memory#2821
robgjansen merged 2 commits intoshadow:mainfrom
stevenengler:readv-writev

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

From #2815 (comment).

If the strace logger is unable to read some memory, a <invalid> tag will be added after the pointer. This matches when an invalid flag is passed to a syscall.

There are two downsides to this change.

  1. Some syscall arguments allow NULL values, but the strace logger will show a NULL as "Invalid". For example this occurs with the address length pointer argument to recvfrom.
  2. We don't always try to read pointers, so some pointers might be invalid but we never try to read them and they never get the "invalid" tag.

@stevenengler stevenengler requested a review from sporksmith March 30, 2023 20:20
@stevenengler stevenengler self-assigned this Mar 30, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Mar 30, 2023
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith left a comment

Choose a reason for hiding this comment

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

re the downsides you mentioned, we could special-case 0 to print "NULL" instead of "invalid", and/or use something like "unreadable" instead of "invalid"

I'm also fine with merging this as-is, though.

@robgjansen
Copy link
Copy Markdown
Member

After discussion, Rob will take over this PR. Here are the cases:

  • we have a NULL ptr, we wont read it
  • we have a real pointer, we tried to read but it is somehow invalid and can't be read
  • we have a real pointer, but we decided not to read it because of its type
  • we have a real pointer, and we read it because of its type, and we have some data

Rob will come up with tag names for these cases.

@robgjansen
Copy link
Copy Markdown
Member

@stevenengler I don't know if we still want this, or maybe I chose bad pointer suffix names, but here is my try at progressing this PR. I'm OK if we decide that it's not enough of an improvement and just want to close it instead.

Copy link
Copy Markdown
Contributor Author

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

@robgjansen robgjansen force-pushed the readv-writev branch 2 times, most recently from 3fc28af to 7171a18 Compare January 3, 2024 14:31
@robgjansen robgjansen changed the title Add <invalid> in strace output when we can't read memory Add context to strace output when we can't read memory Jan 3, 2024
@robgjansen robgjansen enabled auto-merge January 3, 2024 14:34
@robgjansen robgjansen merged commit 348bc4c into shadow:main Jan 3, 2024
@stevenengler stevenengler deleted the readv-writev branch January 3, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants