[Core] switch_ivr: Ensure do_flush decrypts SRTP DTMF packets (bug-fix)

[AndyNewlands]

BUGFIX: switch_ivr.c do_flush() does not decrypt SRPT DTMF packets. Mostly, this results in invalid DTMF character which is discarded. However, it sometimes causes a valid (but spurious) DTMF character to be generated, causing intermittent, unexpected IVR behaviour. This PR fixes that.

[signalwire-ci[bot]]

Unit-tests failed: https://public-artifacts.signalwire.cloud/drone/signalwire/freeswitch/591/artifacts.html

[signalwire-ci[bot]]

Unit-tests failed: https://public-artifacts.signalwire.cloud/drone/signalwire/freeswitch/595/artifacts.html

[AndyNewlands]

Hi. I'm not sure why these unit tests are failing: uac_digest_leak_udp and uac_digest_leak_tcp To my knowledge I have not changed anything which should affect these (just do_flush(), in switch_rtp.c - should be un related). Can anyone please help me fix this? Thanks

[yois615]

@AndyNewlands I'm having the same errors on my PR as well. There's something about the test suite that's failing despite the code being viable. In my case, the exact same code passed once and failed again on uac_digest_leak_udp_ipv6.

[AndyNewlands]

Thanks for confirming @yois615 . I dunno how to fix. I posted a question about this to the FS DEV mailing list.. Not sure if I'll get a reply - the last activity in there was May 2022. Fingers-crossed...

[yois615]

You can rerun the checks by amending your commit timestamp and force pushing: git commit --amend --no-edit && git push --force

The better way to get responses for these kinds of issues is on the Slack channel in #freeswitch-dev.

[dragos-oancea]

this sort of core changes in RTP are hard to take without a proper unit-test, perhaps you can get inspired from https://github.com/signalwire/freeswitch/blob/master/tests/unit/switch_rtp_pcap.c and make an unit-test for this ? there are some other RTP related unit-tests in that dir in the tree.

[andywolk]

Some tests are flaky and we are constantly working on improving that. We always check if a PR is a cause for a unit-test fail. Your change seems unrelated.

[AndyNewlands]

@dragos-oancea @andywolk As mentioned, my changes seem unrelated to the unit test failures I was getting. Any thoughts on how I can fix that. Also, in respect of your suggestion I add unit test for my changes, I have no idea how, and see no value in doing that unless the other (seemingly unrelated) unit test failures can be resolved. As far as I see it, my code addresses a geniune bug (which caues intermittent, difficult to track issues). Will it be dumped if I can't figure out how to add new units tests?

[signalwire-ci[bot]]

Unit-tests compilation failed: https://public-artifacts.signalwire.cloud/drone/signalwire/freeswitch/651/unit-tests-build-result.txt

[signalwire-ci[bot]]

Scan-build compilation failed: https://public-artifacts.signalwire.cloud/drone/signalwire/freeswitch/651/scan-build-result.txt

[AndyNewlands]

@dragos-oancea @andywolk I've added the check for SWITCH_RTP_FLAG_SRTP_HANGUP_ON_ERROR and ripped out the code which was using SWITCH_RTP_FLAG_NACK. Unit tests now all passed (nothing to do with my changes as far as I can tell).

I really have no idea how to add UT coverage for the new code, as a) I am unsure how the test code works (it would require more extensive undertanding of a LOT more of the code base than I currently have) b) I would not know how to force do_flush on an encrypted DTMF packet.

Are you (or anyone else) able to help with that? If there's no UT coverage for this fix, will it be rejected? If that's the case, it would be a shame as this fix addresses a genuine bug which previously caused intemittent problems for our users (so I'm pretty sure others will experiencing, maybe without understanding why).

[AndyNewlands]

Hi @dragos-oancea @andywolk - did you see my comment (above), from a couple of weeks ago?

[AndyNewlands]

Hi @dragos-oancea @andywolk - should I withdraw this PR? Thanks.