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

CFE_ES_SEND_MEM_POOL_STATS_CC Issues #651

Closed
dmknutsen opened this issue Apr 29, 2020 · 4 comments · Fixed by #917 or #936
Closed

CFE_ES_SEND_MEM_POOL_STATS_CC Issues #651

dmknutsen opened this issue Apr 29, 2020 · 4 comments · Fixed by #917 or #936
Assignees
@jphickey
Labels
bug
Milestone
Caelum

Comments

@dmknutsen
Copy link
Contributor

Describe the bug
There are multiple issues with the CFE_ES_SEND_MEM_POOL_STATS_CC command:

  1. The command/function(s) used are fundamentally flawed in that a user can send a seemingly benign command that can result in a segmentation fault if one of the command parameters is incorrect. For example, if the PoolHandle parameter is set to zero a segmentation fault will result (pretty much any value below xFFFFFE28 faults on my machine). This occurs when handle is validated via the CFE_ES_ValidateHandle function. Should consider modifying the CFE_ES_ValidateHandle function + updating the unit test to test command on boundary/extreme conditions.
  2. On a 64-bit machine if the PoolHandle parameter is set to a valid value the function will fail. This is because CFE_PSP_MemValidateRange – which is called via the CFE_ES_ValidateHandle function as part of the validation process, limits the max memory range of the handle to xFFFFFFFE.
  3. On a 64-bit machine - compiler added padding will be applied to the command. The order of variable declaration should ideally be descending in size to avoid future conflicts.

Expected behavior
Command works nominally and is vetted via combination of unit/functional tests.

System observed on:
Oracle VM VirtualBox
OS: ubuntu-19.10
Versions: cFE 6.7.13.0, OSAL 5.0.12.0, PSP 1.4.9.0

Reporter Info
Dan Knutsen
NASA/Goddard
@dmknutsen dmknutsen added the bug label Apr 29, 2020
@skliper skliper added this to the 7.0.0 milestone May 7, 2020
@skliper
Copy link
Contributor

skliper commented Aug 5, 2020

@jphickey want to take this one?

@jphickey jphickey self-assigned this Aug 5, 2020
@jphickey
Copy link
Contributor

jphickey commented Aug 5, 2020

Sure, sounds good to me. My proposal (if others agree) would be to apply the same paradigm I'm using for CFE_ES_AppID -- define a type (NOT an address) which abstractly identifies a memory pool, e.g. CFE_ES_PoolID_t ... then this gets put into CMD/TLM packets to identify the memory pool. The CFE_ES then looks up this value to get the REAL memory address of the pool.

@jphickey
Copy link
Contributor

jphickey commented Aug 5, 2020

Actually, since we already have a CFE_ES_MemHandle_t type -- then no new type is needed -- can just use this, but make it an abstract ID rather than a memory address.

@skliper
Copy link
Contributor

skliper commented Aug 6, 2020

Perfect. Hopefully we don't have any other memory addresses in commands...

@jphickey jphickey mentioned this issue Aug 10, 2020
Closed
This was referenced Aug 18, 2020
Closed
Closed
jphickey added a commit to jphickey/cFE that referenced this issue Sep 29, 2020
@jphickey
Fix nasa#651, Use resource ID for memory pools
69e129e 
Instead of identifying a memory pool by its memory address,
use a resource ID.  IDs are a constant size, regardless of
whether the host machine is 32 or 64 bits.

- IDs can be put into commands/telemetry and maintain a more
  consistent format with consistent alignment requirements.
- IDs can be independently verified without dereferencing
  memory.  Previously the only way to validate a memory pool
  was to read the address pointed to, which results in a SEGV
  if the address was bad.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 29, 2020
@jphickey
Fix nasa#651, Use resource ID for memory pools
dc51842 
Instead of identifying a memory pool by its memory address,
use a resource ID.  IDs are a constant size, regardless of
whether the host machine is 32 or 64 bits.

- IDs can be put into commands/telemetry and maintain a more
  consistent format with consistent alignment requirements.
- IDs can be independently verified without dereferencing
  memory.  Previously the only way to validate a memory pool
  was to read the address pointed to, which results in a SEGV
  if the address was bad.
@jphickey jphickey mentioned this issue Sep 29, 2020
Merged
jphickey added a commit to jphickey/cFE that referenced this issue Sep 29, 2020
@jphickey
Fix nasa#651, Use resource ID for memory pools
f6ae2fb 
Instead of identifying a memory pool by its memory address,
use a resource ID.  IDs are a constant size, regardless of
whether the host machine is 32 or 64 bits.

- IDs can be put into commands/telemetry and maintain a more
  consistent format with consistent alignment requirements.
- IDs can be independently verified without dereferencing
  memory.  Previously the only way to validate a memory pool
  was to read the address pointed to, which results in a SEGV
  if the address was bad.
astrogeco added a commit that referenced this issue Oct 1, 2020
@astrogeco
Merge pull request #917 from jphickey/fix-651-abstract-mempool-id
3a809eb 
Fix #651, Use resource ID for memory pools
@astrogeco astrogeco mentioned this issue Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jphickey jphickey

Labels
bug
Projects
None yet
Milestone
Caelum
3 participants
@jphickey @skliper @dmknutsen