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

sched: remove DEBUGASSERT from nx_waitpid #6137

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

pkarashchenko
Copy link
Contributor

@pkarashchenko pkarashchenko commented Apr 22, 2022

Summary

stat_loc of wait and waitpid can be NULL according to POSIX (https://pubs.opengroup.org/onlinepubs/9699919799/functions/wait.html): In this case, if the value of the argument stat_loc is not a null pointer, information shall be stored in the location pointed to by stat_loc.

Remove DEBUGASSERT(stat_loc); from nx_waitpid

Impact

Minimal if this was not reported by anyone till now

Testing

Pass CI
Tested with SAME70-QMTECH board with CONFIG_SCHED_HAVE_PARENT=n

@pkarashchenko pkarashchenko marked this pull request as draft April 22, 2022 15:19
Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
@pkarashchenko pkarashchenko marked this pull request as ready for review April 22, 2022 15:25
@gustavonihei
Copy link
Contributor

gustavonihei commented Apr 22, 2022

@pkarashchenko Could you please link a reference to the POSIX chapter that states that stat_loc may be NULL?

@pkarashchenko
Copy link
Contributor Author

@pkarashchenko Could you please link a reference to the POSIX chapter that states that stat_loc may be NULL?

I've updated PR description

@gustavonihei
Copy link
Contributor

@pkarashchenko Could you please link a reference to the POSIX chapter that states that stat_loc may be NULL?

I've updated PR description

Thanks, I had skimmed that page, but I failed to find the information.
The excerpt you pasted does not state that stat_loc may be NULL. It just determines the behavior when stat_loc is non-NULL.

I tried looking at other implementations. In the Linux Kernel, if stat_loc (stat_addr in this case) is NULL, the kernel returns -EFAULT to the userspace:
https://github.com/torvalds/linux/blob/master/kernel/exit.c#L1683

@pkarashchenko
Copy link
Contributor Author

Yeah, but POSIX does not define an error code if start_loc is NULL, so Linux interpreted that to an error code.
For me it is natural that if start_loc is NULL then the caller "does not care" about exit status.

@gustavonihei
Copy link
Contributor

Yeah, but POSIX does not define an error code if start_loc is NULL, so Linux interpreted that to an error code. For me it is natural that if start_loc is NULL then the caller "does not care" about exit status.

I agree with your point.
In summary:

  • Prior to this PR, wait and waitpid had a premise of stat_loc being non-NULL. Passing a NULL pointer resulted in undefined behavior when DEBUG assertions are not enabled.
  • This PR defines a proper behavior for a NULL stat_loc.

I am okay with the proposed changes.

@pkarashchenko
Copy link
Contributor Author

@gustavonihei I think if POSIX want to define start_loc to be non-NULL, then the error code section would state that explicitly. And there would be no sentence that I pasted in the PR description.

@gustavonihei gustavonihei merged commit 1b4d8b3 into apache:master Apr 22, 2022
@pkarashchenko pkarashchenko deleted the bugfix/waitpid branch August 20, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants