| Message ID | 1458657612-12669-3-git-send-email-msuraev@sysmocom.de |
|---|---|
| State | New |
| Headers | show |
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
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
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)
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(-)