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

acl_profiler_test: fix undefined behaviour #196

Merged
merged 3 commits into from
Nov 7, 2022
Merged

Conversation

pcolberg
Copy link
Contributor

@pcolberg pcolberg commented Nov 7, 2022

This resolves the final issue before enabling address sanitizer 🥳

putenv() should be replaced with acl_test_setenv() and acl_test_unsetenv() in a subsequent pull request, but for now this is sufficient to pass address sanitizer.

@pcolberg pcolberg added the bug Something isn't working label Nov 7, 2022
@pcolberg pcolberg added this to the 2023.1 milestone Nov 7, 2022
@pcolberg pcolberg self-assigned this Nov 7, 2022
acl_getenv() always returns NULL, hence the undefined behaviour was
never triggered. This looks like a debugging leftover, though assert()
would have been the safe alternative to dereferencing a NULL pointer.

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
_putenv_s() is recommended over _putenv() as a more secure variant,
though the exact benefits are not detailed. Unlike the unsafe putenv()
on Linux, _putenv() on Windows does appear to copy its argument. Use
_putenv_s() anyway since it provides the same interface as setenv().

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
The buffer passed to putenv() becomes part of the environment. It is
the caller's responsibility to keep the buffer alive as long as the
environment variable is not modified by a subsequent invocation.

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
@pcolberg pcolberg marked this pull request as ready for review November 7, 2022 21:09
@pcolberg pcolberg requested a review from zibaiwan November 7, 2022 21:09
Copy link
Contributor

@zibaiwan zibaiwan left a comment

Choose a reason for hiding this comment

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

Thanks @pcolberg !

@pcolberg pcolberg merged commit 900e86e into intel:main Nov 7, 2022
@pcolberg pcolberg deleted the putenv branch November 7, 2022 21:58
pcolberg added a commit to pcolberg/fpga-runtime-for-opencl that referenced this pull request Nov 8, 2022
Ensure each setenv() is matched with unsetenv().

This amends intel#196
pcolberg added a commit to pcolberg/fpga-runtime-for-opencl that referenced this pull request Nov 8, 2022
Ensure each setenv() is matched with unsetenv().

This amends intel#196

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
pcolberg added a commit that referenced this pull request Nov 8, 2022
Ensure each setenv() is matched with unsetenv().

This amends #196

Signed-off-by: Peter Colberg <peter.colberg@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AddressSanitizer: stack-buffer-overflow in test (track_object, some_leak)
2 participants