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

TBL unreachable branch in CFE_TBL_GetWorkingBuffer and CFE_TBL_UpdateInternal (memcpy overlap avoidance) #1899

Closed
skliper opened this issue Aug 26, 2021 · 1 comment · Fixed by #1904 or #1885
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Aug 26, 2021

Is your feature request related to a problem? Please describe.
Can't hit ever hit the second false condition below since it only sets the buffer to inactive or a working buffer (never the same as active):

     789 [ +  + ][ +  - ]:         22 :             if ((*WorkingBufferPtr) != NULL &&
     790                 :         17 :                 (*WorkingBufferPtr)->BufferPtr != RegRecPtr->Buffers[RegRecPtr->ActiveBufferIndex].BufferPtr)
     791                 :            :             {
     792                 :            :                 /* In case the file contains a partial table load, get the active buffer contents first */
     793                 :         17 :                 memcpy((*WorkingBufferPtr)->BufferPtr, RegRecPtr->Buffers[RegRecPtr->ActiveBufferIndex].BufferPtr,
     794                 :            :                        RegRecPtr->Size);
     795                 :            :             }

if ((*WorkingBufferPtr) != NULL &&
(*WorkingBufferPtr)->BufferPtr != RegRecPtr->Buffers[RegRecPtr->ActiveBufferIndex].BufferPtr)
{
/* In case the file contains a partial table load, get the active buffer contents first */
memcpy((*WorkingBufferPtr)->BufferPtr, RegRecPtr->Buffers[RegRecPtr->ActiveBufferIndex].BufferPtr,
RegRecPtr->Size);
}

Same pattern in CFE_TBL_UpdateInternal

    1009         [ +  - ]:          6 :                 if (RegRecPtr->Buffers[0].BufferPtr != CFE_TBL_Global.LoadBuffs[RegRecPtr->LoadInProgress].BufferPtr)
    1010                 :            :                 {
    1011                 :          6 :                     memcpy(RegRecPtr->Buffers[0].BufferPtr,
    1012                 :          6 :                            CFE_TBL_Global.LoadBuffs[RegRecPtr->LoadInProgress].BufferPtr, RegRecPtr->Size);
    1013                 :            :                 }

/* To update a single buffered table requires a memcpy from working buffer */
if (RegRecPtr->Buffers[0].BufferPtr != CFE_TBL_Global.LoadBuffs[RegRecPtr->LoadInProgress].BufferPtr)
{
memcpy(RegRecPtr->Buffers[0].BufferPtr,
CFE_TBL_Global.LoadBuffs[RegRecPtr->LoadInProgress].BufferPtr, RegRecPtr->Size);
}

Describe the solution you'd like
Trade "defensive" programming (avoids memcpy overlap which is undefined behavior) w/ removing the impossible condition check since the only way to reach it would be to introduce a bug.

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper changed the title TBL unreachable branch in CFE_TBL_GetWorkingBuffer TBL unreachable branch in CFE_TBL_GetWorkingBuffer and CFE_TBL_UpdateInternal (memcpy overlap avoidance) Aug 26, 2021
@skliper skliper self-assigned this Aug 27, 2021
@skliper skliper added this to the 7.0.0 milestone Aug 27, 2021
@skliper
Copy link
Contributor Author

skliper commented Aug 27, 2021

Turns out it is possible to force these "bugs" from unit test by corrupting the global (basically just set the addresses the same).

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