Change in libosmo-sccp[master]: add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()

[Neels Hofmeyr]

Hello Jenkins Builder, 

I'd like you to reexamine a change. Please visit

    https://gerrit.osmocom.org/13277

to look at the new patch set (#2).

Change subject: add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()
......................................................................

add caller-owns-msgb variant osmo_sccp_user_sap_down_nofree()

Add osmo_sccp_user_sap_down_nofree(), which is identical to
osmo_sccp_user_sap_down(), but doesn't imply a msgb_free().

To implement that, sccp_sclc_user_sap_down_nofree() with the same msgb
semantics is required.

Rationale:

Avoiding msgb leaks is easiest if the caller retains ownership of the msgb.
Take this hypothetical chain where leaks are obviously avoided:

  void send()
  {
  	msg = msgb_alloc();
	dispatch(msg);
	msgb_free(msg);
  }

  void dispatch(msg)
  {
  	osmo_fsm_inst_dispatch(fi, msg);
  }

  void fi_on_event(fi, data)
  {
	if (socket_is_ok)
		socket_write((struct msgb*)data);
  }

  void socket_write(msgb)
  {
  	if (!ok1)
		return;
	if (ok2) {
		if (!ok3)
			return;
		write(sock, msg->data);
	}
  }

However, if the caller passes ownership down to the msgb consumer, things
become nightmarishly complex:

  void send()
  {
  	msg = msgb_alloc();
	rc = dispatch(msg);
	/* dispatching event failed? */
	if (rc)
		msgb_free(msg);
  }

  int dispatch(msg)
  {
  	if (osmo_fsm_inst_dispatch(fi, msg))
		return -1;
	if (something_else())
		return -1; // <-- double free!
  }

  void fi_on_event(fi, data)
  {
	if (socket_is_ok) {
		socket_write((struct msgb*)data);
	else
		/* socket didn't consume? */
		msgb_free(data);
  }

  int socket_write(msgb)
  {
  	if (!ok1)
		return -1; // <-- leak!
	if (ok2) {
		if (!ok3)
			goto out;
		write(sock, msg->data);
	}
  out:
        msgb_free(msg);
	return -2;
  }

If any link in this call chain fails to be aware of the importance to return a
failed RC or to free a msgb if the chain is broken, or to not return a failed
RC if the msgb is consumed, we have a hidden msgb leak or double free.

This is the case with osmo_sccp_user_sap_down(). In new osmo-msc, passing data
through various FSM instances, there is high potential for leak/double-free
bugs. A very large brain is required to track down every msgb path.

osmo_sccp_user_sap_down_nofree() makes this problem trivial to solve even for
humans.

Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
---
M include/osmocom/sigtran/sccp_sap.h
M src/sccp_internal.h
M src/sccp_sclc.c
M src/sccp_scoc.c
4 files changed, 38 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmo-sccp refs/changes/77/13277/2
-- 
To view, visit https://gerrit.osmocom.org/13277
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic818efa78b90f727e1a94c18b60d9a306644f340
Gerrit-Change-Number: 13277
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190412/715c804e/attachment.htm>