fsevents: move free to finalizer

[gridbugs]

Fixes #6151

This just a copy of #6215 rebased onto main. I've tested this on both an M1 and x86 mac and it fixes the problem.

[gridbugs]

(copied from #6151)

I did some digging today into why the crash happens when the fsevents callback calls Fsevents.stop.

The c function dune_fsevents_callback is called on each event, and invokes the user-provided callback (the ~f argument to Fsevents.create) before dereferencing a field of the dune_fsevents_t struct . The c function dune_fsevents_stop deallocates the dune_events_t struct and lucky for us the deallocator seems to corrupt its pointer fields. dune_fsevents_stop is called by the ocaml function Fsevents.stop. So when the fsevents callback calls Fsevents.stop dune_fsevents_callback tries to dereference a field of the dune_events_t after its pointer fields have been corrupted and a segfault happens.

This implies that we'd also see this crash if we ever started, stopped, and then re-started a Fsevents.t as stop causes the value to be deallocated but it's only allocated inside create, though is moot as the Fsevents.state state machine doesn't allow stopping and re-starting.

@emillon's fix (#6215) solves the problem by removing the call to caml_stat_free(t); from dune_fsevents_stop and registering a custom finalizer for the dune_fsevents_t object instead. I've copied to #7312 and rebased onto main.

[emillon]

Sounds good of course but I think you could have pushed the original PR too.

My comments from there still apply - I'm not confident about the correctness of the memory management here but I think it's an improvement compared to the current situation (and it fixes the issue!).

Please add a changelog entry.

[gridbugs]

Sounds good of course but I think you could have pushed the original PR too.

I couldn't figure out how to push to your branch :)

My comments from there still apply - I'm not confident about the correctness of the memory management here but I think it's an improvement compared to the current situation (and it fixes the issue!).

Should we hold off merging this until we better understand the memory management?

Please add a changelog entry.

Done!

[emillon]

Should we hold off merging this until we better understand the memory management?

I'd say it's fine.

[rgrinberg]

I'd remove such details from the CHANGE list. It's supposed to make sense to end users that know nothing about dune's code. Perhaps something like:

- Fix segfault on MacOS when dune was being shutdown while in watch mode.

[rgrinberg]

@nojb could you take a look please?

[nojb]

LGTM

Another approach: using a custom block as in this PR, you can set the "data pointer" to NULL after deallocating the dune_fsevents_t data structure. The callback would check whether the field is NULL before trying to dereference it and not do anything otherwise. This avoids the use of the finalisation function.

[rgrinberg]

@gridbugs what do you think about my suggesting for the CHANGES entry?

[gridbugs]

@gridbugs what do you think about my suggesting for the CHANGES entry?

Looks good. I've updated CHANGES with your suggestion.