lchan->s and integer overflow

[Holger Hans Peter Freyther]

On Thu, Mar 14, 2013 at 07:47:25AM +0100, jolly wrote:
> Holger Hans Peter Freyther wrote:

Hi,

this must be frustrating for you, but it really is for me as well. I
take absolutely no joy in pointing out these issues. I would prefer to
work on my code but it is something that will bite us in a commercial
setup so I a have to be the PITA here.



>  	/* 9.4.14 Connection Failure Criterion */
>  	if (TLVP_PRESENT(&tp, NM_ATT_CONN_FAIL_CRIT) &&
> -	    (TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2) &&
> -	    *TLVP_VAL(&tp, NM_ATT_CONN_FAIL_CRIT) == 0x01) {
> +	    (TLVP_LEN(&tp, NM_ATT_CONN_FAIL_CRIT) >= 2)) {
>  		const uint8_t *val = TLVP_VAL(&tp, NM_ATT_CONN_FAIL_CRIT);
> -		btsb->radio_link_timeout = val[1];
> +		if (val[0] == 0x01 && val[1] >= 4)
> +			btsb->radio_link_timeout = val[1];
>  	}

Here you point out that the range of 4 to UINT8_MAX is coming from the
specification. Which is very good.  The way it is done is a violation of
the principle of least surprise though. If a BSC sends the value '2' it
doesn't make sense that '32' is used. The configurator/implementor
certainly wanted to have a low value.

IMHO the right thing to do is to NACK the set bts attributes with
a descriptive error message. The same goes for the conn fail crit
being present but not of the type we support.



holger


PS:

the lchan->s handling is fine. The switch/case has grown to a state
I would move the link failure handling to a new helper function though,
this way one could even write a unit test for the handling...
>  	switch (data_ind->sapi) {
>  	case GsmL1_Sapi_Sacch:
> -		/* process radio link timeout coniter S */
> +		/* process radio link timeout counter S */