On Thu, Dec 22, 2016 at 10:15:36AM -0800, scan-admin@coverity.com wrote:

*** CID 158969: Null pointer dereferences (FORWARD_NULL) /source-Osmocom/osmo-pcu/src/gprs_rlcmac_sched.cpp: 131 in sched_select_ctrl_msg(unsigned char, unsigned char, unsigned int, unsigned char, gprs_rlcmac_pdch *, gprs_rlcmac_tbf *, gprs_rlcmac_tbf *, gprs_rlcmac_ul_tbf *)() 125 struct msgb *msg = NULL; 126 struct gprs_rlcmac_tbf *tbf = NULL; 127 struct gprs_rlcmac_tbf *next_list[3] = { ul_ass_tbf, dl_ass_tbf, ul_ack_tbf }; 128 129 for (size_t i = 0; i < ARRAY_SIZE(next_list); ++i) { 130 tbf = next_list[(pdch->next_ctrl_prio + i) % 3];

...

...

...

CID 158969:  Null pointer dereferences  (FORWARD_NULL)
Comparing "tbf" to null implies that "tbf" might be null.

131 if (!tbf) 132 continue; 133 134 /* 135 * Assignments for the same direction have lower precedence, 136 * because they may kill the TBF when the CONTROL ACK is

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