nimble-code / Cobra

An interactive (fast) static source code analyzer
139 stars 31 forks source link

(Potential) False positive: "caller does not check return value" #26

Closed mjeronimo closed 2 years ago

mjeronimo commented 3 years ago

Using the jpl ruleset, this rule indicates that the code does not check the return value. However, the calls return the return a duration and are then used in a boolean expression. Not sure what checking could happen on the return values.

Also, the rule complains about this:

  return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);

but not this:

  rmw_duration_t d1 = rmw_time_total_nsec(left);
  rmw_duration_t d2 = rmw_time_total_nsec(right);
  return d1 == d2;

Which are semantically equivalent.

To reproduce:

  cobra -C++ -comments -f jpl t.c

Where t.c contains:

#include "rmw/time.h"
#include "rcutils/time.h"

RMW_PUBLIC
RMW_WARN_UNUSED
bool
rmw_time_equal(const rmw_time_t left, const rmw_time_t right)
{
  return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);
}

RMW_PUBLIC
RMW_WARN_UNUSED
rmw_duration_t
rmw_time_total_nsec(const rmw_time_t time)
{
  static const uint64_t max_sec = INT64_MAX / RCUTILS_S_TO_NS(1);
  if (time.sec > max_sec) {
    // Seconds not representable in nanoseconds
    return INT64_MAX;
  }

  const int64_t sec_as_nsec = RCUTILS_S_TO_NS(time.sec);
  if (time.nsec > (uint64_t)(INT64_MAX - sec_as_nsec)) {
    // overflow
    return INT64_MAX;
  }
  return sec_as_nsec + time.nsec;
}

Results in:

=== R13: declare data objects at smallest possible level of scope
    globals used in one scope only:   0
    globals used in one file  only:   4
    bool    used in only file /root/src/spaceros_ws/src/rmw/rmw/src/t.c
    RMW_PUBLIC  used in only file /root/src/spaceros_ws/src/rmw/rmw/src/t.c
    RMW_WARN_UNUSED used in only file /root/src/spaceros_ws/src/rmw/rmw/src/t.c
    rmw_duration_t  used in only file /root/src/spaceros_ws/src/rmw/rmw/src/t.c
=== R14f: caller does not check return value: 1
t.c:9:
  1:      9    return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);
=== R14g: caller does not check return value: 1
t.c:9:
  1:      9    return rmw_time_total_nsec(left) == rmw_time_total_nsec(right);
=== R16: Nr of statements: 6
=== R16: Nr of assertions: 0
=== R16: the minimum number of assertions is 2% = 0
1 errors
nimble-code commented 2 years ago

Updated the implementation of Rule 14 (d,e,f,and g) to remove this false positive. The checker verified if the result of the fct call was used in an assignment, or in a condition (indicated by the call appearing inside round braces). The example used a condition without the round braces. The fix now checks if the fct call is preceded or followed by a boolean operator as well, to address this case. thanks for the report and the example!