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

Fix #2488, separate bad argument test #2489

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

jphickey
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Pass in CFE_SB_INVALID_MSG_ID separately from a NULL pointer. This also required updates to the way the MSG module checks the MsgID, as it was not using the IsValidMsgID check.

Fixes #2488

Testing performed
Build and run all tests

Expected behavior changes
CFE_MSG_Init() will now return the BAD_ARGUMENT code if passed CFE_SB_INVALID_MSG_ID. Oddly, this was not the case previously, it would still return CFE_SUCCESS. This is because the MSG implementation wasn't using the same check function (CFE_SB_IsValidMsgID). This PR corrects that.

System(s) tested on
Debian

Additional context
This is correct behavior now, but its one of those things where I can't be totally sure if some app was relying on the incorrect behavior.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@@ -56,7 +56,7 @@
{
CFE_SB_MsgId_Atom_t msgidval = CFE_SB_MsgIdToValue(MsgId);

if (MsgPtr == NULL || msgidval > CFE_PLATFORM_SB_HIGHEST_VALID_MSGID)
if (MsgPtr == NULL || !CFE_SB_IsValidMsgId(MsgId))

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
Pass in CFE_SB_INVALID_MSG_ID separately from a NULL pointer.
This also required updates to the way the MSG module checks the
MsgID, as it was not using the IsValidMsgID check.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Dec 19, 2023
@jphickey
Copy link
Contributor Author

@skliper --- do you know why the original "SetMsgId()" only checked against HIGHEST_VALID_MSGID instead of using the check implemented in SB? Was there a justification for having a different/lesser validation of MsgId in this particular code?

I'm curious because I don't see any real reason it should be more permissive, yet there seemed to be coverage tests checking for the fact that a msgID value of "0" can be successfully set.

@skliper
Copy link
Contributor

skliper commented Dec 19, 2023

I think that's just left over from when CFE_SB_INVALID_MSG_ID was not zero. I think at some point in history it was in a config file. With it now in the core_api the current tests to check for 0 being valid don't make sense to me.

@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Jan 11, 2024
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 17, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request Jan 17, 2024
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
@dzbaker dzbaker merged commit dfcc3e0 into nasa:main Jan 18, 2024
22 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Jan 18, 2024
*Combines:*

sample_app equuleus-rc1+dev36
sch_lab equuleus-rc1+dev29
to_lab equuleus-rc1+dev38
sample_lib equuleus-rc1+dev2
tblCRCTool equuleus-rc1+dev2
cFS-GroundSystem equuleus-rc1+dev2
elf2cfetbl equuleus-rc1+dev10
cFE equuleus-rc1+dev75
PSP equuleus-rc1+dev38
osal equuleus-rc1+dev33

**Includes:**

*sample_app*
- nasa/sample_app#226

*sch_lab*
- nasa/sch_lab#161

*to_lab*
- nasa/sch_lab#186

*sample_lib*
- nasa/sample_lib#96

*tblCRCTool*
- nasa/tblCRCTool#80

*cFS-GroundSystem*
- nasa/cFS-GroundSystem#239

*elf2cfetbl*
- nasa/elf2cfetbl#144
- nasa/elf2cfetbl#143

*cFE*
- nasa/cFE#2463
- nasa/cFE#2486
- nasa/cFE#2485
- nasa/cFE#2489

*PSP*
- nasa/PSP#422
- nasa/PSP#427

*osal*
- nasa/osal#1437
- nasa/osal#1442

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Dylan Baker <dzbaker@users.noreply.github.com>
@jphickey jphickey deleted the fix-2488-msginit-args branch January 18, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSG testcase passes in two bad arguments at once
3 participants