On Thu, Dec 22, 2016 at 10:15:36AM -0800, scan-admin(a)coverity.com wrote: Even though coverity triggered on the recent addition of tbf->ms()->update_dl_ctrl_msg(); in osmo-pcu da7250ad2c1cd5ddc7d3c6e10435a00b357ef8f7, this problem has existed before. This function is handling the tbf pointer improperly. Let's take a look: static struct msgb *sched_select_ctrl_msg( uint8_t trx, uint8_t ts, uint32_t fn, uint8_t block_nr, struct gprs_rlcmac_pdch *pdch, struct gprs_rlcmac_tbf *ul_ass_tbf, struct gprs_rlcmac_tbf *dl_ass_tbf, struct gprs_rlcmac_ul_tbf *ul_ack_tbf) { struct msgb *msg = NULL; struct gprs_rlcmac_tbf *tbf = NULL; This NULL init would only make sense if ARRAY_SIZE(next_list) were zero. Will not happen. struct gprs_rlcmac_tbf *next_list[3] = { ul_ass_tbf, dl_ass_tbf, ul_ack_tbf }; for (size_t i = 0; i < ARRAY_SIZE(next_list); ++i) { tbf = next_list[(pdch->next_ctrl_prio + i) % 3]; if (!tbf) continue; /* * Assignments for the same direction have lower precedence, * because they may kill the TBF when the CONTROL ACK is * received, thus preventing the others from being processed. */ if (tbf == ul_ass_tbf && tbf->direction == GPRS_RLCMAC_DL_TBF) if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS_REJ) msg = ul_ass_tbf->create_packet_access_reject(); else msg = ul_ass_tbf->create_ul_ass(fn, ts); else if (tbf == dl_ass_tbf && tbf->direction == GPRS_RLCMAC_UL_TBF) msg = dl_ass_tbf->create_dl_ass(fn, ts); else if (tbf == ul_ack_tbf) msg = ul_ack_tbf->create_ul_ack(fn, ts); if (!msg) { tbf = NULL; If this is not the last for() iteration, setting tbf = NULL makes no sense, it will be set to next_list[..] at the start of the next iteration. If this *is* the last iteration, tbf = NULL makes no sense either, because msg is NULL and tbf will be set to dl_ass_tbf or ul_ass_tbf in the if (!msg) {} case below. continue; } pdch->next_ctrl_prio += 1; pdch->next_ctrl_prio %= 3; break; } if (!msg) { /* * If one of these is left, the response (CONTROL ACK) from the * MS will kill the current TBF, only one of them can be * non-NULL */ if (dl_ass_tbf) { tbf = dl_ass_tbf; msg = dl_ass_tbf->create_dl_ass(fn, ts); } else if (ul_ass_tbf) { tbf = ul_ass_tbf; msg = ul_ass_tbf->create_ul_ass(fn, ts); } } At this point, tbf may be NULL, either from 'if (!tbf) continue;' in the last iteration, or because dl_ass_tbf or ul_ass_tbf were NULL. Below code assumes that tbf is non-NULL and will segfault. Add 'if (!tbf) return NULL' here? /* any message */ if (msg) { tbf->rotate_in_list(); LOGP(DRLCMACSCHED, LOGL_DEBUG, "Scheduling control " "message at RTS for %s (TRX=%d, TS=%d)\n", tbf_name(tbf), trx, ts); /* Updates the dl ctrl msg counter for ms */ tbf->ms()->update_dl_ctrl_msg(); return msg; } /* schedule PACKET PAGING REQUEST */ msg = pdch->packet_paging_request(); if (msg) { LOGP(DRLCMACSCHED, LOGL_DEBUG, "Scheduling paging request " "message at RTS for (TRX=%d, TS=%d)\n", trx, ts); /* Updates the dl ctrl msg counter for ms */ tbf->ms()->update_dl_ctrl_msg(); return msg; } return NULL; } @osmo-pcu tbf folks, please fix this potential segmentation fault ASAP. Thanks! ~N -- - Neels Hofmeyr &lt;nhofmeyr(a)sysmocom.de&gt; 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