On Thu, Sep 24, 2020 at 10:30 AM Gedare Bloom <ged...@rtems.org> wrote:
>
> On Thu, Sep 24, 2020 at 6:13 AM Sebastian Huber
> <sebastian.hu...@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@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
