search

konrad-kruczynski / elfsharp

Pure managed C# library for reading ELF, UImage, Mach-O binaries.
https://elfsharp.it
Other
151 stars 56 forks source link

Fixed file handle leaks #18

Closed bastianeicher closed 10 years ago

bastianeicher commented 10 years ago

After parsing an ELF file a few file handles are still open. This can be reproduced by trying to delete a file right after running ElfLoader.TryLoad(). This patch adds the missing using() Blocks.

konrad-kruczynski commented 10 years ago

Hi, I'll try to review the commit once I deal with my PC's disk failure...

Anyway, thanks for the input.

konrad-kruczynski commented 10 years ago

Finally I had some time to look at the patch. You definitely raised a good point here. I'm not sure about the whole implementation though. Couple things:

  1. I guess that file should actually be opened until the elf object obtained by Load is disposed. This is due to the fact that we're still relying on the disk file after loading. In case of deletion the elf object will be half usable, for example you would not be able to get the content of the section. What do you think? Naturally, the file should be opened with read sharing in mind.
  2. I see no point in moving the inner stream of binary reader to the using clause instead of the binary reader itself as it would also close the underlying stream: http://msdn.microsoft.com/en-us/library/system.io.binaryreader.close.aspx

What do you think?

bastianeicher commented 10 years ago
  1. Yes, making IELF derive from IDisposable sounds like the perfect solution to me as well.
  2. You're right, I hadn't though that change properly through.

In case you are curious, this is where I am using your library: https://github.com/0install/0install-win/blob/master/src/Publish/EntryPoints/PosixBinary.cs Zero Install is a cross-platform "stateless" package manager and I am currently working on a GUI packaging tool for it. I use ELFSharp to automatically detect ELF binaries (along side Windows EXEs, Python scripts, etc.).

konrad-kruczynski commented 10 years ago

I think this change should resolve the problem, unless there are some missing usings that I overlooked. Could you look at it and close the pull request if it is satisfying?

On your project: interesting idea :) BTW, it can be very surprising how the library one provides will be used by people. When I first started this project, I could not find useful scenario for it apart from the one that I needed. And it turned out there are plenty.

bastianeicher commented 10 years ago

Unfortunately there still seem to be leaking file handles. I will create some unit tests to reproduce my use case and try to narrow down what still needs to be disposed.

konrad-kruczynski commented 10 years ago

Have you managed to narrow down the case?

bastianeicher commented 10 years ago

Your new Dispose method closes the stream opened in ELF's constructor. However, the NoteData constructor opens its own stream (using the readerSource callback) which is not disposed.

konrad-kruczynski commented 10 years ago

I'm sorry, I could finally some time to look at the issue. Please look at the proposed fix. Does it resolve all the problems?

bastianeicher commented 10 years ago

Perfect, fixes all file handle leaks I encountered. Thank you. :)