#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:

https://scan5.coverity.com/reports.htm#v29808/p10069/fileInstanceId=108958257&defectInstanceId=30877219&mergedDefectId=1399703

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

  1. address_of: Taking address with &snapshot->active_posix_keys yields a singleton pointer.
  2. assign: Assigning: active = &snapshot->active_posix_keys.

142 active = &snapshot->active_posix_keys;
143

  1. 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

  1. Condition information != NULL, taking true branch.

152 if (information != NULL) {

CID 1399703 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)

  1. 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: newassigned

comment:2 Changed on 03/13/17 at 06:00:40 by Sebastian Huber

Yes, the code is ugly, but it works quite well.

comment:3 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 in reply to:  3 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.124.12.0

comment:6 Changed on 05/11/17 at 07:43:47 by Sebastian Huber

Resolution: wontfix
Status: assignedclosed

comment:7 Changed on 11/09/17 at 06:27:14 by Sebastian Huber

Milestone: 4.12.05.1

Milestone renamed

Note: See TracTickets for help on using tickets.