Skip to content

fsevents: move free to finalizer#7312

Merged
rgrinberg merged 1 commit intoocaml:mainfrom
gridbugs:fsevents-finalize
Mar 21, 2023
Merged

fsevents: move free to finalizer#7312
rgrinberg merged 1 commit intoocaml:mainfrom
gridbugs:fsevents-finalize

Conversation

@gridbugs
Copy link
Copy Markdown
Collaborator

@gridbugs gridbugs commented Mar 14, 2023 •
edited
Loading

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 gridbugs mentioned this pull request Mar 14, 2023
Closed
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Mar 14, 2023

(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.

@gridbugs gridbugs force-pushed the fsevents-finalize branch from 9cec062 to 1661847 Compare March 14, 2023 06:47
@gridbugs gridbugs requested review from emillon and rgrinberg March 16, 2023 05:08
@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Mar 16, 2023

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 gridbugs force-pushed the fsevents-finalize branch from 1661847 to 690cad8 Compare March 16, 2023 23:48
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Mar 16, 2023

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
Copy link
Copy Markdown
Collaborator

emillon commented Mar 17, 2023

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

I'd say it's fine.

rgrinberg
rgrinberg reviewed Mar 17, 2023
View reviewed changes
CHANGES.md Outdated
(#7139, @anmonteiro).

- Fix segfault on macos when `Fsevents.stop` is called inside the callback to
`Fsevents.create`. (#7312, fixes #6151, @gridbugs, @emillon)
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rgrinberg added this to the 3.8.0 milestone Mar 17, 2023
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Mar 17, 2023

@nojb could you take a look please?

nojb
nojb reviewed Mar 17, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gridbugs gridbugs force-pushed the fsevents-finalize branch from 690cad8 to a298f29 Compare March 20, 2023 01:19
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Mar 21, 2023

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

@gridbugs gridbugs force-pushed the fsevents-finalize branch from a298f29 to a04bb99 Compare March 21, 2023 01:45
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Mar 21, 2023

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

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

@rgrinberg rgrinberg added the macos Issues that relate to Dune on macOS label Mar 21, 2023
@emillon @rgrinberg
fix(fsevents): move free to finalizer
efcb00e 
turning off dune in watch mode on macos will no longer segfault
sporadically

Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit eed25bf into ocaml:main Mar 21, 2023
emillon pushed a commit to emillon/dune that referenced this pull request Mar 30, 2023
@gridbugs @emillon
fsevents: move free to finalizer (ocaml#7312)
66dd937 
turning off dune in watch mode on macos will no longer segfault
sporadically

Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
emillon pushed a commit to emillon/dune that referenced this pull request Mar 31, 2023
@gridbugs @emillon
fsevents: move free to finalizer (ocaml#7312)
5ef719d 
turning off dune in watch mode on macos will no longer segfault
sporadically

Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
emillon added a commit that referenced this pull request Mar 31, 2023
@emillon @gridbugs
fsevents: move free to finalizer (#7312) (#7452)
1ebef6b 
turning off dune in watch mode on macos will no longer segfault
sporadically

Signed-off-by: Etienne Millon <me@emillon.org>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Co-authored-by: Stephen Sherratt <stephen@sherra.tt>
@emillon emillon mentioned this pull request Apr 4, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 4, 2023
@emillon
[new release] xdg, stdune, ordering, ocamlc-loc, dyn, dune, dune-site…
d8807cd 
…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.1)

CHANGES:

- Fix segfault on MacOS when dune was being shutdown while in watch mode.
  (ocaml/dune#7312, fixes ocaml/dune#6151, @gridbugs, @emillon)

- Fix preludes not being recorded as dependencies in the `(mdx)` stanza (ocaml/dune#7109,
  fixes ocaml/dune#7077, @emillon).

- Pass correct flags when compiling `stdlib.ml`. (ocaml/dune#7241, @emillon)

- Handle "Too many links" errors when using Dune cache on Windows.  The fix in
  3.7.0 for this same issue was not effective due to a typo. (ocaml/dune#7472, @nojb)
@emillon emillon mentioned this pull request Apr 5, 2023
Closed
@gridbugs gridbugs deleted the fsevents-finalize branch October 11, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nojb nojb nojb left review comments

@rgrinberg rgrinberg rgrinberg left review comments

@emillon emillon emillon approved these changes

Assignees

No one assigned

Labels

macos Issues that relate to Dune on macOS

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

fsevents segfaulting under M1 mac

4 participants

@gridbugs @emillon @rgrinberg @nojb