[PATCH 2/8] score: Fix allocation size calculation

[Gedare Bloom]

On Thu, Sep 24, 2020 at 10:30 AM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Thu, Sep 24, 2020 at 6:13 AM Sebastian Huber
> <sebastian.huber at embedded-brains.de> wrote:
> >
> > The previous multiplication error check is broken on 64-bit machines.  Use the
> > recommended check from SEI CERT C Coding Standard, "INT30-C. Ensure that
> > unsigned integer operations do not wrap".
> >
> > Make sure the message size computation does not overflow.
> >
> > Update #4007.
> > ---
> >  cpukit/score/src/coremsg.c | 74 ++++++++++++--------------------------
> >  1 file changed, 23 insertions(+), 51 deletions(-)
> >
> > diff --git a/cpukit/score/src/coremsg.c b/cpukit/score/src/coremsg.c
> > index af8dbd6583..137d9973f4 100644
> > --- a/cpukit/score/src/coremsg.c
> > +++ b/cpukit/score/src/coremsg.c
> > @@ -19,28 +19,12 @@
> >  #endif
> >
> >  #include <rtems/score/coremsgimpl.h>
> > +#include <rtems/score/assert.h>
> >  #include <rtems/score/wkspace.h>
> >
> > -/*
> > - *  size_t_mult32_with_overflow
> > - *
> > - *  This method multiplies two size_t 32-bit numbers and checks
> > - *  for overflow.  It returns false if an overflow occurred and
> > - *  the result is bad.
> > - */
> > -static inline bool size_t_mult32_with_overflow(
> > -  size_t  a,
> > -  size_t  b,
> > -  size_t *c
> > -)
> > -{
> > -  long long x = (long long)a*b;
> > -
> > -  if ( x > SIZE_MAX )
> > -    return false;
> > -  *c = (size_t) x;
> > -  return true;
> > -}
> > +#define MESSAGE_SIZE_LIMIT \
> > +  ( SIZE_MAX - sizeof( uintptr_t ) - 1 \
> Minor: should it be - ( sizeof( uintptr_t ) - 1 )?
> Or: - sizeof(uintptr_t) + 1
>
> The alignment up can add at most sizeof(uintptr_t)-1 bytes overhead I
> think is what this is trying to capture?
>
> > +    - sizeof( CORE_message_queue_Buffer_control ) )
> >
> >  bool _CORE_message_queue_Initialize(
> >    CORE_message_queue_Control     *the_message_queue,
> > @@ -49,48 +33,36 @@ bool _CORE_message_queue_Initialize(
> >    size_t                          maximum_message_size
> >  )
> >  {
> > -  size_t message_buffering_required = 0;
> > -  size_t aligned_message_size;
> > +  size_t buffer_size;
> >
> >    the_message_queue->maximum_pending_messages   = maximum_pending_messages;
> >    the_message_queue->number_of_pending_messages = 0;
> >    the_message_queue->maximum_message_size       = maximum_message_size;
> >    _CORE_message_queue_Set_notify( the_message_queue, NULL );
> >
> > -  /*
> > -   * Align up the maximum message size to be an integral multiple of the
> > -   * pointer size.
> > -   */
> > -  aligned_message_size = RTEMS_ALIGN_UP(
> > -    maximum_message_size,
> > -    sizeof( uintptr_t )
> > -  );
> > -
> > -  /*
> > -   * Check for an integer overflow.  It can occur while aligning up the maximum
> > -   * message size.
> > -   */
> > -  if (aligned_message_size < maximum_message_size)
> > +  /* Make sure the message size computation does not overflow */
> > +  if ( maximum_message_size > MESSAGE_SIZE_LIMIT ) {
> >      return false;
> > +  }
> >
> > -  /*
> > -   *  Calculate how much total memory is required for message buffering and
> > -   *  check for overflow on the multiplication.
> > -   */
> > -  if ( !size_t_mult32_with_overflow(
> > -        (size_t) maximum_pending_messages,
> > -        aligned_message_size + sizeof(CORE_message_queue_Buffer_control),
> > -        &message_buffering_required ) )
> > +  buffer_size = RTEMS_ALIGN_UP( maximum_message_size, sizeof( uintptr_t ) );
> > +  _Assert( buffer_size >= maximum_message_size );
> > +
> > +  buffer_size += sizeof( CORE_message_queue_Buffer_control );
> > +  _Assert( buffer_size >= sizeof( CORE_message_queue_Buffer_control ) );
> > +
> > +  /* Make sure the memory allocation size computation does not overflow */
> > +  if ( maximum_pending_messages > SIZE_MAX / buffer_size ) {
>
> optimization: can we use mult instead?
>   if ( maximum_pending_messages * buffer_size > SIZE_MAX )
> save a few cycles...

Then again, maybe the division is needed here to ensure there isn't an
overflow later? This stuff gets a little tricky!

>
> >      return false;
> > +  }
> >
> > -  /*
> > -   *  Attempt to allocate the message memory
> > -   */
> > -  the_message_queue->message_buffers = (CORE_message_queue_Buffer *)
> > -     _Workspace_Allocate( message_buffering_required );
> > +  the_message_queue->message_buffers = _Workspace_Allocate(
> > +    (size_t) maximum_pending_messages * buffer_size
> ... and the compiler should propagate that value to here to save even
> more cycles. Or it could be put in a variable for simpler code.
>
>
> > +  );
> >
> > -  if (the_message_queue->message_buffers == 0)
> > +  if ( the_message_queue->message_buffers == NULL ) {
> >      return false;
> > +  }
> >
> >    /*
> >     *  Initialize the pool of inactive messages, pending messages,
> > @@ -100,7 +72,7 @@ bool _CORE_message_queue_Initialize(
> >      &the_message_queue->Inactive_messages,
> >      the_message_queue->message_buffers,
> >      (size_t) maximum_pending_messages,
> > -    aligned_message_size + sizeof( CORE_message_queue_Buffer_control )
> > +    buffer_size
> >    );
> >
> >    _Chain_Initialize_empty( &the_message_queue->Pending_messages );
> > --
> > 2.26.2
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel