[PATCH v2] tests/validation: Fix 64bit test failure

[Sebastian Huber]

On 15/02/2021 22:34, Martin Erik Werner wrote:

> [...]
> I'm however wondering if this is the right way to fix this...
>
> I'm guessing that the failure mentioned is based on this specification
> in rtems-central : spec/rtems/message/req/construct-errors.yml
>
> 393	- enabled-by: true
> 394	  post-conditions:
> 395	    Status: InvNum
> 396	  pre-conditions:
> 397	    Area: all
> 398	    AreaSize: all
> 399	    Id:
> 400	    - Id
> 401	    MaxPending:
> 402	    - Big
> 403	    MaxSize:
> 404	    - Valid
> 405	    Name:
> 406	    - Valid
> 407	    Queues:
> 408	    - Avail
>
> Which in practice seems to specify that
>
> rtems_message_queue_create
> (
> name,
> UINT32_MAX /* count */,
> 1 /* size */,
> attribute_set,
> &id
> );
>
> must fail with RTEMS_INVALID_NUMBER due to
>
> 117  - name: Big
> 118    test-code: |
> 119      ctx->config.maximum_pending_messages = UINT32_MAX;
> 120    text: |
> 121      The maximum number of pending messages of the message queue configuration
> 122      shall be big enough so that a calculation to get the message buffer
> 123      storage area size overflows.
>
> which in the code looks like
>
> /* Make sure the memory allocation size computation does not overflow */
> if ( maximum_pending_messages > SIZE_MAX / buffer_size ) {
>    return STATUS_MESSAGE_QUEUE_INVALID_NUMBER;
> }
>
> But when the SIZE_MAX is a 64bit size_t, then UINT32_MAX * (1 + buffer
> overhead) cannot reasonably overflow SIZE_MAX, so this will report
> success instead of the expected invalid number which is the failure
> seen in the validation test, is that correct?

Yes, this is correct.

The issue can be fixed in two ways.

1. We change the API, so that the error condition can happen also on 
64-bit architectures (current patch).

2. We change the specification and implementation, so that this error 
condition is removed on 64-bit architectures. In the implementation, 
this is easy. In the specification, this is a bit more difficult since I 
would have to introduce a new option which enables or disables parts of 
the specification based on the word size of the architecture (similar to 
RTEMS_SMP). This is the main reason why I didn't fix the issue immediately.

>
>
> If so, it seems very odd to change the interface just to allow this
> failure to occur.
>
> Would it be possible to instead specify that if
>
> SIZE_MAX >= UINT32_MAX * (1 + buffer overhead)
>
> then this case should be skipped, or expects success?

I would have probably fixed the issue without changing the API.

If we change the API, it should be consistent and all unnecessary cast 
should be removed. For example

rtems_status_code rtems_message_queue_create(
   rtems_name      name,
   uint32_t        count,
   size_t          max_message_size,
   rtems_attribute attribute_set,
   rtems_id       *id
);

should change as well.

-- 
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/