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.

Sign up for GitHub

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

Jump to bottom

Fix #1189, correct compiler warnings #1197

Merged
merged 1 commit into from
Mar 3, 2021
+15 −18
Merged

Fix #1189, correct compiler warnings #1197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions fsw/cfe-core/src/inc/private/cfe_sbr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@
#include "cfe_msg_typedefs.h"
#include "cfe_platform_cfg.h"

/*
* Macro Definitions
*/

/** \brief Invalid route id */
#define CFE_SBR_INVALID_ROUTE_ID ((CFE_SBR_RouteId_t) {.RouteId = 0})

/******************************************************************************
* Type Definitions
*/
Expand Down
2 changes: 2 additions & 0 deletions fsw/cfe-core/src/sb/cfe_sb_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,7 @@ int32 CFE_SB_TransmitMsg(CFE_MSG_Message_t *MsgPtr, bool IncrementSequenceCount

PendingEventID = 0;
BufDscPtr = NULL;
RouteId = CFE_SBR_INVALID_ROUTE_ID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about having CFE_SB_TransmitMsgValidate actually set RouteId (actually done by the underlying GetRouteId call)? Then it fixes everywhere the GetRouteId call is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that too, but I see it as a separate problem ...

  1. A caller should never rely on/use output values from a function that returned non-success
  2. A function should always set its output buffers to something, even when it returns non-success

I see (1) as the main issue, and (2) as the backup/failsafe. I have no objection to doing both, I just see it as a secondary concern. I can add it though.

Copy link
Contributor

@skliper skliper Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In virtually every case I've seen that the static analyzer or compiler warning for possible uninitialized variable comes up 1 is met by the caller by either returning or checking status prior to using what is returned. 2 then is both good practices and squashes the warning. Is initializing the variable in this case really required where 1 is already satisfied by logic?

This seems even more useless when the value being initialized is not checked explicitly before used, and would cascade into other issues down the line if there WAS a possible way to get to that code. Basically, if you are checking for status == SUCCESS but depend on RouteId being valid why not check instead (or in addition if both cases need to be true) for RouteId being valid (that also squashes the warning).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for the case where the code IS checking for RouteId to be valid, I definitely agree explicit initialization is the way to go. I just struggle with justifying initialization if it's never accessed based on failure logic. Feels cleaner to me (and would be easier for me to maintain) when initialization is done locally when it matters (and squash errors via setting outputs in the underlying APIs when it doesn't matter).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skliper I added this in commit bf8f481 however I have my concerns/reservations - there is a chance that the original passed-in buffers themselves are NULL, where the status code becomes CFE_SB_BAD_ARGUMENT ... this will then try to write a value to the buffer. Could add NULL checks on each, but then this is just getting overly complicated at that point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if:

cFE/fsw/cfe-core/src/sb/cfe_sb_api.c

Lines 1536 to 1541 in bf8f481

/* check input parameter */
if (MsgPtr == NULL)
{
PendingEventID = CFE_SB_SEND_BAD_ARG_EID;
Status = CFE_SB_BAD_ARGUMENT;
}

Is really the sweet spot for checking, why not just add the else and set the outputs to defaults here instead of at the end? Then it avoids setting them for the CFE_SB_BAD_ARGUMENT case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, anything is possible, if you want to do that. Each output could be individually checked for NULL and assigned a default if not null. If it helps meet a checkbox on some review procedure, that's well and good - but it doesn't make sense in the overall design of the software, it is just unnecessary extra complexity for no actual benefit (and cpu cycles burnt in code that actually does execute rather frequently in CFE)

I'm just taking a more holistic view of the software. This is an internal helper function, called only from the two public "TransmitMsg" variants, in a specific scenario, to consolidate the common logic.

The only reason that MsgPtr is explicitly checked is because of the fact that (for some reason?) public APIs are all supposed to accept and detect a NULL pointer input. This assumes the role of checking that input.

For the other 3 - those are outputs to stack variables. They are stack variables in the parent - never NULL. This is not a public API - they aren't going to be randomly called with arbitrary values in a general-purpose sense. Again, the role of this function is to consolidate common code between CFE_SB_TransmitMsg and CFE_SB_TransmitBuffer, initializing local stack variables on behalf of the caller, such that the code is not repeated between the two.

By definition, any NULL pointer check here is impossible to get at runtime - thereby making it dead code. The way I see it this is very similar to the recent case in OSAL where a check for an impossible condition was removed due to the code before it. This is the exact same thing, just in a helper function. The helper is not invoked with NULL args, so any check for NULL args is immediately dead/unreachable code.

Copy link
Contributor

@skliper skliper Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was you're comment about "there is a chance that the original passed-in buffers themselves are NULL, where the status code becomes CFE_SB_BAD_ARGUMENT" that triggered my comments... so can they be NULL or not? If not, the current implementation is fine (I already approved that one when I first saw it.) If they can be NULL, then check for it. Alternatively, just set the passed in pointers as the else of the MsgPtr check. Are all 3 of these possible approaches really not acceptable and the only option is to not initialize these values? I still don't follow why there is any opposition? Maybe contact me for a chat?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that - that was only my initial knee-jerk before looking closer and seeing that the function really only does need to handle MsgPtr being null and not the others. To allow the others to be null would be pointless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy. And I'm fine w/ how it's currently just checking MsgPtr (probably would benefit from a comment to that affect, but implementation is fine) based on the background you provided.


Status = CFE_SB_TransmitMsgValidate(MsgPtr, &MsgId, &Size, &RouteId);

Expand Down Expand Up @@ -1886,6 +1887,7 @@ int32 CFE_SB_ReceiveBuffer(CFE_SB_Buffer_t **BufPtr,
BufDscPtr = NULL;
DestPtr = NULL;
BufDscSize = 0;
RcvStatus = OS_SUCCESS;

/*
* Check input args and see if any are bad, which require
Expand Down
18 changes: 6 additions & 12 deletions fsw/cfe-core/src/sb/cfe_sb_priv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,15 +536,7 @@ void CFE_SB_RemoveDestNode(CFE_SBR_RouteId_t RouteId, CFE_SB_DestinationD_t *Nod
int32 CFE_SB_ZeroCopyReleaseAppId(CFE_ES_AppId_t AppId)
{
CFE_SB_BufferLink_t *NextLink;

/* Using a union type permits promotion of CFE_SB_BufferLink_t* to CFE_SB_BufferD_t*
* without violating aliasing rules or alignment rules. Note that the first element
* of CFE_SB_BufferD_t is a CFE_SB_BufferLink_t, which makes this possible. */
union LocalBufDesc
{
CFE_SB_BufferLink_t As_Link;
CFE_SB_BufferD_t As_Dsc;
} *BufPtr;
CFE_SB_BufferD_t *DscPtr;

/*
* First go through the "ZeroCopy" tracking list and find all nodes
Expand All @@ -560,16 +552,18 @@ int32 CFE_SB_ZeroCopyReleaseAppId(CFE_ES_AppId_t AppId)
while(!CFE_SB_TrackingListIsEnd(&CFE_SB_Global.ZeroCopyList, NextLink))
{
/* Get buffer descriptor pointer */
BufPtr = (union LocalBufDesc*)NextLink;
/* NOTE: casting via void* here rather than CFE_SB_BufferD_t* avoids a false
* alignment warning on platforms with strict alignment requirements */
DscPtr = (void *)NextLink;

/* Read the next link now in case this node gets moved */
NextLink = CFE_SB_TrackingListGetNext(NextLink);

/* Check if it is a zero-copy buffer owned by this app */
if (CFE_RESOURCEID_TEST_EQUAL(BufPtr->As_Dsc.AppId, AppId))
if (CFE_RESOURCEID_TEST_EQUAL(DscPtr->AppId, AppId))
{
/* If so, decrement the use count as the app has now gone away */
CFE_SB_DecrBufUseCnt(&BufPtr->As_Dsc);
CFE_SB_DecrBufUseCnt(DscPtr);
}
}

Expand Down
6 changes: 0 additions & 6 deletions modules/sbr/private_inc/cfe_sbr_priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,6 @@
*/
#include "private/cfe_sbr.h"

/*
* Macro Definitions
*/

/** \brief Invalid route id */
#define CFE_SBR_INVALID_ROUTE_ID ((CFE_SBR_RouteId_t) {.RouteId = 0})

/******************************************************************************
* Function prototypes
Expand Down