[3/5] LC15: fix segfault
diff mbox

Message ID 1458657612-12669-3-git-send-email-msuraev@sysmocom.de
State New
Headers show

Commit Message

Max March 22, 2016, 2:40 p.m. UTC
From: Max <msuraev@sysmocom.de>

Add null pointer check and propagate error.
---
 include/osmo-bts/phy_link.h     | 5 ++++-
 src/osmo-bts-litecell15/l1_if.c | 6 ++++++
 src/osmo-bts-litecell15/l1_if.h | 6 ++++--
 3 files changed, 14 insertions(+), 3 deletions(-)

Comments

Neels Hofmeyr March 22, 2016, 3:27 p.m. UTC | #1
On Tue, Mar 22, 2016 at 03:40:10PM +0100, msuraev@sysmocom.de wrote:
> --- a/src/osmo-bts-litecell15/l1_if.h
> +++ b/src/osmo-bts-litecell15/l1_if.h
> @@ -117,8 +117,10 @@ int l1if_ms_pwr_ctrl(struct gsm_lchan *lchan, const int uplink_target,
>  static inline struct lc15l1_hdl *trx_lc15l1_hdl(struct gsm_bts_trx *trx)
>  {
>  	struct phy_instance *pinst = trx_phy_instance(trx);
> -	OSMO_ASSERT(pinst);
> -	return pinst->u.lc15.hdl;
> +	if (pinst)
> +		return pinst->u.lc15.hdl;
> +
> +	return NULL;

So, an OSMO_ASSERT() is typically there to ensure that the code never ends
up breaking the assertion. If the OSMO_ASSERT() ever hits, it would mean
that there's something fatally wrong elsewhere in the code.

Have you checked in this instance that a NULL pinst may be a valid
situation that doesn't need to be asserted upon? If so, I would welcome if
that were mentioned in the log message.

The other chunks above look like pretty obvious improvements, yet maybe
the reasons why they cause segfaults are also elsewhere in the code?
(We don't always check all pointers because we "know" they are fine.)

I have no idea, really, just poking / provoking verbosity ;)

~Neels
Harald Welte March 27, 2016, 8:40 a.m. UTC | #2
Hi Max,

regarding the original patch:

* the commit message fails to explain why this fixes a segfault,
  and when that segfault occurs
* if Neels provides some feedback like the one he did, please follow-up
  to that to keep progress on this patch going.

If the OSMO_ASSERT() in the original code was wrong, then please explain
why it was wrong.  This explanation belongs in the commit log message.

Also, as trx_lc15l1_hdl() can now return NULL, did you verify that all
callers of that function can actually deal with a NULL return value?

Regards,
	Harald

Patch
diff mbox

diff --git a/include/osmo-bts/phy_link.h b/include/osmo-bts/phy_link.h
index a559aa3..5ab5d49 100644
--- a/include/osmo-bts/phy_link.h
+++ b/include/osmo-bts/phy_link.h
@@ -125,7 +125,10 @@  void phy_user_statechg_notif(struct phy_instance *pinst, enum phy_link_state lin
 
 static inline struct phy_instance *trx_phy_instance(struct gsm_bts_trx *trx)
 {
-	return trx->role_bts.l1h;
+	if (trx)
+		return trx->role_bts.l1h;
+
+	return NULL;
 }
 
 int bts_model_phy_link_open(struct phy_link *plink);
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index f625968..d89cc29 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -1242,6 +1242,12 @@  static int reset_compl_cb(struct gsm_bts_trx *trx, struct msgb *resp,
 	Litecell15_Prim_t *sysp = msgb_sysprim(resp);
 	GsmL1_Status_t status = sysp->u.layer1ResetCnf.status;
 
+	if (!fl1h) {
+		LOGP(DL1C, LOGL_ERROR, "reset_compl_cb() is unable to get fl1h"
+		     " from trx\n");
+		return 1;
+	}
+
 	LOGP(DL1C, LOGL_NOTICE, "Rx L1-RESET.conf (status=%s)\n",
 		get_value_string(lc15bts_l1status_names, status));
 
diff --git a/src/osmo-bts-litecell15/l1_if.h b/src/osmo-bts-litecell15/l1_if.h
index 0c8843b..773840d 100644
--- a/src/osmo-bts-litecell15/l1_if.h
+++ b/src/osmo-bts-litecell15/l1_if.h
@@ -117,8 +117,10 @@  int l1if_ms_pwr_ctrl(struct gsm_lchan *lchan, const int uplink_target,
 static inline struct lc15l1_hdl *trx_lc15l1_hdl(struct gsm_bts_trx *trx)
 {
 	struct phy_instance *pinst = trx_phy_instance(trx);
-	OSMO_ASSERT(pinst);
-	return pinst->u.lc15.hdl;
+	if (pinst)
+		return pinst->u.lc15.hdl;
+
+	return NULL;
 }
 
 static inline struct gsm_bts_trx *lc15l1_hdl_trx(struct lc15l1_hdl *fl1h)