OpenSRF Crashes with Ejabberd 21.12

Bug #1973060 reported by Jason Stephenson
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenSRF
Fix Committed
High
Unassigned

Bug Description

O/S Ubuntu 22.04

Changes in the ProcessOne XMPP Erlang package between releases 1.5.4 and 1.5.6 cause crashes in OpenSRF when the latter handles a XMPP error message. This is most easily seen when making a request to an OpenSRF service that does not exist. srfsh will segfault as the transport client code attempts to access a non-existent "code" attribute on the error element. The code extension attribute was removed from the XMPP Erlang library in August of 2021. The code attribute was deprecated prior to the release of RFC 3920 in 2004. Ejabbered and the ProessOne XMPP library both continued to support it for legacy clients for a number of years.

The standard way to deal with XMPP errors is to use the error condition element which should be the first child of the error element.

My recommendation is that we add an "error_condition" character pointer to the transport_message_struct and use this instead of the "error_code" field. If we must continue to use the error_code, XEP-0086 provides suggested mappings between the error condition string and numbers: https://xmpp.org/extensions/xep-0086.html. Constants for these values are already provided in the osrf_message.h header file.

This issue was uncovered while implementing bug 1970667.

Tags: pullrequest
description: updated
Galen Charlton (gmc)
Changed in opensrf:
status: New → Confirmed
importance: Undecided → High
Revision history for this message
Galen Charlton (gmc) wrote :

I've taken an initial lo look at this. There seems to be very little code that's doing more with the <error> element's code and type attributes besides logging them, so it's looking easier than I expected to drop any expectation that XEP-0086 is supported. In fact, we might be able to pull off a fix without even breaking ABI for Evergreen.

Revision history for this message
Galen Charlton (gmc) wrote :

Looks like the only error condition that is specifically handled specially by OpenSRF is <not-authorized/>.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Yes, I think we could make the changes without affecting Evergreen.

To clarify my thoughts from the bug description, we could use the code attribute if it is still present and if it isn't, then look up the error_condition and get the code using the table of suggested values in XEP-0086. If the error_condition element isn't listed, we could fallback to undefined-condition.

I was thinking that a new function could be implemented for the error_condition to code lookup. Though, I suppose that a hash map data structure could be implemented.

Changed in opensrf:
assignee: nobody → Jason Stephenson (jstephenson)
milestone: none → 3.3-beta
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I have pushed a branch to user/dyrcona/lp1973060-modernize-xmpp (https://git.evergreen-ils.org/?p=working/OpenSRF.git;a=shortlog;h=refs/heads/user/dyrcona/lp1973060-modernize-xmpp) that almost does what I suggest in comment #3.

It does NOT add an error_condition member to the transport_message_struct as that was deemed unnecessary.

This branch should go in at the same time as the branch from bug 1970667 and that branch is required for testing this one on Ubuntu 22.04.

Changed in opensrf:
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: pullrequest
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I rebased the branch on master and fixed up a couple of follow up commits this morning.

Bill Erickson (berick)
Changed in opensrf:
assignee: nobody → Bill Erickson (berick)
Revision history for this message
Bill Erickson (berick) wrote :

See bug #1970667 for sign off.

Changed in opensrf:
assignee: Bill Erickson (berick) → nobody
Changed in opensrf:
status: Confirmed → Fix Committed
Revision history for this message
Jane Sandberg (sandbergja) wrote :

Works well for me! Thanks, Jason and Bill. Merged for inclusion in 3.3.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.