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

If a message is subscribed, then unsubscribed, additional unsubscribes do not raise error events #862

Closed
johnphamngc opened this issue Sep 3, 2020 · 5 comments · Fixed by #865 or #876
Assignees
Labels
Milestone

Comments

@johnphamngc
Copy link

Describe the bug
If a message is subscribed, then unsubscribed, additional unsubscribes do not raise error events

To Reproduce
Have SB subscribe to a message
Have SB unsubscribe to that message
Have SB unsubscribe again to that message

Expected behavior
Event message should be raised

Code snips

/* get index into routing table */
MsgKey = CFE_SB_ConvertMsgIdtoMsgKey(MsgId);
RouteIdx = CFE_SB_GetRoutingTblIdx(MsgKey);
/* if there are no subscriptions for this message id... */
if(!CFE_SB_IsValidRouteIdx(RouteIdx)){
char PipeName[OS_MAX_API_NAME] = {'\0'};
CFE_SB_UnlockSharedData(__func__,__LINE__);
CFE_SB_GetPipeName(PipeName, sizeof(PipeName), PipeId);
CFE_EVS_SendEventWithAppID(CFE_SB_UNSUB_NO_SUBS_EID,CFE_EVS_EventType_INFORMATION,CFE_SB.AppId,
"Unsubscribe Err:No subs for Msg 0x%x on %s,app %s",
(unsigned int)CFE_SB_MsgIdToValue(MsgId),
PipeName,CFE_SB_GetAppTskName(TskId,FullName));
return CFE_SUCCESS;
}/* end if */
/* At this point, there must be at least one destination. */
/* So the value of 'ListHeadPtr' will not be NULL by design */
RoutePtr = CFE_SB_GetRoutePtrFromIdx(RouteIdx);
/* search the list for a matching pipe id */
DestPtr = RoutePtr->ListHeadPtr;
do{
if(DestPtr->PipeId == PipeId){
/* match found, remove node from list */
CFE_SB_RemoveDest(RoutePtr,DestPtr);
/* return node to memory pool */
CFE_SB_PutDestinationBlk(DestPtr);
RoutePtr->Destinations--;
CFE_SB.StatTlmMsg.Payload.SubscriptionsInUse--;
MatchFound = true;
}/* end if */
DestPtr = DestPtr->Next;
}while((MatchFound == false)&&(DestPtr != NULL));
CFE_SB_UnlockSharedData(__func__,__LINE__);
CFE_EVS_SendEventWithAppID(CFE_SB_SUBSCRIPTION_REMOVED_EID,CFE_EVS_EventType_DEBUG,CFE_SB.AppId,
"Subscription Removed:Msg 0x%x on pipe %d,app %s",
(unsigned int)CFE_SB_MsgIdToValue(MsgId),
(int)PipeId,CFE_SB_GetAppTskName(TskId,FullName));
return CFE_SUCCESS;

If a message was previously subscribed to, and all pipes subscribing to it were subsequently unsubscribed, and an additional unsubscribe is issued, a valid routing index would still exist in the msgmap table, and thus an error event would not be raised.

System observed on:
N/A, discovered via code inspection

Additional context
N/A

Reporter Info
John N Pham, Northrop Grumman

@CDKnightNASA CDKnightNASA self-assigned this Sep 4, 2020
@CDKnightNASA
Copy link
Contributor

This is likely a side-effect of a fix I implemented in #101

@skliper
Copy link
Contributor

skliper commented Sep 4, 2020

Yeah, actually looks to me like this will seg fault (tries to access NULL pointer). Note I'd like to add this as a test scenario to the new functional test framework (part of subscribe/unsubscribe API testing).

@skliper skliper added the bug label Sep 4, 2020
@skliper skliper added this to the 7.0.0 milestone Sep 4, 2020
@CDKnightNASA
Copy link
Contributor

Yeah, actually looks to me like this will seg fault (tries to access NULL pointer). Note I'd like to add this as a test scenario to the new functional test framework (part of subscribe/unsubscribe API testing).

I'm re-designing; can you point at the line you expect a seg fault?

@skliper
Copy link
Contributor

skliper commented Sep 4, 2020

I don't think 1078 is true now, so 1083 will == NULL, and any of the following references to it would fault. Didn't test, but that's what it looks like to me upon inspection.

/* At this point, there must be at least one destination. */
/* So the value of 'ListHeadPtr' will not be NULL by design */
RoutePtr = CFE_SB_GetRoutePtrFromIdx(RouteIdx);
/* search the list for a matching pipe id */
DestPtr = RoutePtr->ListHeadPtr;

@CDKnightNASA
Copy link
Contributor

I don't think 1078 is true now, so 1083 will == NULL, and any of the following references to it would fault. Didn't test, but that's what it looks like to me upon inspection.

/* At this point, there must be at least one destination. */
/* So the value of 'ListHeadPtr' will not be NULL by design */
RoutePtr = CFE_SB_GetRoutePtrFromIdx(RouteIdx);
/* search the list for a matching pipe id */
DestPtr = RoutePtr->ListHeadPtr;

Ah, yes...I am guessing RoutePtr will always be != NULL, but ListHeadPtr could be NULL and yes line 1087 would segfault. My redesign avoids this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants