[bug]: app_voicemail_imap wrong behavior when losing IMAP connection

[OlafTitz]

Severity

Minor

Versions

18.14.0

Components/Modules

app_voicemail_imap

Operating Environment

Ubuntu 22.04

Frequency of Occurrence

Frequent

Issue Description

Seen by user: When the IMAP server is down, a wrong MWI is delivered. My desk phone shows 2 waiting messages (when there were none previously).

Apparent cause: the function static int messagecount(const char *mailbox_id, const char *folder) just returns the result from calling __messagecount(context, mailbox, folder). But that can be negative in case of error. Then the clients get a NOTIFY with "-1" messages waiting. The function has_voicemail(const char *mailbox, const char *folder) similarly checks for the result of __messagecount being != 0 instead of >0.

This happens when the IMAP connection is lost and can't be reestablished (e.g. IMAP server shut down), but judging from the code it can happen in other error situations too.

Relevant log output

When the IMAP server shuts down, asterisk regularly logs:

[May  4 20:42:51] ERROR[1642]: app_voicemail_imap.c:3287 mm_log: IMAP Error: Can't connect to 10.9.4.2,993: Connection refused
[May  4 20:42:51] ERROR[1642]: app_voicemail_imap.c:2497 __messagecount: Houston we have a problem - IMAP mailstream is NULL

The clients get this bogus NOTIFY:

NOTIFY sip:30@***;transport=tcp;x-reg=*** SIP/2.0
Via: SIP/2.0/TCP ***;rport;branch=***;alias
From: <sip:30@***>;tag=***
To: <sip:30@***;x-reg=***>
Contact: <sip:30@***;transport=TCP>
Call-ID: ***
CSeq: *** NOTIFY
Subscription-State: terminated
Event: message-summary
Allow-Events: message-summary, presence, dialog, refer
Max-Forwards: 70
User-Agent: Asterisk PBX 18.14.0~dfsg+~cs6.12.40431414-1
Content-Type: application/simple-message-summary
Content-Length:   104

Messages-Waiting: yes
Message-Account: sip:*90@***;transport=TCP
Voice-Message: -1/0 (0/0)

Asterisk Issue Guidelines

• Yes, I have read the Asterisk Issue Guidelines

[seanbright]

@jcolp should I delete unlicensed code?

[jcolp]

@seanbright Since there is no CLA checking on patch attachments on issues yes, I think removing the patch is the best way to ensure that the licensing remains clean and noone tries to PR it themselves without proper licensing.

[seanbright]

@OlafTitz - I had to remove your patch because we do not have CLA checking on issues yet. If you put up a PR and sign the CLA we will be able to get the patch integrated.

[OlafTitz]

I'll try. From first reading, the required workflow is extraordinarily complicated and needs tooling with which I'm not familiar.
I understand the licensing concerns. However note that this is an extremely unfriendly reaction to a first-time contributor, and reading the simple patch with a total of just 13 lines changed would have assured you that there should be no special issues with it.

[jcolp]

Our workflow is the standard Github pull request workflow, with added comments to specify what other branches the change should go into. We require all code changes to have a CLA associated with it.