FDInputStream: add FDStreamBuf destructor to stop fd leak.

[semenzato]

As mentioned in DeviceBase.cpp in setConnectType(), FDInputStream (stdio_filebuf) is responsible for closing the file descriptor returned by sysfs_device.openAttribute().. This patch closes the fd.

[semenzato]

Thank you for looking into this and please forgive mistakes---it's my first GitHub pull request... ;)

[semenzato]

(I updated this commit with git push --force to include the fixes clause.)

[semenzato]

@semenzato I didn't see this because it didn't reference #666 or CC me anywhere but great to have now, thank you! 👍

I did find keyword override to need >=C++11 only which is good. I'm having trouble finding a definite answer whether override marks the new destructor virtual implicitly also, so far. I'm approving with that question unanswered.

CC @Cropi

Hello and sorry for the delay!

Unfortunately my C++ sucks. However this appears to be correct, override implies virtual and at least here (Google) both the style guide and the linter recommend to NOT use both. (To be sure, I also checked that std::streambuf is an alias for std::basic_streambuf, which defines a virtual destructor.) So we should be OK! But I am not sure about backwards compatibility issues, if any.

[hartwork]

@semenzato thanks for the update! We have no source suggesting to add both currently and it has been discussed now so everything is ensured to be conscious, so I'm good with moving forward with status quo, personally :+1:

[semenzato]

I am not sure if there is anything else I can do here to get this done. Thanks!

[hartwork]

@semenzato there is a comment from @Cropi at https://github.com/USBGuard/usbguard/issues/663#issuecomment-2889849545 that he will need a few days to recover first. I'm hoping for review and a merge from him after his return.

[Cropi]

I think the use of override is fine as std::streambuf... indeed has a virtual destructor. I think it's mainly cosmetics, but still :smiley: What if HAVE_EXT_STDIO_FILEBUF_H is defined and we have a different implementation of FDInputStream, don't we need to update it as well?

[hartwork]

@Cropi interesting point. My C++ is a bit rusty (..), I think the final answer needs these sub-questions ansered:

1. Does _filebuf_ptr get destroyed without an explicit desctructor in class FDInputStream?
2. Does std::unique_ptr call delete on its data even without an explicit deleter? Answer seems to be: the default deleter does call delete for us (please validate).
3. Does stdio_filebuf close the file descriptor in its destructor? The answer seems to be yes, it does.

@Cropi @semenzato what do you think?

[semenzato]

I wish I could be of more help, but my C++ knowledge is also quite superficial. I'll see if I can find the time to dig in a bit more.

I haven't tried to build this under different configurations (such as having HAVE_EXT_STDIO_FILEBUF), so I suppose that this patch could break those. I wonder if there is some kind of automated pre-submit or post-submit test? Or would the reviewer/maintainer ask me to do that?

The other question is whether this doesn't break anything now, but it's a kludge which might require changes in future configurations.

Is it common to get no responses from the maintainer for weeks? What happens when the maintainer stops maintaining?

Thanks!