#2923 closed defect (wontfix)
Questionable Code in resource_snapshot.c
Reported by: | Joel Sherrill | Owned by: | Sebastian Huber |
---|---|---|---|
Priority: | normal | Milestone: | 5.1 |
Component: | score | Version: | 5 |
Severity: | normal | Keywords: | |
Cc: | Blocked By: | ||
Blocking: |
Description
Coverity URL:
Coverity Scan doesn't like taking the address of a single uint32_t and then treating it as an array pointer. I think the code is likely doing what is intended but there is an implicit linkable between the order of the fields in the structure here and the class numbers used in the object IDs. Is there a way to improve this code and reduce the linkage?
141
- address_of: Taking address with &snapshot->active_posix_keys yields a singleton pointer.
- assign: Assigning: active = &snapshot->active_posix_keys.
142 active = &snapshot->active_posix_keys;
143
- Condition i < 19U /* sizeof (objects_info_table) / sizeof (objects_info_table[0]) */, taking true branch.
144 for (i = 0; i < RTEMS_ARRAY_SIZE(objects_info_table); ++i) {
145 const Objects_Information *information;
146
147 information = _Objects_Get_information(
148 objects_info_table[i].api,
149 objects_info_table[i].cls
150 );
151
- Condition information != NULL, taking true branch.
152 if (information != NULL) {
CID 1399703 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)
- ptr_arith: Using active as an array. This might corrupt or misinterpret adjacent memory locations.
153 active[i] = _Objects_Active_count(information);
Change History (7)
comment:1 Changed on 03/10/17 at 18:13:45 by Joel Sherrill
Owner: | changed from joel.sherrill@… to Sebastian Huber |
---|---|
Status: | new → assigned |
comment:2 Changed on 03/13/17 at 06:00:40 by Sebastian Huber
comment:3 follow-up: 4 Changed on 03/13/17 at 15:08:59 by Gedare Bloom
Very ugly. The hack might be a bit more clear if we use
typedef struct { Heap_Information_block workspace_info; Heap_Information_block heap_info; uint32_t active_posix_key_value_pairs; union { uint32_t anonymous[ 1 + ( sizeof(rtems_resource_rtems_api) + sizeof(rtems_resource_posix_api) ) / sizeof(uint32_t) ]; struct { uint32_t active_posix_keys; rtems_resource_rtems_api rtems_api; rtems_resource_posix_api posix_api; } } int open_files; } rtems_resource_snapshot;
comment:4 Changed on 03/14/17 at 06:28:40 by Sebastian Huber
Replying to Gedare:
Very ugly. The hack might be a bit more clear if we use
typedef struct { Heap_Information_block workspace_info; Heap_Information_block heap_info; uint32_t active_posix_key_value_pairs; union { uint32_t anonymous[ 1 + ( sizeof(rtems_resource_rtems_api) + sizeof(rtems_resource_posix_api) ) / sizeof(uint32_t) ]; struct { uint32_t active_posix_keys; rtems_resource_rtems_api rtems_api; rtems_resource_posix_api posix_api; } } int open_files; } rtems_resource_snapshot;
The rtems_resource_snapshot structure is supposed to be easy to understand for the user and should not make implementation details visible. With the union you still assume a certain mapping of array elements and normal structure members. It is an out-of-bounds access, so Coverity is right, but it is intentional. I don't think this should be changed.
comment:5 Changed on 05/11/17 at 07:31:02 by Sebastian Huber
Milestone: | 4.12 → 4.12.0 |
---|
comment:6 Changed on 05/11/17 at 07:43:47 by Sebastian Huber
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
comment:7 Changed on 11/09/17 at 06:27:14 by Sebastian Huber
Milestone: | 4.12.0 → 5.1 |
---|
Milestone renamed
Yes, the code is ugly, but it works quite well.