Mbed-os: EventQueue bug with signed `ms` value in `call_in` and friends

[maxgerhardt]

Description of defect

The API for EventQueue::call_in takes a int ms (int32_t) argument which describes when the target callback shall be executed. This is then given to equeue_event_delay() which copies the int ms value into the e->target field, which is a unsigned -- a guard against negative millisecond values never happens. Or rather, the type of ms should be changed.

See code

https://github.com/ARMmbed/mbed-os/blob/727cf54873a970017a7be0cf466b0f76bb21caf9/events/equeue.h#L51

and https://github.com/ARMmbed/mbed-os/blob/727cf54873a970017a7be0cf466b0f76bb21caf9/events/source/equeue.c#L577-L582

And

https://github.com/ARMmbed/mbed-os/blob/727cf54873a970017a7be0cf466b0f76bb21caf9/events/EventQueue.h#L64

notice how you choose the representation rep to be int, thus also in

https://github.com/ARMmbed/mbed-os/blob/727cf54873a970017a7be0cf466b0f76bb21caf9/events/EventQueue.h#L828-L840

the result of ms.count() will be int (docs)

And thus a int is passed in equeue_event_delay() which takes it and writes it into a unsigned member -- an internal integer overflow will occur and -1 would be interpreted as 0xFFFFFFFF milliseconds aka 49.7 days.

Target(s) affected by this defect ?

Every target.

Toolchain(s) (name and version) displaying this defect ?

Every toolchain

What version of Mbed-os are you using (tag or sha) ?

727cf54873a970017a7be0cf466b0f76bb21caf9

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli

How is this defect reproduced ?

#include <mbed.h>
#include <EventQueue.h>
using namespace events;

#define MAX_NUMBER_OF_EVENTS 16
EventQueue ev_queue(MAX_NUMBER_OF_EVENTS * EVENTS_EVENT_SIZE);

void target_function() {
    printf("Target function reached!\n");
}

void main() {
    ev_queue.call_in(-1, &target_function);
    ev_queue.dispatch_forever();
}

[ciarmcom]

@maxgerhardt thank you for raising this issue.Please take a look at the following comments:

We cannot automatically identify a release based on the version of Mbed OS that you have provided.
Please provide either a single valid sha of the form #abcde12 or #3b8265d70af32261311a06e423ca33434d8d80de
or a single valid release tag of the form mbed-os-x.y.z .
E.g. '(727cf54873a970017a7be0cf466b0f76bb21caf9)' has not been matched as a valid tag or sha.
NOTE: If there are fields which are not applicable then please just add 'n/a' or 'None'.This indicates to us that at least all the fields have been considered.
Please update the issue header with the missing information, the issue will not be mirroredto our internal defect tracking system or investigated until this has been fully resolved.

[maxgerhardt]

Updated

[kjbracey-arm]

I would say that the prototype of call_in should be changed so that is unsigned, but that just matches, documents and preserves current behaviour. The test program would behave exactly the same.

It is possible there are programs out there that are happily calculating delays bigger than 24 days, feeding them to call_in via a wrapped int, and relying on this behaviour. I don't think I'd want to halve the range here.

Pretty much all other core 32-bit relative-time APIs do take uint32_t or a duration<uint32_t, milli> - these signed millisecond ones are anomalies.

[maxgerhardt]

I would say that the prototype of call_in should be changed so that is unsigned, but that just matches, documents and preserves current behaviour. The test program would behave exactly the same.

Indeed I think that is what should be done here -- just match the signature of the outer functions to what is going in on the inside. The equeue_event_delay() function shouldn't take an int ms and write it into unsigned target, but have a unsigned ms argument instead.

But is that really documented? Looking into https://os.mbed.com/docs/mbed-os/v6.0/mbed-os-api-doxy/classevents_1_1_event_queue.html, there's no mention of a 49 day limit (or that negative values will be interpreted as being beyond 24 days).

[kjbracey-arm]

No, it's not documented. But changing the prototype to unsigned would serve as documentation. "Pass -1 to this, and you'll get 2^32-1 ms".

[0xc0170]

cc @ARMmbed/mbed-os-core

[evedon]

I also agree with changing the prototype to unsigned. The fix could go in a patch release as this would not affect current behaviour.

[ciarmcom]

@maxgerhardt it has been 5 days since the last reminder. Could you please update the issue header as previously requested?

[maxgerhardt]

Your CI bot is really broken if it can't recognize the text 727cf54873a970017a7be0cf466b0f76bb21caf9 as commit hash. It even shows the git commit hash link in the text for "did not recognize" 😆. Trying with a # prefix now.

[0xc0170]

@maxgerhardt Yes, there is an issue. its rather related to released version and mapping sha to it. rather than not recognizing the sha itself. will be fixed

[adbridge]

@maxgerhardt as @0xc0170 said it is because the bot is designed to determine the last released version of software to contain the sha and as this sha is so far in unreleased code, it can't obviously find a release. This is a known issue that we are working on, please bear with us.

[maxgerhardt]

If it isn't critical then it's not in issue. The bot did however say that without a correctly formed SHA, it wouldn't mirror it to the internal repo where the actual work is done. Ignoring the bot then.

[maxgerhardt]

Also, closing upon merging of the patch into the next patch release.

[ciarmcom]

@maxgerhardt it has been 5 days since the last reminder. Could you please update the issue header as previously requested?

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2288