Fix #494, Updates CFE_SB_GetLastSenderID to check if message has been sent on pipe #605
Conversation
| *Ptr = (CFE_SB_SenderId_t *) &Ptr2BufDescriptor -> Sender; | ||
| CFE_SB_UnlockSharedData(__func__,__LINE__); | ||
| return CFE_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Actually, this API is fundamentally broken. I had never really looked at it before.
The content of CFE_SB.PipeTbl[PipeId].CurrentBuff is shared, and needs to be locked while accessing it. (which it is, within this function). But then it is gettng a direct pointer to the data within the descriptor, unlocking, and returning that pointer for the application to access. This is unsafe - that data can be overwritten at any time after the unlock.
Rather, this should take a pointer to the CFE_SB_SenderId_t buffer (not a pointer-to-pointer) and copy the data, then unlock.
Of course, this changes the API, and makes it incompatible.
One possible mitigation to make it backward compatible is to have a separate buffer in the PipeTbl struct holding another copy, then copy the data into that and return a pointer to it. That way at least the returned pointer would remain valid until the next call to the same function on the same pipe (sort of like the way the posix readdir issue was mitigated).
There was a problem hiding this comment.
Agree with the general philosophy. I just wanted to point out that the documentation does state that *Ptr is only valid until the next call to CFE_SB_RcvMsg for the same pipe.
fsw/cfe-core/src/inc/cfe_sb_events.h
Outdated
| ** descriptor for the last msg received for a given pipe is NULL. This usually occurs because | ||
| ** CFE_SB_GetLastSenderId was called prior to recieving a msg. | ||
| **/ | ||
| #define CFE_SB_NO_LAST_MESG_RECV_EID 64 |
There was a problem hiding this comment.
Does this really warrant an "error event"? Seems like normal operational context - there is nothing inherently an "error" about a pipe that exists but hasn't been sent to yet.
I can foresee this API being used to simply check if a pipe has been written to or not, where so long as the application handles the return code its not an error either way, and generating an "Error event" might be confusing.
There was a problem hiding this comment.
Just removed error event and pushed the updates.
skliper
added
the
CCB:Ready
…s been sent on pipe
astrogeco
changed the title
|
CCB 20200415 - APPROVED |
astrogeco
added
CCB:Approved
Describe the contribution
Updates CFE_SB_GetLastSenderID to check if a message has been sent on the associated pipe prior to setting the receivers pointer to the associated address of 'sender' in the buffer descriptor.
Includes associated unit test addition to cover the additional path added to CFE_SB_GetLastSenderID.
Includes an addition of a SB status code and event ID.
Fix #494
Testing performed
Expected behavior changes
CFE_SB_GetLastSenderID will now detect if it is being called prior to a message being sent on a given pipe.
System(s) tested on
Oracle VM VirtualBox
OS: ubuntu-19.10
Versions: cFE 6.7.11.0, OSAL 5.0.9.0, PSP 1.4.7.0
Contributor Info
Dan Knutsen
NASA/Goddard