[Coverity CID :219662] Inequality comparison against NULL in lib/os/cbprintf_packaged.c

[zephyrbot]

Static code scan issues found in file:

https:/zephyrproject-rtos/zephyr/tree/bd97359a5338b2542d19011b6d6aa1d8d1b9cc3f/lib/os/cbprintf_packaged.c#L54

Category: Incorrect expression
Function: ptr_in_rodata
Component: Other
CID: 219662

Details:

https:/zephyrproject-rtos/zephyr/blob/bd97359a5338b2542d19011b6d6aa1d8d1b9cc3f/lib/os/cbprintf_packaged.c

47     #define RO_END _rodata_end
48     #else
49     #define RO_START 0
50     #define RO_END 0
51     #endif
52    
>>>     CID 219662:    (BAD_COMPARE)
>>>     Comparing pointer "addr" against "NULL" using anything besides "==" or "!=" is likely to be incorrect.
53         return ((addr >= (const char *)RO_START) &&
54             (addr < (const char *)RO_END));
55     }
56    
57     /*
58      * va_list creation
48     #else
49     #define RO_START 0
50     #define RO_END 0
51     #endif
52    
53         return ((addr >= (const char *)RO_START) &&
>>>     CID 219662:    (BAD_COMPARE)
>>>     Comparing pointer "addr" against "NULL" using anything besides "==" or "!=" is likely to be incorrect.
54             (addr < (const char *)RO_END));
55     }
56    
57     /*
58      * va_list creation
59      */

Please fix or provide comments in coverity using the link:

https://scan9.coverity.com/reports.htm#v32951/p12996.

Note: This issue was created automatically. Priority was set based on classification
of the file affected and the impact field in coverity. Assignees were set using the CODEOWNERS file.

[gudnimg]

This code looks very similar: https:/zephyrproject-rtos/zephyr/blob/master/subsys/logging/log_core.c#L116 could the slight differences between log_core.c and this code that could trigger the error?

For example in log_core.c we have:

#if defined(CONFIG_ARM) || defined(CONFIG_ARC) || defined(CONFIG_X86)
	extern const char *_image_rodata_start[];
	extern const char *_image_rodata_end[];
	#define RO_START _image_rodata_start
	#define RO_END _image_rodata_end

But this function in cbprintf_packaged.c

#elif defined(CONFIG_ARC) || defined(CONFIG_ARM) || defined(CONFIG_X86) \
	|| defined(CONFIG_RISCV)
	extern char _image_rodata_start[];
	extern char _image_rodata_end[];
#define RO_START _image_rodata_start
#define RO_END _image_rodata_end

I am specifically wondering about this part: extern const char * <--> extern char

[npitre]

On Thu, 11 Mar 2021, Guðni Már Gilbert wrote: I am specifically wondering about this part: `extern const char *` <--> `extern char`

Adding a `const` certainly won't hurt. In any case, this would gain to be made into some per-architecture abstraction common to all users.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

[de-nordic]

This is false positive. The coverity does not understand what is happening there: it thinks that the comparison is to detect whether addr has valid (non-NULL) or invalid (NULL) pointer. Because it sees that RO_START is 0 (NULL), it thinks that someone is trying to check if addr is NULL or not, and does not understand that it is basically test if addr fits in specific range, which is given by [RO_START,RO_END)

@npitre do you agree?

[ioannisg]

Going to close this, since it is already triaged as false-positive.
@npitre @galak feel free to re-open if applicable.