Unbounded AGCH queue in OsmoBTS

[Holger Hans Peter Freyther]

On Tue, Feb 11, 2014 at 11:13:35AM +0400, Ivan Kluchnikov wrote:

Good Morning,

> The attached patches for openbsc and osmo-bts finally fix issues which
> were discussed above.

what a surprise. Jacob has started implementing AGCH queue handling
as well. So I will let him comment on the BTS part.



OpenBSC:
> +
> +	/* bts didn't send IMM_ASSIGN, so we should release allocated channel */
> +	ia = (struct gsm48_imm_ass *) (rqd_hdr->data + 2);

Please add a size check that the mandatory element actually fits
and use early returns.

> +	if (ia->msg_type == GSM48_MT_RR_IMM_ASS) {
> +		chan_nr = ia->chan_desc.chan_nr;
> +		lchan = lchan_lookup(trx, chan_nr);

same thing for the lchan. Verify it was found and that the state
is actually the right one.


> +		rsl_rf_chan_release(lchan, 1, SACCH_DEACTIVATE);

Maybe use rsl_direct_rf_release. By definition there is no one listening
on the SACCH. So there is not point in going through the normal release
procedure.


> +/* 8.5.4 DELETE INDICATION */
> +int rsl_tx_delete_ind(struct gsm_bts *bts, uint8_t len, uint8_t *val)
> +{
> +	struct msgb *msg;
> +
> +	msg = rsl_msgb_alloc(sizeof(struct abis_rsl_cchan_hdr));
> +	if (!msg)
> +		return -ENOMEM;
> +	rsl_cch_push_hdr(msg, RSL_MT_DELETE_IND, RSL_CHAN_PCH_AGCH);
> +	msgb_tlv_put(msg, RSL_IE_FULL_IMM_ASS_INFO, len, val);
> +	msg->trx = bts->c0;

Have you manually tested this with multi-trx support and the channel
being on the second trx? The lchan_lookup in OpenBSC will be done using
the "bts->c0"?


> -		msgb_free(msg);
> +		rsl_tx_delete_ind(trx->bts, msg->len, msg->data);
> +		return -ENOMEM;

Does it leak now? or was it a double free before?