This bugfix actually opens a new opportunity: given that it is now clear that arbitrary code can execute during caml_alloc, we could decide to check for signals in caml_alloc and therefore unify the signal handling mechanism in bytecode and native mode.

What would you think of this idea?

Instead of relying on saving global variables can't we turn them into a record that is passed to all the functions?

Instead of relying on saving global variables can't we turn them into a record that is passed to all the functions?

This would certainly have some performance penalty (an additional parameter for every single function of the input machinery), and would require quite a large refactoring.

By the way, note that even with my fixed version, if the allocation function raises an exception (via a finalizer), then intern_cleanup will not be called, and we will leak memory.

Not sure how acceptable this is! The only fix I can think about is not trying to allocate in the minor heap in intern_alloc.

In the backtrace-related PR I wondered if it would be possible to have a function that allocates in the minor heap without calling the finalizers (by keeping finalizer-attached blocks in the minor heap), or fails if not possible. But that would be a major undertaking, if I understand the runtime correctly.

In the backtrace-related PR I wondered if it would be possible to have a function that allocates in the minor heap without calling the finalizers (by keeping finalizer-attached blocks in the minor heap), or fails if not possible. But that would be a major undertaking, if I understand the runtime correctly.

That's actually not so difficult: There already is such a mechanism (in order to prevent finalisers being called from finalisers). It suffices to play with running_finalisation_function (and possibly renaming it). The only complication is that we will have to call caml_final_do_calls in e.g., caml_check_urgent_gc to prevent finalisers to be delayed too much.

What would you think of that?

Looks good to me!

Before merging, I'd like to get @xavierleroy to weigh in on the comment change in signals_nat.c - it comes from here. I think the change is correct: there's now no need to distinguish between a GC that can context-switch and a GC that cannot since, due to signal handlers and finalisers, anything bad that can happen during a context-switch can already happen without one.

Actually, because of the possibility of a finaliser raising an exception even the fix implemented in this PR is not fully correct. See my comment at #8674 (comment). Hence, my new proposal would be, as suggested by @gasche, to have a new version of Alloc_small, which is not allowed to call finalizers. See #8674 (comment).

What would you think of that?

(I'm not qualified to answer this question but the general idea sounds quite nice to me.)

I see two seperate issues here:

1. Finalisers and signal handlers can call functions from Marshal during intern_alloc, which means they can re-enter intern.c, corrupting global state.

2. Finalisers and signal handlers can raise exceptions during intern_alloc, which means they can cause intern_alloc to terminate early without cleaning up properly.

This PR fixes 1. I think the fix is correct, and I'd like to merge it.

I think issue 2 is a seperate problem. To me, the problem is that finalisers and signal handlers should not be allowed to raise exceptions - I suspect this causes resource-handling problems elsewhere in the runtime and it certainly causes resource-handling problems in user code. Fixing it just for intern_alloc by adding a special form of Alloc_small does not seem like the right solution. (I talked a little at the recent dev meeting about an alternative solution to this problem: essentially, raising exceptions from a signal handler or finaliser should be a fatal error, and there should be a different mechanism for handling Ctrl-C).

Alright, then, what about deactivating callbacks from Alloc_small and friends when called from C code? I discussed with @damiendoligez, who agrees.

I also agree with @stedolan on the principle that exceptions should not escape from finalisers and signals handlers. But I think the exceptions should be caught and ignored, rather than turned into fatal errors.

Alright, then, what about deactivating callbacks from Alloc_small and friends when called from C code? I discussed with @damiendoligez, who agrees.

That makes sense to me!

Alright, then, what about deactivating callbacks from Alloc_small and friends when called from C code? I discussed with @damiendoligez, who agrees.

Ah, but this is not so easy to do this for memprof callbacks. We could use the "postponing" mechanism, but then the callbacks could be called in a very different context...

@stedolan: in the last weeks or so I have the impression that I encountered several situations (coming from code from either you or @jhjourdan) where it would be convenient to allocate memory on the minor heap without having to protect the caller against arbitrary OCaml code executing -- in one case, the get_backtrace one, the solution you proposed was to allocate in the major heap instead specifically to avoid this problem, which raises non-obvious performance questions. Wouldn't it be nice to have enough control in the runtime to ask for an allocation that does not return control to OCaml code before the next check_urgent_gc() or dispatch() or something?

You seem to suggest that conceptually this is not a good approach, but it is not clear to me why. Would it make things harder for multicore? (I guess there the assumption would be that no OCaml code runs on the same domain -- thread-local values are preserved, but of course mutators may run in other places.)

I agree with your comment that the present PR is already an improvement and could be merged as-is. (After conflicts are fixed.) I would be happy if other people took care of merging, because I didn't look at the code here carefully.

Regarding statmemprof callbacks: yes, I think that postponing them would be the right thing -- for this discussed Alloc_small_silent function/macro. I'm not sure why this would delay things "too much" -- if you call caml_check_urgent_gc() or something at the end of the critical section that use this Alloc_silent feature, you know that no OCaml code has run before the postponed things get the chance to run. (It would make sense to run statmemprof callback first, where the temporal distance to the allocation point is important, and finalizers second, where the invocation delay matters less.)

I think we are discussing about two different approaches to the problem:

• To my understanding, @gasche is suggesting to have two different (set of) allocation functions depending on whether or not we allow then to trigger callbacks. In addition to making the API more complex, this solution will only fix the uses we specifically think about. It is very likely that other parts of the runtime and C binding have similar bugs (i.e., no proper cleaning/memory leaks in case of an exception during caml_alloc).

• I am suggesting to change the behavior of all allocation functions (and forbidding them to trigger a callback) as soon as they are called from C code. The main advantage is that C code using allocation functions will be fine (well, except in the case of OOM/stack overflow exceptions, but these exceptions are a problem by themselves). My main problem here is that memprof callbacks wil have to be postponed, and C code allocating in the minor heap usually don't call caml_check_urgent_gc.

in one case, the get_backtrace one, the solution you proposed was to allocate in the major heap instead specifically to avoid this problem,

Note that in that case, we wanted to prevent GC, not only arbitrary OCaml code. The solutions discussed here will not fix the callstack-in-memprof issue.

@gasche

in the last weeks or so I have the impression that I encountered several situations (coming from code from either you or @jhjourdan) where it would be convenient to allocate memory on the minor heap without having to protect the caller against arbitrary OCaml code executing

You seem to suggest that conceptually this is not a good approach, but it is not clear to me why.

I agree with what @jhjourdan says.

I was against the original idea of adding a separate version of caml_alloc_small which did not call finalisers, so that the new function could be used from intern.c. One of the major difficulties when working on the runtime or using the C API is the sheer number of different functions with different GC invariants and semantics, and so I am by default against any proposal that suggests adding more of them.

Afterwards, @jhjourdan suggested that in general, caml_alloc_small should never invoke finalisers (finalisers instead being postponed to some other point, such as an allocation from OCaml code or a call to caml_check_urgent_gc). This sounds like a good idea to me: it actually makes the API simpler to use, and adds no more options. As you say, this would resolve my difficulty in #8670.

@jhjourdan

Ah, but this is not so easy to do this for memprof callbacks. We could use the "postponing" mechanism, but then the callbacks could be called in a very different context...

Ah, you're right, this is a difficulty. I was just discussing this with @lpw25, and he suggested that one approach is to have an implicit caml_check_urgent_gc just after each non-noalloc C call. (I think this can be done with a single extra branch in caml_c_call, so I there shouldn't be significant overhead).

I implemented the change we are discussing here as part of #8691. Let's continue the discussion there.