New Defects reported by Coverity Scan for RTEMS

[Joel Sherrill]

On Tue, Aug 30, 2022 at 9:53 AM Sebastian Huber <
sebastian.huber at embedded-brains.de> wrote:

>
>
> On 30/08/2022 16:37, Joel Sherrill wrote:
> >
> > On Tue, Aug 30, 2022 at 8:44 AM Sebastian Huber
> > <sebastian.huber at embedded-brains.de
> > <mailto:sebastian.huber at embedded-brains.de>> wrote:
> >
> >     On 30/08/2022 15:40, Joel Sherrill wrote:
> >      >
> >      >
> >      > On Tue, Aug 30, 2022 at 12:05 AM Sebastian Huber
> >      > <sebastian.huber at embedded-brains.de
> >     <mailto:sebastian.huber at embedded-brains.de>
> >      > <mailto:sebastian.huber at embedded-brains.de
> >     <mailto:sebastian.huber at embedded-brains.de>>> wrote:
> >      >
> >      >     On 30/08/2022 00:56, scan-admin at coverity.com
> >     <mailto:scan-admin at coverity.com>
> >      >     <mailto:scan-admin at coverity.com
> >     <mailto:scan-admin at coverity.com>> wrote:
> >      >      > ** CID 1512552:  High impact quality  (Y2K38_SAFETY)
> >      >      > /cpukit/score/src/kern_tc.c: 1804 in _Timecounter_Windup()
> >      >      >
> >      >      >
> >      >      >
> >      >
> >
>  ________________________________________________________________________________________________________
> >      >      > *** CID 1512552:  High impact quality  (Y2K38_SAFETY)
> >      >      > /cpukit/score/src/kern_tc.c: 1804 in _Timecounter_Windup()
> >      >      > 1798          /* Go live with the new struct timehands. */
> >      >      > 1799     #ifdef FFCLOCK
> >      >      > 1800          switch (sysclock_active) {
> >      >      > 1801          case SYSCLOCK_FBCK:
> >      >      > 1802     #endif
> >      >      > 1803                  time_second =
> th->th_microtime.tv_sec;
> >      >      >>>>      CID 1512552:  High impact quality  (Y2K38_SAFETY)
> >      >      >>>>      A "time_t" value is stored in an integer with too
> few
> >      >     bits to accommodate it.  The expression "th->th_offset.sec"
> >     is cast
> >      >     to "int32_t".
> >      >      > 1804                  time_uptime = th->th_offset.sec;
> >      >      > 1805     #ifdef FFCLOCK
> >      >      > 1806                  break;
> >      >      > 1807          case SYSCLOCK_FFWD:
> >      >      > 1808                  time_second =
> >     fftimehands->tick_time_lerp.sec;
> >      >      > 1809                  time_uptime =
> >      >     fftimehands->tick_time_lerp.sec - ffclock_boottime.sec;
> >      >      >
> >      >      > ** CID 1512551:    (Y2K38_SAFETY)
> >      >
> >      >     This seems to be a new Coverity feature. The Newlib time_t
> >      >     definition is:
> >      >
> >      >     #if defined(_USE_LONG_TIME_T) || __LONG_MAX__ > 0x7fffffffL
> >      >     #define _TIME_T_ long
> >      >     #else
> >      >     #define _TIME_T_ __int_least64_t
> >      >     #endif
> >      >     typedef _TIME_T_        __time_t;
> >      >
> >      >     Does Coverity use the Newlib header files? The
> >     _USE_LONG_TIME_T should
> >      >     be undefined for RTEMS.
> >      >
> >      >
> >      > Yes it should. It works by doing something like this:
> >      >
> >      > cov-build waf|make
> >      >
> >      > And looking at the newlib headers, I agree everything looks like
> >     it should
> >      > be defined to _int_least64_t. And preprocessing a simple file
> >     that includes
> >      > <sys/time.h> shows that it is typed to that.
> >      >
> >      > But.... something else is going on. time_uptime is defined to
> >      > _Timecounter_Time_uptime
> >      > which is an int32_t.
> >
> >     Oh, sorry, I didn't notice this. Then this is a false positive.
> >     Using an
> >     int32_t for uptime seconds is enough.
> >
> >
> > The variable  time_uptime is still defined as two separate types. The
> > prototype and the declaration do not match.
>
> I think this is all right:
>
> #ifndef __rtems__
> volatile time_t time_second = 1;
> volatile time_t time_uptime = 1;
> #else /* __rtems__ */
> volatile time_t time_second = TOD_SECONDS_1970_THROUGH_1988;
> volatile int32_t time_uptime = 1;
> #endif /* __rtems__ */
>
> >
> > Even if int32_t is wide enough for seconds of uptime, it is an assignment
> > that narrows types. Casting to make this narrowing intentional would at
> > least hint this was intentional.
>
>  From looking at the other Y2K38_SAFETY reports, a cast probably doesn't
> help to get rid of it.
>
> >
> > But better would be to use the proper type and just make it time_t like
> > FreeBSD.
>
> The time_uptime is used in the libbsd. This is a performance optimization.
>

And the declaration you show is mismatched because of these:

cpukit/include/machine/_kernel_time.h:#define   time_uptime
_Timecounter_Time_uptime
cpukit/include/rtems/score/timecounter.h:extern volatile int32_t
_Timecounter_Time_uptime;

This is inconsistent with the declaration of time_uptime above.


>
> There is also an unsolved issue with the time_second. On 32-bit targets
> you can't reliably read this value.
>

I assume you mean because it will take two accesses?

>
> --
> embedded brains GmbH
> Herr Sebastian HUBER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: sebastian.huber at embedded-brains.de
> phone: +49-89-18 94 741 - 16
> fax:   +49-89-18 94 741 - 08
>
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20220830/04979c5c/attachment-0001.htm>