[Asterisk-code-review] res pjsip pubsub: re-re-fix persistent subscription storage. (asterisk[certified/13.1])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_pjsip_pubsub: re-re-fix persistent subscription storage.
......................................................................


res_pjsip_pubsub: re-re-fix persistent subscription storage.

A recent change to res_pjsip_pubsub switched to using pjsip_msg_print as
a means of writing an appropriate packet to persistent storage. While
this partially solved the issue, it had its own problems.
pjsip_msg_print will always add a Content-Length header to the message
it prints. Frequent restarts of Asterisk can result in persistent
subscriptions being written with five or more Content-Length headers. In
addition, sometimes some apparent corruption of individual headers could
be seen.

This aims to fix the problem by not running a parsed message through an
interpreter but rather by taking the raw message and saving it. The
logic for what to save is going to be different depending on whether a
SUBSCRIBE was received from the wire or if it was pulled from
persistence. When receiving a packet from the wire, when using a
streaming transport, the rdata->pkt_info.packet may contain multiple SIP
messages or fragments. However, the rdata->msg_info.msg_buf will always
contain the current SIP message to be processed. When pulling from
persistence, though, the rdata->msg_info.msg_buf will be NULL since no
transport actually handled the packet. However, since we know that we
will always ever pull one SIP message from persistence, we are free to
save directly from rdata->pkt_info.packet instead.

ASTERISK-25365 #close
Reported by Mark Michelson

Change-Id: I33153b10d0b4dc8e3801aaaee2f48173b867855b
---
M res/res_pjsip_pubsub.c
1 file changed, 13 insertions(+), 2 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index b0d86dc..1d4054e 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -558,8 +558,19 @@
 		expires = expires_hdr ? expires_hdr->ivalue : DEFAULT_PUBLISH_EXPIRES;
 		sub_tree->persistence->expires = ast_tvadd(ast_tvnow(), ast_samp2tv(expires, 1));
 
-		pjsip_msg_print(rdata->msg_info.msg, sub_tree->persistence->packet,
-				sizeof(sub_tree->persistence->packet));
+		/* When receiving a packet on an streaming transport, it's possible to receive more than one SIP
+		 * message at a time into the rdata->pkt_info.packet buffer. However, the rdata->msg_info.msg_buf
+		 * will always point to the proper SIP message that is to be processed. When updating subscription
+		 * persistence that is pulled from persistent storage, though, the rdata->pkt_info.packet will
+		 * only ever have a single SIP message on it, and so we base persistence on that.
+		 */
+		if (rdata->msg_info.msg_buf) {
+			ast_copy_string(sub_tree->persistence->packet, rdata->msg_info.msg_buf,
+					MIN(sizeof(sub_tree->persistence->packet), rdata->msg_info.len));
+		} else {
+			ast_copy_string(sub_tree->persistence->packet, rdata->pkt_info.packet,
+					sizeof(sub_tree->persistence->packet));
+		}
 		ast_copy_string(sub_tree->persistence->src_name, rdata->pkt_info.src_name,
 				sizeof(sub_tree->persistence->src_name));
 		sub_tree->persistence->src_port = rdata->pkt_info.src_port;

-- 
To view, visit https://gerrit.asterisk.org/1169
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I33153b10d0b4dc8e3801aaaee2f48173b867855b
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: certified/13.1
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>