magiclantern_simplified icon indicating copy to clipboard operation
magiclantern_simplified copied to clipboard

Module exports can be called before their init function has completed

Open reticulatedpines opened this issue 10 months ago • 0 comments

Spotted with dual_iso.

shoot.c called registered cbrs:

#if defined(CONFIG_MODULES)
        module_exec_cbr(CBR_SHOOT_TASK);
#endif

These are listed at the end of dual_iso.c:

MODULE_CBR(CBR_SHOOT_TASK, isoless_refresh, CTX_SHOOT_TASK)
MODULE_CBR(CBR_SHOOT_TASK, isoless_playback_fix, CTX_SHOOT_TASK)

Also in dual_iso.c, isoless_init() creates isoless_sem. But the shoot.c code can trigger before this, and isoless_refresh() did take_semaphore(isoless_sem, 0); with no guard for null pointer. On old cams, this is non-fatal, returning an error (which we didn't check for), but on new cams this triggers an OS assert, hanging the cam.

We might want to wrap take_semaphore() to do the null check before calling, it's an easy way to hang the cam.

It's highly unintuitive that the various module init functions don't run first. We might want module loading to have some globally visible manner to say when the init functions are finished, probably a semaphore that module_exec_cbr() would wait on. We should check for other routes that can call module exports, and have some appropriate guard there, too.

reticulatedpines avatar Apr 25 '24 16:04 reticulatedpines