angr / simuvex

[DEPRECATED] A symbolic execution engine for the VEX IR
BSD 2-Clause "Simplified" License
79 stars 57 forks source link

Feature: Multi Open a File #108

Closed ekilmer closed 7 years ago

ekilmer commented 7 years ago

See issue #85 for more info.

The important stuff is in the modification to posix.py. I think we should add a file to the fs dictionary after we open it so that future calls to open will not try to create a separate file with different contents.

As of now this doesn't work on a few of the tests in angr (test_file_struct_funcs in particular). I was hoping for some input on how to fix these.

The test_file_struct_funcs failure looks like it is from the way I flush the file contents of file.txt when it is closed on 238. Ignore the fact that we will no longer be able to access the fd to the file--we'll have to change that to dump a closed file's contents by path. The objective was to 'save' the file modifications into the fs but if we try to read the file's contents at the end (after paths have deadended), the file's contents can't be read due to the file.content weakly-referenced object no longer existing. If there was some way to keep a file's contents stored in the fs even at the end of an exploration, then it shouldn't be too difficult to check the contents.

Here is the test C file and script that works with my changes:C program and python test script. The test C program was taken from the glibc test for fdopen function.

Thank you!

P.S. I'm not sure if this was the best way to organize this, so please let me know if you have some organization tips. The new simprocedures are from the test C program.

ekilmer commented 7 years ago

So I might have a potential solution that passes all (modified) tests.

We would have to modify both test_file_struct_funcs.py and test_echo.py to dump the closed file contents by replacing state.posix.dumps(fd) with something like the following using the file's path:

state.posix.dump_file_by_path('/dev/stdout')

See PR in angr for test changes and binaries for new test binary

ekilmer commented 7 years ago

One issue that will arise is to check whether the file has been closed (read: synchronized) with the fs. Currently, we do not synchronize the modified contents of an open file with the fs until it is closed, so if you try to dump_file_by_path('file.txt') and the program hasn't ever closed it, then you will likely get an empty string until the file is closed (using this method), however accessing its contents through its file descriptor, dumps(3) would give correct results.

zardus commented 7 years ago

It makes me nervous that we replace the reference in the state.posix.fs dict when we open a file of the same name, so (for example) opening two files in append mode would cause unexpected behavior. Not that this works as things are right now anyways, so it's kinda a non-issue with regards to this PR.

My understanding for the reason that the test cases have to be modified is that the files are actually closed in the testcase, but the file descriptor was never removed before. Is that the case? If so, I'm not against switching it to dump_file_by_path.

ekilmer commented 7 years ago

That is correct. When the file is closed now, the file descriptor entry disappears and the contents are updated in the fs.

There could also be an immediate update to the fs contents on a write or other file modification, but I think that's another design decision.

That's a great point, opening two files in append mode. I haven't looked too deep into the libc implementation, but yes, I would think that even without this PR, there would be issues with that scenario.

I think a file may be locked during write operations in the libc implementation, and that might take care of appending, but I'd have to look into it more.

Thank you!

ekilmer commented 7 years ago

@zardus Any updates on this? Would you like me to resolve the "append" issue before merging?

zardus commented 7 years ago

I haven't had a chance to pull this in and run CI on it all. That cross-repo CI support might be a good long-term investment :-)

I'll try to push this up my TODO list!

ekilmer commented 7 years ago

Okay, no worries! It doesn't seem like there's too much other activity with file support, so this should stay conflict-free for a bit longer fingers crossed :-)

zardus commented 7 years ago

Thanks!