reticulatedpines / magiclantern_simplified

A Git based version of Magic Lantern, for those unwilling or unable to work using Mercurial. The vast majority of branches have been removed, with those thought to be important brought in individually and merged.
GNU General Public License v2.0
142 stars 47 forks source link

Module exports can be called before their init function has completed #144

Open reticulatedpines opened 2 months ago

reticulatedpines commented 2 months ago

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.