ralphje / imagemounter

Command line utility and Python package to ease the (un)mounting of forensic disk images
MIT License
118 stars 36 forks source link

Updated _load_fsstat_data method to terminate at specified time. #19

Closed ldgriffin closed 7 years ago

ldgriffin commented 7 years ago

In my testing the fsstat subprocess was not being terminated after the allotted 5 seconds.

1077 if thread.is_alive(): 1078 # noinspection PyBroadException 1079 try: 1080 process.terminate()

"process" at this point was type = <class 'NoneType'>

I added self. to the process references and the subprocess was then able to be terminated after 5 seconds. After the change, self.process became type = <class 'subprocess.Popen'> as expected.

I haven't worked with threading so this may not be the proper way to fix this so feel free to suggest a more appropriate fix. It appeared as though the scope of process was within the thread.

ralphje commented 7 years ago

I think that a global process should work too.

Additonally, the CI fails on your commit ;)

ldgriffin commented 7 years ago

That was my first thought as well, and is a simple change.

ldgriffin commented 7 years ago

If I understand correctly, you mean have the process = None within the _load_fsstat_data rather than outside the method. If so that does not work in my testing. That was what I tried initially, rather than the "self.", but the variable inside the thread was not accessible for the process.terminate(). The only way I've been able to get it to work is by having the process declaration outside of the method.

Maybe I'm missing something.

ralphje commented 7 years ago

I'll look into it asap

ldgriffin commented 7 years ago

Removing the process = None from both the module and the method works. Just declaring the global process inside of the thread does it.

ldgriffin commented 7 years ago

I pushed the change from my latest comment for your review and the codecov/project failed. If it's not apparent, I haven't used these types of project tools, so I can't tell what the problem is. I'll hold off on anything else until you can review.

ldgriffin commented 7 years ago

After continued searching, it looks like the keyword nonlocal would work for this for python 3, but to keep 2.7 compatibility there are suggestions of using a mutable object such as a list to allow access to the object in the outer scope. https://stackoverflow.com/questions/8447947/is-it-possible-to-modify-variable-in-python-that-is-in-outer-but-not-global-sc. Any thoughts on this approach?

1028        plist = [] #process = None

1035                process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
1036                plist.append(process)

1080                process = plist.pop()
1081                process.terminate()
ralphje commented 7 years ago

I have provided a commit myself, since I also wanted to have tests for this 😄 . Thanks for the heads-up.

ldgriffin commented 7 years ago

Thanks, works great with my problem image file.