Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CFE_SB_GetLastSenderID will crash if if called before message sent on pipe #494

Closed
ghost opened this issue Jan 27, 2020 · 2 comments · Fixed by #605
Closed

CFE_SB_GetLastSenderID will crash if if called before message sent on pipe #494

ghost opened this issue Jan 27, 2020 · 2 comments · Fixed by #605
Assignees
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Jan 27, 2020

CFE_SB_GetLastSenderID assumes that CFE_SB.PipeTbl[PipeId].CurrentBuff is not NULL. Upon CFE_SB_CreatePipe, this will be null. CurrentBuff is only set upon receiving a message in CFE_SB_RcvMsg(). So, if CFE_SB_GetLastSenderID is called before receiving a message the program will crash.

This is Steven Seeger from GSFC. Guess I have a personal account on github. :)

@skliper skliper added the bug label Jan 27, 2020
@skliper skliper added this to the 6.8.0 milestone Jan 27, 2020
@jwilmot
Copy link

jwilmot commented Feb 12, 2020

This should be fixed. A user application error (last sender API called early) should not cause the cFE to crash. Recommend returning error and setting last sender to a known invalid id. uint32 0xFFFFFFFF is a possible choice.

@jphickey
Copy link
Contributor

Please note that with respect to the "invalid message id" notion, there are a at least two other other related changes waiting in the wings that should be coordinated with this:

  1. expose CFE_SB_IsValidMsgId() #263 is to provide a CFE_SB_IsValidMsgId() function in the public SB API (currently private for some unknown reason)
  2. Type safety and improved handling of CFE_SB_MsgId_t values #245 is to provide type safety for CFE_SB_MsgId_t so it isn't a "bare number" that can be interchanged with other integers.

Lastly, note that there is an existing definition for an invalid CFE_SB_MsgId_t value, and it is currently:

#define CFE_SB_INVALID_MSG_ID           0xFFFF /**< \brief Initializer for CFE_SB_MsgId_t values that will not match any real MsgId */

In particular I'm bringing all this up because one must resist the temptation to do something like:
if (MsgId == CFE_SB_INVALID_MSG_ID).... bur rather the SB API function should be used to test the MsgId.

And secondly don't assume CFE_SB_MsgId_t will always remain a simple integer. Assignments are OK (i.e. MsgId = CFE_SB_INVALID_MSG_ID will need to work) but addition/subtraction/comparison may NOT work. Testing and manipulation of the value should always go through the SB API.

@dmknutsen dmknutsen self-assigned this Apr 9, 2020
dmknutsen added a commit to dmknutsen/cFE that referenced this issue Apr 13, 2020
dmknutsen added a commit to dmknutsen/cFE that referenced this issue Apr 14, 2020
astrogeco added a commit that referenced this issue Apr 21, 2020
Fix #494, Updates CFE_SB_GetLastSenderID to check if message has been sent on pipe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants