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

errno not always set correctly on failure #461

Closed
ronorton opened this issue Feb 14, 2022 · 6 comments
Closed

errno not always set correctly on failure #461

ronorton opened this issue Feb 14, 2022 · 6 comments

Comments

@ronorton
Copy link
Contributor

The malloc test contains some calls that are expected to fail such as attempts to allocate very large chunks of memory. Currently these return nullptr but do not set errno to ENOMEM as required by the SUSv2 standard (at least according to the Linux manpage for malloc). Unfortunately check_result notices the incorrect errno but does not fail the test due to the return here so the test emits an error message but does not fail, for example:

realloc(0x40e60000(16384), 9223372036854775807)
Expected error: 12 but got 0
(test continues)

Note that there is a suspicious check for realloc(-1) in override here but I'm sure if this is related to the failure.

@ronorton
Copy link
Contributor Author

I suppose this is tangentially related to #423 .

@mjp41
Copy link
Member

mjp41 commented Feb 20, 2022

So I am thinking of adding

errno = ENOMEM;

on line 321:

static void* reserve(size_t size) noexcept
{
// If enforcing access, map pages initially as None, and then
// add permissions as required. Otherwise, immediately give all
// access as this is the most efficient to implement.
auto prot = PalEnforceAccess ? PROT_NONE : PROT_READ | PROT_WRITE;
void* p = mmap(
nullptr,
size,
prot,
MAP_PRIVATE | MAP_ANONYMOUS | DefaultMMAPFlags<OS>::flags,
AnonFD<OS>::fd,
0);
if (p != MAP_FAILED)
{
#ifdef SNMALLOC_TRACING
std::cout << "Pal_posix reserved: " << p << " (" << size << ")"
<< std::endl;
#endif
return p;
}
return nullptr;
}

We should then review other reserve operations for specialised Pals such as

static void* reserve_aligned(size_t size) noexcept
{
// Alignment must be a power of 2.
SNMALLOC_ASSERT(bits::is_pow2(size));
SNMALLOC_ASSERT(size >= minimum_alloc_size);
int log2align = static_cast<int>(bits::next_pow2_bits(size));
auto prot =
state_using || !PalEnforceAccess ? PROT_READ | PROT_WRITE : PROT_NONE;
void* p = mmap(
nullptr,
size,
prot,
MAP_PRIVATE | MAP_ANONYMOUS | MAP_ALIGNED(log2align),
-1,
0);
if (p == MAP_FAILED)
return nullptr;
return p;
}

and add the same to the MAP_FAILED case.

@mjp41
Copy link
Member

mjp41 commented Feb 20, 2022

We probably need to do something here too

if (slab_sizeclass >= NUM_SLAB_SIZES)
{
// Your address space is not big enough for this allocation!
return {nullptr, nullptr};
}

@rmn30
Copy link
Contributor

rmn30 commented Feb 21, 2022

mmap should set errno on failure so setting it in pal_posix.h should not be required. Maybe the test is hitting the chunk allocator case?

@rmn30
Copy link
Contributor

rmn30 commented Feb 21, 2022

I can confirm that setting errno in chunkallocator.h fixes the test failure, but is that portable to non-posix PALs? Do we need a new PAL method?

rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
Previously this test would emit an error message but not
abort if the `errno` was not as expected on failure. This
was because the `return` in the `null == true` case prevented the
check for `failed == true` at the end of `check_result` from
being reached. To resolve this just abort immediately instead
of instead of setting `failed` to true, as in the null case.

See microsoft#461.
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
…space.

This resolves a test failure in the malloc functional test.

See microsoft#461.
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
Previously this test would emit an error message but not
abort if the errno was not as expected on failure. This
was because the return in the null == true case prevented the
check for failed == true at the end of check_result from
being reached. To resolve this just abort immediately instead
of instead of setting failed to true, as in the null case.

See microsoft#461.
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
…ace.

This resolves a test failure in the malloc functional test.

See microsoft#461.
@rmn30
Copy link
Contributor

rmn30 commented Feb 21, 2022

I think errno is specified by C / C++ standard so probably safe to just set it?

@rmn30 rmn30 mentioned this issue Feb 21, 2022
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
…ace.

This resolves a test failure in the malloc functional test.

See microsoft#461.
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 21, 2022
…ace.

This resolves a test failure in the malloc functional test.

See microsoft#461.
rmn30 pushed a commit to rmn30/snmalloc that referenced this issue Feb 22, 2022
This was not being done for certain cases as per microsoft#461.
Note that malloc and calloc are probably still not setting errno for
those cases but this is not currently tested...
rmn30 added a commit that referenced this issue Feb 24, 2022
Correctly set errno on failure and improve the related test.

Previously the malloc test would emit an error message but not
abort if the errno was not as expected on failure. This
was because the return in the null == true case prevented the
check for failed == true at the end of check_result from
being reached. To resolve this just abort immediately as in the 
null case.

Also add tests of allocations that are expected to fail for
calloc and malloc.

To make the tests pass we need to set errno in several places,
making sure to keep this off the fast path.

We must also take care not to attempt to zero nullptr in case
of calloc failure.

See #461 and #463.
@rmn30 rmn30 closed this as completed Feb 24, 2022
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

No branches or pull requests

3 participants