search

nasa / osal

The Core Flight System (cFS) Operating System Abstraction Layer (OSAL)
Apache License 2.0
546 stars 213 forks source link

Array access overflow in src/os/vxworks/src/os-impl-idmap.c #1304

Closed jhnphm closed 1 year ago

jhnphm commented 1 year ago

Describe the bug Array access overflow in src/os/vxworks/src/os-impl-idmap.c

To Reproduce Enable FM in cFS on VxWorks. Streaming semTake/semGive errors should occur when OSAL_CONFIG_DEBUG_PRINTF is enabled

Expected behavior Error printouts should not occur

Code snips This PR adds the following lines: https://github.com/nasa/osal/blob/b8e9b83c9a09c8e47ee796105d2a1aed3001db56/src/os/shared/src/osapi-idmap.c#L163-L164

This allows OS_OBJECT_TYPE_OS_CONDVAR to become a valid index for the OS_ObjectIdIteratorInit()->OS_ObjectIdTransactionInit()->OS_Lock_Global()->OS_Lock_Global_Impl() call chain. Unfortunately, OS_impl_objtype_lock_table for VxWorks does not contain an entry for OS_OBJECT_TYPE_OS_CONDVAR:

https://github.com/nasa/osal/blob/b8e9b83c9a09c8e47ee796105d2a1aed3001db56/src/os/vxworks/src/os-impl-idmap.c#L74-L87

OS_Lock_Global_Impl/OS_Unlock_Global_Impl runs past the end of the array, which results in invalid vxids being passed to semTake()/semGive()

System observed on:

Additional context

Reporter Info John N Pham, Northrop Grumman

jphickey commented 1 year ago

Thanks for catching this, this was an oversight in the original change. Since VxWorks 6 does not support condvars there is no real need for a lock for this objtype because it uses the "no-condvar" implementation.

The problem is that this function unconditionally tries to take the lock, regardless of whether the lock exists or not. It was assumed in OS_Lock_Global_Impl that all defined object types would have a corresponding lock, but if a particular object type is not being supported then this lock can (validly) not exist.

To fix the immediate issue I'd propose to add a null check on the table entry, and just skip the lock if so. This is OK as long as the "no-condvar" implementation is used, as that does not need a real lock.

jphickey commented 1 year ago

Actually, even in the case of a non-implementation it still goes through the motions of allocating a table slot (i.e. OS_ObjectIdFindNextFree is still called, it just won't actually be successful later on). This is necessary to avoid conditional compilation among other things.

So the best bet is probably to just instantiate the missing lock....