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

Fix #337, fix memory corruption produced by misplaced memset() #338

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

stanislaw
Copy link
Contributor

@stanislaw stanislaw commented Jan 2, 2020

Describe the contribution

This PR fixes #337. The exact change has been recommended by @jphickey.

Testing performed

Currently, I cannot test this on Linux only on macOS. As reported in the issue, I confirm that that the issue gets fixed with this change applied. Also, the bug has been confirmed by @jphickey and the changeset is created from his recommendation.

Expected behavior changes

See related issue: #337.

System(s) tested on

Running osal-core-test tests (BSEM tests)
OS: macOS Mojave 10.14.6 (18G87)
Versions: master branch as of 155e9ebcd6d1930890231a44237e6883d229d22c.

Contributor Info - All information REQUIRED for consideration of pull request

Stanislav Pankevich, personal

The signed individual CLA has been sent to the email specified in the CLA document.

Calling memset too late after assigning the `sem` variable in the code of
`OS_BinSemCreate_Impl` caused the data pointed to by `sem` to get corrupted.

The issue was not caught by the existing test suite for POSIX OSAL when running
on Linux. However running the test suite on macOS revealed the anomalies
in the behavior of `pthread_cond_destroy()` which was working on the corrupted
memory as was demonstrated in:

Calling pthread_cond_destroy results in “Function not implemented” ENOSYS on macOS,
https://stackoverflow.com/questions/59560940/calling-pthread-cond-destroy-results-in-function-not-implemented-enosys-on-mac?noredirect=1#comment105301077_59560940

The original commit that introduced the issue is:

nasa@bfa7a33
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my Linux dev machine (Ubuntu 18.04 LTS 64 bit) and confirmed everything works as expected.

@skliper skliper added the CCB:PendingCLA External contribution pending CLA confirmation label Jan 22, 2020
@stanislaw
Copy link
Contributor Author

@skliper I have just signed the individual CLA and sent it to the email address specified in the document.

@skliper
Copy link
Contributor

skliper commented Jan 23, 2020

@stanislaw could you indicate if this is your personal contribution or as part of work for your company? I need to reference the appropriate CLA for our process. If your other pull requests are different (personal vs company or vice versa), could you indicate there also?

Thanks.

@stanislaw
Copy link
Contributor Author

stanislaw commented Jan 23, 2020

I have just edited the description of this PR. It is individual and I have signed the ICLA and sent it yesterday.

@astrogeco astrogeco added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Feb 11, 2020
@astrogeco astrogeco changed the title Fix #337: fix memory corruption produced by misplaced memset() Fix #337, fix memory corruption produced by misplaced memset() Feb 11, 2020
@astrogeco astrogeco added this to the 5.1.0 milestone Feb 12, 2020
@astrogeco
Copy link
Contributor

CCB 20200212 - Approved

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Feb 13, 2020
@stanislaw
Copy link
Contributor Author

Just a reminder: the individual CLA has been signed on this one. See here.

@skliper skliper removed the CCB:PendingCLA External contribution pending CLA confirmation label Feb 20, 2020
@astrogeco astrogeco changed the base branch from master to ic-20200226 February 25, 2020 22:29
@astrogeco astrogeco merged commit ae68a3f into nasa:ic-20200226 Feb 25, 2020
@astrogeco astrogeco mentioned this pull request Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible bug in os/posix/osapi.c when running osal-core-test / BSEM tests.
4 participants