Switching to using memfd for input data #990
Conversation
Codecov Report
@@ Coverage Diff @@
## master #990 +/- ##
==========================================
+ Coverage 80.83% 83.91% +3.08%
==========================================
Files 139 142 +3
Lines 4716 4905 +189
==========================================
+ Hits 3812 4116 +304
+ Misses 904 789 -115
Continue to review full report at Codecov.
|
quantum5
force-pushed
the
memfd-judge
branch
8 times, most recently
from
4b68ee6
to
5688538
Compare
quantum5
force-pushed
the
memfd-judge
branch
2 times, most recently
from
71f78a6
to
e30f7b1
Compare
| super().__init__(memory_fd_create(), 'r+') | ||
| _name: Optional[str] = None | ||
|
|
||
| def __init__(self, prefill: Optional[bytes] = None, seal=False) -> None: |
There was a problem hiding this comment.
Maybe one or both of these should be required kwargs. I'm thinking the second should. What is the difference between prefilling with nothing, and passing None?
| with NamedTemporaryFile(delete=False) as f: | ||
| self._name = f.name | ||
| super().__init__(os.dup(f.fileno()), 'r+') | ||
| else: |
There was a problem hiding this comment.
Do we need to dup and specify delete=False ?
There was a problem hiding this comment.
Definitely. Otherwise the fd gets closed or the file gets unlinked, respectively.
| if e.errno == errno.ENOSYS: | ||
| # FreeBSD | ||
| self.seek(0, os.SEEK_SET) | ||
| return |
There was a problem hiding this comment.
I'm not quite sure what this does. Does it deserve more of a comment?
| self._input_data_fd = self._make_input_data_fd() | ||
| return self._input_data_fd | ||
|
|
||
| def _make_input_data_fd(self): |
There was a problem hiding this comment.
Maybe this needs to be typed to return MemoryIO now?
There was a problem hiding this comment.
Any file object will do, MemoryIO is just a special hack.
| @@ -306,7 +306,7 @@ int memory_fd_create(void) { | |||
| #ifdef __FreeBSD__ | |||
There was a problem hiding this comment.
Actually, is this function called on FreeBSD anymore? Are you creating the tempfile in Python instead?
| try: | ||
| os.dup2(new_fd, fd) | ||
| finally: | ||
| os.close(new_fd) |
There was a problem hiding this comment.
Maybe we could use a comment about why this dup is needed. Also, why isn't it implemented in the C function?
There was a problem hiding this comment.
C code is just that much more painful to maintain.
| @@ -21,10 +21,10 @@ def get_fs(self): | |||
| def get_allowed_syscalls(self): | |||
| return super().get_allowed_syscalls() + ['fork', 'waitpid', 'wait4'] | |||
|
|
|||
| def get_security(self, launch_kwargs=None): | |||
| def get_security(self, launch_kwargs=None, extra_fs=None): | |||
There was a problem hiding this comment.
Not that this is a direct result of this PR, but maybe this should be converted to **kwargs instead?
Breaking changes:
_interact_with_processThis closes #835.