libct: always close m.dstFile in mountToRootfs #5024
Conversation
Signed-off-by: lifubang <[email protected]>
| defer dstFile.Close() | ||
| // "proc" and "sys" mounts need special handling (without resolving the | ||
| // destination) to avoid attacks. | ||
| m.dstFile = dstFile |
There was a problem hiding this comment.
The leak happened in here.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah I see. I was confused because the line you linked here cannot be a leak, but the re-open is the issue. That's quite tricky, thanks for finding it...
There was a problem hiding this comment.
Pull Request Overview
This PR moves the defer statement that closes m.dstFile from after the createOpenMountpoint call to the beginning of the mountToRootfs function. The intent is to ensure that m.dstFile is always closed regardless of which execution path is taken through the function.
- Relocated the defer cleanup for
m.dstFileto the function entry point for more comprehensive resource management
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cyphar
added
easy-to-review
backport/1.2-todo
lifubang
force-pushed
the
fix-fd-leak-in-mountToRootfs
branch
from
c932c56 to
b644a90
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| defer dstFile.Close() |
There was a problem hiding this comment.
I don't think it's necessary to remove this -- having the defer is clearer and Go allows for calling Close on the same *os.File multiple times without issue (they are no-ops).
There was a problem hiding this comment.
Yes, there’s no harm in keeping it. If removal isn’t necessary, I’ll revert to leaving it as is.
lifubang
force-pushed
the
fix-fd-leak-in-mountToRootfs
branch
from
b644a90 to
c932c56
Compare
|
I've released v0.5.2 and v0.6.1 of filepath-securejoin which includes the leak fix you sent -- do you want to consolidate all of the leak fixes into one PR so it's a bit easier to review? (For 1.2.x and 1.3.x I would update go-selinux to v1.13.1 so that we can downgrade filepath-securejoin to v0.5.2 -- for 1.4.x I think we can just use the newer one.) |
|
|
This has been cherry-picked into #5026, closing in favour of that. |
cyphar
removed
backport/1.2-todo
2 participants
No description provided.