diff mbox series

[v2] qmi: call-forwarding: fallback to basic forwarding info if no extended

Message ID 20241208123946.255405-1-ivo.g.dimitrov.75@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] qmi: call-forwarding: fallback to basic forwarding info if no extended | expand

Commit Message

Ivaylo Dimitrov Dec. 8, 2024, 12:39 p.m. UTC 
At least Motorola Droid 4 does not support extended call forward info, so
fallback to basic
---
 drivers/qmimodem/call-forwarding.c | 81 +++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 29 deletions(-)

Comments

Denis Kenzior Dec. 11, 2024, 5:27 a.m. UTC | #1 
Hi Ivo,

On 12/8/24 6:39 AM, Ivaylo Dimitrov wrote:
> At least Motorola Droid 4 does not support extended call forward info, so
> fallback to basic
> ---
>   drivers/qmimodem/call-forwarding.c | 81 +++++++++++++++++++-----------
>   1 file changed, 52 insertions(+), 29 deletions(-)
> 

CI complains this fails under clang:

Fedora (glibc) clang debug+sanitizers
=====================================
Configure: PASS
Build: FAIL
     drivers/qmimodem/call-forwarding.c:30:4: error: field 'i' with variable 
sized type 'struct (unnamed struct at drivers/qmimodem/call-forwarding.c:25:2)' 
not at the end of a struct or class is a GNU extension 
[-Werror,-Wgnu-variable-sized-type-not-at-end]
        30 |         } i;
           |           ^
     1 error generated.
     make[1]: *** [Makefile:4083: drivers/qmimodem/call-forwarding.o] Error 1
     make[1]: Target 'all-am' not remade because of errors.
     make: *** [Makefile:2388: all] Error 2

> diff --git a/drivers/qmimodem/call-forwarding.c b/drivers/qmimodem/call-forwarding.c
> index e7ae3515..fb9bb4e1 100644
> --- a/drivers/qmimodem/call-forwarding.c
> +++ b/drivers/qmimodem/call-forwarding.c
> @@ -21,6 +21,16 @@ struct call_forwarding_data {
>   	struct qmi_service *voice;
>   };
>   
> +struct call_forwarding_info {
> +	struct {
> +		int8_t active;
> +		uint8_t cls;
> +		uint8_t len;
> +		uint8_t number[];
> +	} i;
> +	uint8_t time;
> +} __attribute__((__packed__));
> +
>   struct call_forwarding_info_ext {
>   	uint8_t active;
>   	uint8_t cls;
> @@ -31,7 +41,7 @@ struct call_forwarding_info_ext {
>   	uint8_t plan;
>   	uint8_t len;
>   	uint8_t number[];
> -};
> +} __attribute__((__packed__));
>   
>   static int forw_type_to_reason(int type)
>   {
> @@ -78,55 +88,68 @@ static void query_cb(struct qmi_result *result, void *user_data)
>   	ofono_call_forwarding_query_cb_t cb = cbd->cb;
>   	const uint8_t *p;

const void *p;

>   	uint8_t num;
> +	const uint8_t *end;
>   	uint16_t length;
> -
> +	struct ofono_call_forwarding_condition *list = NULL;

A nicer way might be:
_auto_(l_free) struct ofono_call_forwarding_condition *list = NULL;

> +	int i;
> +	bool extended = false;
>   	DBG("");
>   
>   	if (qmi_result_set_error(result, NULL))
>   		goto error;
>   
>   	/*
> -	 * we want extended info, because of the number type.
> -	 * FIXME - shall we fallback to 0x10 if there is no extended info?
> +	 * we want extended info if any, because of the number type.
>   	 */
>   	p = qmi_result_get(result, 0x16, &length);
> -	if (p && length) {
> -		struct ofono_call_forwarding_condition *list;
> -		const uint8_t *end = p + length;
> -		int i;
> +	if (p && length)
> +		extended = true;
> +	else
> +		p = qmi_result_get(result, 0x10, &length);
>   
> -		num = *p++;
> +	if (!extended && (!p || !length))
> +		goto error;
>   
> -		list = l_new(struct ofono_call_forwarding_condition, num);
> +	end = p + length;
> +	num = *p++;

num = l_get_u8(p);

> +	list = l_new(struct ofono_call_forwarding_condition, num);
>   
> -		for (i = 0; i < num; i++) {
> -			struct call_forwarding_info_ext *info = (void *)p;
> +	for (i = 0; i < num; i++) {
> +		if (extended) {
> +			struct call_forwarding_info_ext *fi = (void *)p;

No need for cast if p is void *

> +			const uint8_t *iend = p + sizeof(*fi);
>   			int type;
>   
> -			/* do not try to access beyond buffer end */
> -			if (p + sizeof(*info) > end ||
> -					p + sizeof(*info) + info->len > end) {
> -				l_free(list);
> +			if (iend > end || iend + fi->len > end)
>   				goto error;
> -			}
>   
> -			if (info->type == 1)
> -				type = OFONO_NUMBER_TYPE_INTERNATIONAL;
> -			else
> -				type = OFONO_NUMBER_TYPE_UNKNOWN;
> +			type = fi->type == 1 ?
> +					OFONO_NUMBER_TYPE_INTERNATIONAL :
> +					OFONO_NUMBER_TYPE_UNKNOWN;
> +			set_fwd_cond(&list[i], fi->active, fi->cls,
> +					fi->time, type, fi->number, fi->len);
>   
> -			set_fwd_cond(&list[i], info->active, info->cls,
> -					info->time, type, info->number,
> -					info->len);
> -			p += sizeof(*info) + info->len;
> -		}
> +			p += sizeof(*fi) + fi->len;
> +		} else {
> +			struct call_forwarding_info *fi = (void *)p;
> +			const uint8_t *iend = p + sizeof(*fi);
>   
> -		CALLBACK_WITH_SUCCESS(cb, num, list, cbd->data);
> -		l_free(list);
> -		return;
> +			if (iend > end || iend + fi->i.len > end)
> +				goto error;
> +
> +			set_fwd_cond(&list[i], fi->i.active, fi->i.cls,
> +					fi->time, OFONO_NUMBER_TYPE_UNKNOWN,
> +					fi->i.number, fi->i.len);
> +			p += sizeof(*fi) + fi->i.len;
> +		}
>   	}
>   
> +	CALLBACK_WITH_SUCCESS(cb, num, list, cbd->data);
> +	l_free(list);

No need for l_free here

> +	return;
> +
>   error:
> +	l_free(list);

or here if using _auto_

>   	CALLBACK_WITH_FAILURE(cb, 0, NULL, cbd->data);
>   }
>   

Regards,
-Denis
Ivaylo Dimitrov Dec. 11, 2024, 3:51 p.m. UTC | #2 
Hi Denis,

On 11.12.24 г. 7:27 ч., Denis Kenzior wrote:

...

> 
> const void *p;
> 

I'd better not, see below.

>>       uint8_t num;
>> +    const uint8_t *end;
>>       uint16_t length;
>> -
>> +    struct ofono_call_forwarding_condition *list = NULL;
> 
> A nicer way might be:
> _auto_(l_free) struct ofono_call_forwarding_condition *list = NULL;
> 
>> +    int i;
>> +    bool extended = false;
>>       DBG("");
>>       if (qmi_result_set_error(result, NULL))
>>           goto error;
>>       /*
>> -     * we want extended info, because of the number type.
>> -     * FIXME - shall we fallback to 0x10 if there is no extended info?
>> +     * we want extended info if any, because of the number type.
>>        */
>>       p = qmi_result_get(result, 0x16, &length);
>> -    if (p && length) {
>> -        struct ofono_call_forwarding_condition *list;
>> -        const uint8_t *end = p + length;
>> -        int i;
>> +    if (p && length)
>> +        extended = true;
>> +    else
>> +        p = qmi_result_get(result, 0x10, &length);
>> -        num = *p++;
>> +    if (!extended && (!p || !length))
>> +        goto error;
>> -        list = l_new(struct ofono_call_forwarding_condition, num);
>> +    end = p + length;

void * arithmetic, UB last time I checked, unless you are absolutely 
sure compilers will do the right thing we'd better keep it like that. p 
is used all over the place as const uint8_t *, I am not sure there is 
elegant way of converting it to void * without casts.

Will send v2 with fix for the above clang error and using _auto_.

Thanks,
Ivo
Denis Kenzior Dec. 11, 2024, 3:56 p.m. UTC | #3 
Hi Ivo,

 >
> void * arithmetic, UB last time I checked, unless you are absolutely sure 

Yes, it is UB in a strict sense.  However, on Linux, the only two compilers we 
care about (or likely will care about for a very long time) are pretty clear in 
void pointer arithmetic semantics.

If the Linux kernel does it and can rely on this behavior, so can we.

> compilers will do the right thing we'd better keep it like that. p is used all 
> over the place as const uint8_t *, I am not sure there is elegant way of 
> converting it to void * without casts.

Regards,
-Denis
diff mbox series

Patch

diff --git a/drivers/qmimodem/call-forwarding.c b/drivers/qmimodem/call-forwarding.c
index e7ae3515..fb9bb4e1 100644
--- a/drivers/qmimodem/call-forwarding.c
+++ b/drivers/qmimodem/call-forwarding.c
@@ -21,6 +21,16 @@  struct call_forwarding_data {
 	struct qmi_service *voice;
 };
 
+struct call_forwarding_info {
+	struct {
+		int8_t active;
+		uint8_t cls;
+		uint8_t len;
+		uint8_t number[];
+	} i;
+	uint8_t time;
+} __attribute__((__packed__));
+
 struct call_forwarding_info_ext {
 	uint8_t active;
 	uint8_t cls;
@@ -31,7 +41,7 @@  struct call_forwarding_info_ext {
 	uint8_t plan;
 	uint8_t len;
 	uint8_t number[];
-};
+} __attribute__((__packed__));
 
 static int forw_type_to_reason(int type)
 {
@@ -78,55 +88,68 @@  static void query_cb(struct qmi_result *result, void *user_data)
 	ofono_call_forwarding_query_cb_t cb = cbd->cb;
 	const uint8_t *p;
 	uint8_t num;
+	const uint8_t *end;
 	uint16_t length;
-
+	struct ofono_call_forwarding_condition *list = NULL;
+	int i;
+	bool extended = false;
 	DBG("");
 
 	if (qmi_result_set_error(result, NULL))
 		goto error;
 
 	/*
-	 * we want extended info, because of the number type.
-	 * FIXME - shall we fallback to 0x10 if there is no extended info?
+	 * we want extended info if any, because of the number type.
 	 */
 	p = qmi_result_get(result, 0x16, &length);
-	if (p && length) {
-		struct ofono_call_forwarding_condition *list;
-		const uint8_t *end = p + length;
-		int i;
+	if (p && length)
+		extended = true;
+	else
+		p = qmi_result_get(result, 0x10, &length);
 
-		num = *p++;
+	if (!extended && (!p || !length))
+		goto error;
 
-		list = l_new(struct ofono_call_forwarding_condition, num);
+	end = p + length;
+	num = *p++;
+	list = l_new(struct ofono_call_forwarding_condition, num);
 
-		for (i = 0; i < num; i++) {
-			struct call_forwarding_info_ext *info = (void *)p;
+	for (i = 0; i < num; i++) {
+		if (extended) {
+			struct call_forwarding_info_ext *fi = (void *)p;
+			const uint8_t *iend = p + sizeof(*fi);
 			int type;
 
-			/* do not try to access beyond buffer end */
-			if (p + sizeof(*info) > end ||
-					p + sizeof(*info) + info->len > end) {
-				l_free(list);
+			if (iend > end || iend + fi->len > end)
 				goto error;
-			}
 
-			if (info->type == 1)
-				type = OFONO_NUMBER_TYPE_INTERNATIONAL;
-			else
-				type = OFONO_NUMBER_TYPE_UNKNOWN;
+			type = fi->type == 1 ?
+					OFONO_NUMBER_TYPE_INTERNATIONAL :
+					OFONO_NUMBER_TYPE_UNKNOWN;
+			set_fwd_cond(&list[i], fi->active, fi->cls,
+					fi->time, type, fi->number, fi->len);
 
-			set_fwd_cond(&list[i], info->active, info->cls,
-					info->time, type, info->number,
-					info->len);
-			p += sizeof(*info) + info->len;
-		}
+			p += sizeof(*fi) + fi->len;
+		} else {
+			struct call_forwarding_info *fi = (void *)p;
+			const uint8_t *iend = p + sizeof(*fi);
 
-		CALLBACK_WITH_SUCCESS(cb, num, list, cbd->data);
-		l_free(list);
-		return;
+			if (iend > end || iend + fi->i.len > end)
+				goto error;
+
+			set_fwd_cond(&list[i], fi->i.active, fi->i.cls,
+					fi->time, OFONO_NUMBER_TYPE_UNKNOWN,
+					fi->i.number, fi->i.len);
+			p += sizeof(*fi) + fi->i.len;
+		}
 	}
 
+	CALLBACK_WITH_SUCCESS(cb, num, list, cbd->data);
+	l_free(list);
+	return;
+
 error:
+	l_free(list);
 	CALLBACK_WITH_FAILURE(cb, 0, NULL, cbd->data);
 }