res_musiconhold: avoid moh state access on unlocked chan

[mbradeen]

Move channel unlock to after moh state access to avoid potential unlocked access to state.

Resolves: #133

[mbradeen]

cherry-pick-to: 20 cherry-pick-to: 18 cherry-pick-to: certified/18.9

[mbradeen]

ast_channel_music_state returns chan->music_state. One of our partners ran into issues on their live systems.

[seanbright]

(as an aside it would still be great to understand what this is actually intended to fix)

[jcolp]

I think you are saying that the channel lock is being used to protect state so you have to hold the channel lock while reading/writing to it.

Unfortunately there are numerous places where unprotected access to state occurs throughout the file (including a couple times near the top of the same function you are updating).

The documentation in channel.h indicates that the channel must be locked before ast_channel_music_state(...) is called which is not occurring in many places throughout this file.

A potential resolution is to give struct moh_files_state its own mutex instead of relying on the channel lock. You would still need to take the channel lock when calling ast_channel_music_state(...) and ast_channel_music_state_set(...) but any manipulation of the state itself could be down under its own lock.

Channel is locked when all generator callbacks are invoked, except for generate (which is on purpose according to a comment in the generator API code). That's why there is some locking in the generate callback. The other cases of starting/stopping MOH also have locking since they're outside of generator land, and appear to do it correctly.

So I think everything else is fine.

[seanbright]

OK so it is just the two cases of missing locks at the beginning of moh_files_generator(...) that need to be addressed (make that 3. The while loop condition needs protecting as well).

[mbradeen]

waiting on feedback from original reporter.

[seanbright]

So moh_files_generator(...) is the only function that reads/writes to the sample_queue member of struct moh_files_state so do we actually need it as part of the struct or can we just use a local variable in that function? That would simplify the locking a bit if that is the case.

[jcolp]

So moh_files_generator(...) is the only function that reads/writes to the sample_queue member of struct moh_files_state so do we actually need it as part of the struct or can we just use a local variable in that function? That would simplify the locking a bit if that is the case.

I believe it exists on state such that if the returned frame has a greater number of samples than is currently needed, it tries its best to not produce more on the next invocation. For example the generator is asked for 160 samples but gets back 320 samples, the next invocation then becomes a noop if 160 is requested again.

I'm not sure this happens in practice (though it may depending on underlying codecs).

[seanbright]

So moh_files_generator(...) is the only function that reads/writes to the sample_queue member of struct moh_files_state so do we actually need it as part of the struct or can we just use a local variable in that function? That would simplify the locking a bit if that is the case.

I believe it exists on state such that if the returned frame has a greater number of samples than is currently needed, it tries its best to not produce more on the next invocation. For example the generator is asked for 160 samples but gets back 320 samples, the next invocation then becomes a noop if 160 is requested again.

I'm not sure this happens in practice (though it may depending on underlying codecs).

The only nominal way out of moh_files_generator(...) is with state->sample_queue being 0 (or less than 0). If we leave for any other reason (return -1) the generator is stopped, so there wouldn't be a subsequent invocation.

[jcolp]

So moh_files_generator(...) is the only function that reads/writes to the sample_queue member of struct moh_files_state so do we actually need it as part of the struct or can we just use a local variable in that function? That would simplify the locking a bit if that is the case.

I believe it exists on state such that if the returned frame has a greater number of samples than is currently needed, it tries its best to not produce more on the next invocation. For example the generator is asked for 160 samples but gets back 320 samples, the next invocation then becomes a noop if 160 is requested again. I'm not sure this happens in practice (though it may depending on underlying codecs).

The only nominal way out of moh_files_generator(...) is with state->sample_queue being 0 (or less than 0). If we leave for any other reason (return -1) the generator is stopped, so there wouldn't be a subsequent invocation.

Right, in the case I mentioned state->sample_queue would go negative.

[jcolp]

I've given this some further thought and I think we should keep sample_queue on state to cover the scenario I mentioned, I'm not comfortable making it local and potentially changing that behavior for some people and causing unintendend consequences.

[github-actions[bot]]

Successfully merged to branch master and cherry-picked to ["20","18","certified/18.9"]