search

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

audit take_semaphore() usage, potential DryOS API change #102

Closed reticulatedpines closed 1 month ago

reticulatedpines commented 1 year ago

I think critix might have spotted this. Here's my comment from dual_iso.c:

    // SJE TODO gain better understanding of why modern digic cams,
    // apparently including 1300D, fail to take the sem if 2nd param is 0.
    // On prior cams, 0 works fine.  But there are other uses with 0 on D678
    // that seem okay!  E.g. the menu sem.
    take_semaphore(isoless_sem, 1);

isoless_sem is one we make ourselves, so I'd guess this is an API change on Canon's side.

We should audit all our uses of take_semaphore() and see if the problem happens elsewhere. Potentially this could break a lot of things.

reticulatedpines commented 1 month ago

This was a combination of mistakes. take_semaphore() hasn't changed behaviour here, but we were mistakenly creating the sem locked, so it would deadlock if you called the first take with 0 as 2nd param, since that is "wait forever".

As part of the fix I introduced SEM_CREATE_LOCKED and SEM_CREATE_UNLOCKED enums for create_named_semaphore(), to make it harder to get this wrong in future - and make the code clearer to read.