[sandbox] Filter out fewer SEGV_ACCERR crashes in the CrashFilter [v8/v8 : main]
Samuel Groß (Gerrit)
Samuel Groß added 2 comments![]( "Open in Gerrit")
if (fd_ < 0) return std::nullopt;Samuel GroßCan we make this a CHECK or DCHECK? Currently all reads are gated behind a `IsValid` check, so I think we can make that a requirement.
Done
Related details
- Igor Sheludko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko voted and added 4 comments![]( "Open in Gerrit")
Votes added by Igor Sheludko
| Code-Review | +1 |
4 comments
lgtm with nits
/proc/self/maps. Ideally, we would be able to check that we crashed on a
known mapping instead, but this can be implemented in follow-up CLs.
It feels like an way easier solution to implement than parsing mem file: there are only two known cages+guard regions + tables where any SEGFAULT is allowed. Everything else might be a sandbox escape with enough luck.
: fd_(open("/proc/self/maps", O_RDONLY)), buffer_pos_(0), buffer_end_(0) {}Ideally, it would be nice to unify this code with another "maps" parsing code in this file.
```
// The maps file consists of the following kind of lines:
// 55ac243aa000-55ac243ac000 r--p 00000000 fe:01 31594735 /usr/bin/head
```
Related details
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Groß (Gerrit)
Samuel Groß added 4 comments![]( "Open in Gerrit")
/proc/self/maps. Ideally, we would be able to check that we crashed on a
known mapping instead, but this can be implemented in follow-up CLs.
It feels like an way easier solution to implement than parsing mem file: there are only two known cages+guard regions + tables where any SEGFAULT is allowed. Everything else might be a sandbox escape with enough luck.
Yeah this is basically what the next CL in the chain does. I don't want to add thread-safe tracking (with signal-safe access) for all the pointer tables and similar memory regions we allocate (as these are per-Isolate) just for the sandbox testing mode. Also I think it'd be nice if the regions get names in /proc/self/maps. So with the follow-up CL, we now look for these names to determine where we crashed. But then I still need the Maps parser, so I implemented it in that order.
: fd_(open("/proc/self/maps", O_RDONLY)), buffer_pos_(0), buffer_end_(0) {}Ideally, it would be nice to unify this code with another "maps" parsing code in this file.
Done :D now the CL is larger, but at least there's no longer any code duplication. WDYT?
```
// The maps file consists of the following kind of lines:
// 55ac243aa000-55ac243ac000 r--p 00000000 fe:01 31594735 /usr/bin/head
```
- Igor Sheludko
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
1 comment
- Igor Sheludko
- Samuel Groß
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |