easier way to avoid msgb leaks in libosmo-sccp callers?

[Neels Hofmeyr]

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, we have a hidden msgb leak.

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.

Isn't it possible to provide osmo_sccp_user_sap_down() in the caller-owns
paradigm? Thinking about an osmo_sccp_user_sap_down2() that simply doesn't
msgb_free().

Passing ownership to the consumer is imperative if a msg queue is involved that
might send out asynchronously. (A workaround could be to copy the message
before passing into the wqueue.) However, looks to me like no osmo_wqueue is
involved in osmo_sccp_user_sap_down()? It already frees the msgb right upon
returning, so this should be perfectly fine -- right?

I think I'll just try it, but if anyone knows a definite reason why this will
not work, please let me know.

(Remotely related, I also still have this potential xua msg leak fix lying
around, never got around to verify it:
https://gerrit.osmocom.org/#/c/libosmo-sccp/+/9957/ )

~N

-- 
- Neels Hofmeyr <nhofmeyr at sysmocom.de>          http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Harald Welte
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20190310/57bc0a05/attachment.bin>