Skip to content

Add derive Clone to high_level::Event#14

Merged
Percivalll merged 1 commit into
Percivalll:masterfrom
Foosec:patch-1
Oct 30, 2025
Merged

Add derive Clone to high_level::Event#14
Percivalll merged 1 commit into
Percivalll:masterfrom
Foosec:patch-1

Conversation

@Foosec
Copy link
Copy Markdown
Contributor

@Foosec Foosec commented Oct 29, 2025

All the members already implement Clone and it shouldn't be problematic to copy a FD

@Percivalll Percivalll merged commit a17bf04 into Percivalll:master Oct 30, 2025
1 check passed
@pantsman0
Copy link
Copy Markdown
Collaborator

@Foosec please be aware that cloning the fd is fine, but closing the fd will invalidate it for the any other clones of the Event. A proper implementation can be done with dup2, but realistically we should be implementing Drop on Event too to close the fd as the caller is responsible for making sure the fds don't leak.

@Percivalll
Copy link
Copy Markdown
Owner

@Foosec please be aware that cloning the fd is fine, but closing the fd will invalidate it for the any other clones of the Event. A proper implementation can be done with dup2, but realistically we should be implementing Drop on Event too to close the fd as the caller is responsible for making sure the fds don't leak.

I noticed that in the read_event method (line 209), the event's fd is already being closed with close_fd(metadata.fd) immediately after the Event is created. This means by the time the Event is returned to the caller, the fd is already invalid. Therefore, cloning the Event (which just copies the i32 value) shouldn't cause the issue mentioned above - there's no risk of one clone closing the fd and invalidating it for other clones, since the fd is already closed when the Event leaves read_event.

So implementing Clone for Event should be safe in the current implementation.

That said, using dup2 and implementing Drop to allow users to control the fd lifetime based on the Event's lifetime is a good idea, but it would be a breaking change that requires further discussion.

@pantsman0
Copy link
Copy Markdown
Collaborator

Thank you so much for mentioning that. I've actually been revisiting this project since tuesday and I've been going crazy because I can't read the content in the openned files even with FAN_CLASS_CONTENT set. But of course that won't work because the handle is already closed.

@pantsman0
Copy link
Copy Markdown
Collaborator

@Foosec I've openned up #15 if you want some input into shaping the API.

@Foosec
Copy link
Copy Markdown
Contributor Author

Foosec commented Oct 31, 2025

Hello!

Sorry i was a bit out of commision the last few days,
on this topic i spent a couple nights debugging some odd behaviour:

Trying to use FAN_OPEN_PERM for scanning the file (opening it again in my process whilst holding the initial event blocked) that the FD that comes from the new open() call is the same as the initial one, it seems to me that its caused by closing the file descriptor in read_event() which makes it invalid (or rather re used) for the subsequent send_response
causing it to allow/deny the wrong event!

@pantsman0
Copy link
Copy Markdown
Collaborator

Hi Foosec,

Yeah, I was actually thinking about this when I was writing #15 . That should now keep the handle open the appropriate amount of time

@Foosec
Copy link
Copy Markdown
Contributor Author

Foosec commented Nov 1, 2025

Ah nice! I read the PR just seems i did it very badly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants