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

Update grut699-vxworks6 cfe_psp_memory.c per white box unit testing results #37

Closed
skliper opened this issue Sep 25, 2019 · 9 comments
Closed
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@skliper
Copy link
Contributor

skliper commented Sep 25, 2019

During white box testing a number of functions were identified which return CFE_PSP_ERROR rather than CFE_PSP_INVALID_POINTER for NULL pointers. Functions include:
CFE_PSP_GetCDSSize
CFE_PSP_WriteToCDS
CFE_PSP_ReadFromCDS
CFE_PSP_GetResetArea
CFE_PSP_GetUserReservedArea
CFE_PSP_GetVolatileDiskMem
CFE_PSP_GetKernelTextSegmentInfo
CFE_PSP_GetCFETextSegmentInfo

Other issues:
Several functions take two pointer arguments, but only check if one is a NULL pointer.

Should check calling functions to see if changes to the return values may potentially cause issues.

Issues identified during #14 white box testing commit: [changeset:612f00f3]

@skliper skliper self-assigned this Sep 25, 2019
@skliper skliper added the bug Something isn't working label Sep 25, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Imported from trac issue 33. Created by dasp on 2015-08-11T17:18:16, last modified: 2019-05-06T11:43:03

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by sduran on 2015-08-18 10:44:19:

Changing this from _ERROR to _INVALID_POINTER is effectively an API change. Hold off making changes for the return code until an analysis of all of the calling functions is made. It could potentially result in a large number of changes.

However, the null pointer checks, to prevent potential crashes would be good to add.

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by abrown4 on 2015-08-18 12:57:04:

commit: [changeset:532afb2] Trac #37 Added null pointer checks to cfe_psp_memory.c (no API change).
Updated header comments and unit tests for current return values.

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by abrown4 on 2015-08-18 13:03:53:

In CFE_PSP_GetCFETextSegmentInfo(), line 421, if the moduleInfoGet() fails, the status is checked... but both paths have CFE_PSP_GetCFETextSegmentInfo() return CFE_PSP_SUCCESS. This is a problem. This function is called from cfe_es_task.c, where it does check the return status vs. OS_SUCCESS. The SizeOfCFESegment is assumed to be good with OS_SUCCESS.

commit: [changeset:b0fe8a4] Trac #37 Fixed inproper return type for CFE_PSP_GetCFETextSegmentInfo().

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by abrown4 on 2015-08-18 13:11:13:

In function CFE_PSP_GetKernelTextSegmentInfo(), white box unit testing, commit [changeset:612f00f3] through [changeset:b0fe8a4], shows that there is no sanity-checking the values returned by GetWrsKernelTextStart() and GetWrsKernelTextEnd().

Do we want to add this functionality or not to CFE_PSP_GetKernelTextSegmentInfo()?

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by sduran on 2015-08-18 15:15:01:

There is nothing to sanity check, unless you already know what the addresses should be, but you can't until you call these functions...

/*
** Function: GetWrsKernelTextStart
** Purpose: This function returns the start address of the kernel code.
**
*/
unsigned int GetWrsKernelTextStart (void)
{
return (unsigned int) &wrs_kernel_text_start;
}

/*
** Function: GetWrsKernelTextEnd
** Purpose: This function returns the end address of the kernel code.
**
*/
unsigned int GetWrsKernelTextEnd (void)
{
return (unsigned int) &wrs_kernel_text_end;
}

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by abrown4 on 2015-08-18 15:21:38:

Alan's WB test logic showed an implicit assumption that GetWrsKernelTextEnd() is expected to return a value greater than what you get from GetWrsKernelTextStart().

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by abrown4 on 2015-08-18 15:22:02:

(merged to #14 branch)

@skliper
Copy link
Contributor Author

skliper commented Sep 25, 2019

Trac comment by jhageman on 2019-05-06 11:43:03:

Specific to non-community PSP so closing as invalid. Note changes were merged to techdev-pc-linux-uei, techdev-sp0-vxworks6.9 branches (but not techdev-ut699 for some reason). Not in framework release.

@skliper skliper closed this as completed Sep 25, 2019
@skliper skliper removed their assignment Sep 26, 2019
@skliper skliper added the invalid This doesn't seem right label Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant