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

SB Pipes are not protected. #100

Open
skliper opened this issue Sep 30, 2019 · 6 comments
Open

SB Pipes are not protected. #100

skliper opened this issue Sep 30, 2019 · 6 comments

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

It is possible for a task to fetch messages from an SB Pipe
that was created by some other task, and this can happen
easily if someone fetches using ZERO as the pipe ID.

This prevents the consumed message from arriving at its
intended destination.

Note that applications fetching from the wrong Message ID
might notice this issue if they are reporting the receipt
of messages with unexpected Message ID numbers.

Solving this may require some attention to use cases where
a pipe is created by one task, to be used by another.

We may also want to think about the use case where multiple
tasks fetch messages from a single pipe, with the intention
that messages go to one of the several tasks. Is this use
case sufficiently interesting to offset the complexity of the
code required to support it?

@skliper skliper added this to the 6.8.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 69. Created by glimes on 2015-06-16T13:37:21, last modified: 2019-07-03T12:58:32

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-16 13:38:49:

Placing this ticket in "review" state to give folks
a chance to add information and discussion.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2018-05-14 12:03:03:

I've reviewed this ticket again in the context of cFE 6.6. It probably warrants //something// to be done here, to limit the risk of accidentally reading the wrong pipe ID.

We cannot simply limit pipe IDs to the task that created it, since it is a common use-case where one task, such as a parent/init task, creates pipe IDs that are later used by child tasks.

My Proposal:

  • Use a type-safe abstraction for the externally-used CFE_SB_PipeId_t, similar to what was implemented for CFE_SB_MsgId_t in Type safety and improved handling of CFE_SB_MsgId_t values #245
  • Make sure that the abstract type will not alias a valid pipe ID after being memset() to all zeros.
  • Scrub existing code for CFE_SB_GetPipeIdx() function and checking against CFE_SB_INVALID_PIPE. This is currently defined as 0xFF, it would be preferable to use 0 instead to avoid aliasing by mistake.

NOTE: In reviewing this I see suspicious mixture of PipeId values as direct array indices. The CFE_SB_GetPipeIdx() function does a sequential search of a global table to find a matching pipe ID value. However, in other functions, such as CFE_SB_ValidatePipeId and CFE_SB_GetPipePtr, the Pipe ID value is used directly to index the same table.

If a pipe ID can only exist at a given slot of the table, it would suggest that the GetPipeIdx function is an unnecessary waste of CPU cycles.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-05 09:21:13:

CCB - "safer" code category, no intended functionality change. Review design suggestion.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-04-03 17:23:54:

CCB 4/3/19 - Discussion

  • Make 0 invalid
  • Scrub needed for array indices vs alias (may have been resolved)

These two above actions are sufficient to close this ticket.

Opened ticket #304 to consider an operational requirement to restrict pipes to the apps they were created in. This requirements change if accepted would spawn a new ticket.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-03 12:58:32:

Moved unfinished 6.7.0 tickets to next release

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

No branches or pull requests

1 participant