Enforce aligned allocation on FreeBSD #36

illwieckz · 2023-02-11T23:42:46Z

This fixes those errors experienced on FreeBSD:

crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr"
crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr"

The usual malloc should already be aligned, but for some unknown reason it doesn't work on FreeBSD.

Quote from man malloc on FreeBSD:

Standard API
The malloc() function allocates size bytes of uninitialized memory. The
allocated space is suitably aligned (after possible pointer coercion)
for storage of any type of object.

This patch should not be needed.

Original message:

It's not bad to make sure every system does aligned allocation if we can, so let's use aligned_alloc() on Posix systems, which is meant to be freed with the same free() as the one used to freed memory allocated by malloc():

https://man7.org/linux/man-pages/man3/aligned_alloc.3.html

On macOS, malloc() is always aligned:

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/malloc.3.html

Microsoft did not implemented aligned_alloc(), but implemented _aligned_malloc(), but _aligned_malloc requires allocated memory to be freed with _aligned_free(), which would break compatibility with software freeing with free():

Using _aligned_malloc() instead of malloc() on Windows produced a segmentation fault, so malloc() is kept as is.

This fixes those errors experienced on FreeBSD: crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr" crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr" The usual malloc should already be aligned, but for some unknown reason it doesn't work on FreeBSD. Quote from `man malloc` on FreeBSD: > Standard API > The malloc() function allocates size bytes of uninitialized memory. The > allocated space is suitably aligned (after possible pointer coercion) > for storage of any type of object. This patch should not be needed. See #36

slipher · 2023-02-13T14:12:05Z

This is not needed. malloc guarantees alignment good enough for any of the standard C data types. CRNLIB_MIN_ALLOC_ALIGMENT is only 8 so plain malloc sufficies on any remotely supported system (e.g. one that has uint64_t).

illwieckz · 2023-02-13T14:14:16Z

But the tool doesn't run on some systems like FreeBSD without that… The patch does not look that intrusive.

illwieckz · 2023-02-13T14:17:35Z

@slipher is there any performance impact at calling aligned_alloc instead of malloc?

slipher · 2023-02-13T14:22:13Z

The FreeBSD malloc man page gives the same alignment guarantees as everyone else. There must be a memory corruption bug somewhere else.

illwieckz · 2023-02-14T13:17:11Z

If I disable the tests for “bad ptr”, it works…

illwieckz · 2023-02-21T14:26:56Z

@slipher what would be the drawback of using aligned_alloc instead of malloc, in general?

Because I can do an ifdef for FreeBSD, but how much would it be “less good” for Linux to do it as well?

slipher · 2023-02-21T17:34:00Z

Adding complexity for no benefit.

If you really think that FreeBSD malloc returns incorrect results, you can test that by putting the alignment assertion at the point of allocation rather than (or in addition to) where it is freed.

illwieckz · 2023-02-22T01:13:31Z

diff --git a/crnlib/crn_mem.cpp b/crnlib/crn_mem.cpp
index 4040e4c..349f047 100644
--- a/crnlib/crn_mem.cpp
+++ b/crnlib/crn_mem.cpp
@@ -70,6 +70,10 @@ static void* crnlib_default_realloc(void* p, size_t size, size_t* pActual_size,
 
   if (!p) {
     p_new = ::malloc(size);
+    if ((ptr_bits_t)p_new & (CRNLIB_MIN_ALLOC_ALIGNMENT - 1)) {
+      crnlib_mem_error("crnlib_default_realloc: bad ptr");
+      return NULL;
+    }
     CRNLIB_ASSERT((reinterpret_cast<ptr_bits_t>(p_new) & (CRNLIB_MIN_ALLOC_ALIGNMENT - 1)) == 0);
 
     if (!p_new) {
@@ -160,11 +164,6 @@ void* crnlib_malloc(size_t size, size_t* pActual_size) {
 }
 
 void* crnlib_realloc(void* p, size_t size, size_t* pActual_size, bool movable) {
-  if ((ptr_bits_t)p & (CRNLIB_MIN_ALLOC_ALIGNMENT - 1)) {
-    crnlib_mem_error("crnlib_realloc: bad ptr");
-    return NULL;
-  }
-
   if (size > CRNLIB_MAX_POSSIBLE_BLOCK_SIZE) {
     crnlib_mem_error("crnlib_malloc: size too big");
     return NULL;

Before:

$ ./crunch -file door01a_d.png -out door01a_d.crn
crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr"
crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr"

After:

$ ./crunch -file door01a_d.png -out door01a_d.crn
crunch/crnlib/crn_mem.cpp(129): Assertion failed: "crnlib_default_realloc: bad ptr"
crunch/crnlib/crn_mem.cpp(129): Assertion failed: "crnlib_default_realloc: bad ptr
crunch/crnlib/crn_mem.cpp(129): Assertion failed: "crnlib_malloc: out of memory"
crunch/crnlib/crn_mem.cpp(129): Assertion failed: "crnlib_malloc: out of memory"
Segmentation fault (core dumped)

On FreeBSD the alignment is already considered wrong right after the malloc call.

I don't know if that's a FreeBSD bug or if the alignment check itself is wrong.

slipher · 2023-02-22T02:52:39Z

Seems like there is some heap corruption. Try it with address sanitizer or valgrind?

illwieckz · 2023-02-22T14:58:56Z

When building with current master,

Not running with valgrind:

$ ./crunch -file door01a_d.png -out door01a_d.crn
crunch: Advanced DXTn Texture Compressor (Unity format variant)

Brought to you by:
- 2014-2022 Daemon Developers and contributors
  https://github.com/DaemonEngine/crunch
- 2017-2018 Alexander Suvorov and Unity Software Inc.
  https://github.com/Unity-Technologies/crunch/tree/unity
- 2010-2017 Richard Geldreich, Jr. and Binomial LLC and contributors
  https://github.com/BinomialLLC/crunch

crnlib version v1.04U 64-bit Built from git-eca5c36

Reading source texture: "door01a_d.png"
Texture successfully loaded in 0.008s
Source texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Apparent type: 2D map, Flags: R G B Non-Flipped 
Generating mipmaps using filter "kaiser"
crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr"

crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr"
Generated 9 mipmap levels in 0.041s
Writing DXT1 texture to file: "door01a_d.crn"
Compressing using quality level 128
crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr"

crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr"
crunch/crnlib/crn_vector.cpp(42): Failure: "buf"

crunch/crnlib/crn_vector.cpp(42): Failure: "buf"

Running with valgrind:

$ valgrind ./crunch -file door01a_d.png -out door01a_d.crn
==5270== Memcheck, a memory error detector
==5270== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==5270== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==5270== Command: ./crunch -file door01a_d.png -out door01a_d.crn
==5270== 
crunch: Advanced DXTn Texture Compressor (Unity format variant)

Brought to you by:
- 2014-2022 Daemon Developers and contributors
  https://github.com/DaemonEngine/crunch
- 2017-2018 Alexander Suvorov and Unity Software Inc.
  https://github.com/Unity-Technologies/crunch/tree/unity
- 2010-2017 Richard Geldreich, Jr. and Binomial LLC and contributors
  https://github.com/BinomialLLC/crunch

crnlib version v1.04U 64-bit Built from git-eca5c36

Reading source texture: "door01a_d.png"
Texture successfully loaded in 0.079s
Source texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Apparent type: 2D map, Flags: R G B Non-Flipped 
Generating mipmaps using filter "kaiser"
Generated 9 mipmap levels in 0.966s
Writing DXT1 texture to file: "door01a_d.crn"
Compressing using quality level 128
Processing: 100%              
Texture successfully written in 10.041s
Texture successfully processed in 11.019s
Input texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Input pixels: 131072, Input file size: 219476, Input bits/pixel: 13.396
Output texture: 256x512, Levels: 10, Faces: 1, Format: DXT1
Output pixels: 174763, Output file size: 32467, Output bits/pixel: 1.486

Total time: 11.133s
1 total file(s) successfully processed, 0 file(s) skipped, 0 file(s) failed.

Exit status: 0
==5270== 
==5270== HEAP SUMMARY:
==5270==     in use at exit: 23,552 bytes in 56 blocks
==5270==   total heap usage: 62,926 allocs, 62,870 frees, 134,669,138 bytes allocated
==5270== 
==5270== LEAK SUMMARY:
==5270==    definitely lost: 320 bytes in 20 blocks
==5270==    indirectly lost: 0 bytes in 0 blocks
==5270==      possibly lost: 0 bytes in 0 blocks
==5270==    still reachable: 19,136 bytes in 35 blocks
==5270==         suppressed: 4,096 bytes in 1 blocks
==5270== Rerun with --leak-check=full to see details of leaked memory
==5270== 
==5270== For lists of detected and suppressed errors, rerun with: -s
==5270== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Valgrind simply hides the bug.

illwieckz · 2023-02-22T15:05:23Z

I rebuilt with -fsanitize=address and the bug disappears as well:

$ cmake .. \
	-D'CMAKE_C_FLAGS'='-fsanitize=address' \
	-D'CMAKE_CXX_FLAGS'='-fsanitize=address' \
	-D'CMAKE_EXE_LINKER_FLAGS'='-fsanitize=address'
$ make -j$(sysctl -n hw.ncpu)

$ ./crunch -file door01a_d.png -out door01a_d.crn
crunch: Advanced DXTn Texture Compressor (Unity format variant)

Brought to you by:
- 2014-2022 Daemon Developers and contributors
  https://github.com/DaemonEngine/crunch
- 2017-2018 Alexander Suvorov and Unity Software Inc.
  https://github.com/Unity-Technologies/crunch/tree/unity
- 2010-2017 Richard Geldreich, Jr. and Binomial LLC and contributors
  https://github.com/BinomialLLC/crunch

crnlib version v1.04U 64-bit Built from git-e0b04c2

Reading source texture: "door01a_d.png"
Texture successfully loaded in 0.007s
Source texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Apparent type: 2D map, Flags: R G B Non-Flipped 
Generating mipmaps using filter "kaiser"
Generated 9 mipmap levels in 0.074s
Writing DXT1 texture to file: "door01a_d.crn"
Compressing using quality level 128
Processing: 100%              
Texture successfully written in 0.264s
Texture successfully processed in 0.338s
Input texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Input pixels: 131072, Input file size: 219476, Input bits/pixel: 13.396
Output texture: 256x512, Levels: 10, Faces: 1, Format: DXT1
Output pixels: 174763, Output file size: 32467, Output bits/pixel: 1.486

Total time: 0.346s
1 total file(s) successfully processed, 0 file(s) skipped, 0 file(s) failed.

Exit status: 0

This fixes those errors experienced on FreeBSD: crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr" crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr" The usual malloc should already be aligned, but for some unknown reason it doesn't work on FreeBSD. See #36

This fixes those errors experienced on FreeBSD: crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_free: bad ptr" crunch/crnlib/crn_mem.cpp(125): Assertion failed: "crnlib_realloc: bad ptr" The usual malloc should already be aligned, but for some unknown reason it doesn't work on FreeBSD. Quote from `man malloc` on FreeBSD: > Standard API > The malloc() function allocates size bytes of uninitialized memory. The > allocated space is suitably aligned (after possible pointer coercion) > for storage of any type of object. This patch should not be needed. See #36

illwieckz · 2023-03-11T00:59:36Z

@slipher do you have other ideas to track possible heap corruptions?

illwieckz · 2023-04-11T14:24:02Z

The workaround looks good enough to me, and is only used on the affected system, let's merge it.

illwieckz added the bug Something isn't working label Feb 11, 2023

illwieckz force-pushed the illwieckz/aligned-allocation branch from 9ad2766 to 75bbda6 Compare February 11, 2023 23:43

illwieckz mentioned this pull request Feb 11, 2023

Improve build on FreeBSD #11

Merged

illwieckz changed the title ~~Enforce aligned allocation when possible~~ enforce aligned allocation when possible Feb 11, 2023

illwieckz changed the title ~~enforce aligned allocation when possible~~ Enforce aligned allocation when possible Feb 11, 2023

illwieckz force-pushed the illwieckz/aligned-allocation branch from 75bbda6 to 9e01210 Compare February 13, 2023 10:38

illwieckz force-pushed the illwieckz/aligned-allocation branch from 9e01210 to ccc3768 Compare February 22, 2023 15:45

illwieckz changed the title ~~Enforce aligned allocation when possible~~ Enforce aligned allocation on FreeBSD Feb 22, 2023

illwieckz force-pushed the illwieckz/aligned-allocation branch 2 times, most recently from d42d81d to c504e8e Compare February 22, 2023 15:55

illwieckz merged commit ae59060 into master Apr 11, 2023

illwieckz deleted the illwieckz/aligned-allocation branch April 11, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enforce aligned allocation on FreeBSD #36

Enforce aligned allocation on FreeBSD #36

Uh oh!

illwieckz commented Feb 11, 2023 •

edited

Loading

Uh oh!

slipher commented Feb 13, 2023

Uh oh!

illwieckz commented Feb 13, 2023

Uh oh!

illwieckz commented Feb 13, 2023

Uh oh!

slipher commented Feb 13, 2023 •

edited

Loading

Uh oh!

illwieckz commented Feb 14, 2023

Uh oh!

illwieckz commented Feb 21, 2023 •

edited

Loading

Uh oh!

slipher commented Feb 21, 2023

Uh oh!

illwieckz commented Feb 22, 2023

Uh oh!

slipher commented Feb 22, 2023

Uh oh!

illwieckz commented Feb 22, 2023 •

edited

Loading

Uh oh!

illwieckz commented Feb 22, 2023 •

edited

Loading

Uh oh!

illwieckz commented Mar 11, 2023

Uh oh!

illwieckz commented Apr 11, 2023

Uh oh!

Uh oh!

Enforce aligned allocation on FreeBSD #36

Enforce aligned allocation on FreeBSD #36

Uh oh!

Conversation

illwieckz commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slipher commented Feb 13, 2023

Uh oh!

illwieckz commented Feb 13, 2023

Uh oh!

illwieckz commented Feb 13, 2023

Uh oh!

slipher commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Feb 14, 2023

Uh oh!

illwieckz commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slipher commented Feb 21, 2023

Uh oh!

illwieckz commented Feb 22, 2023

Uh oh!

slipher commented Feb 22, 2023

Uh oh!

illwieckz commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illwieckz commented Mar 11, 2023

Uh oh!

illwieckz commented Apr 11, 2023

Uh oh!

Uh oh!

illwieckz commented Feb 11, 2023 •

edited

Loading

slipher commented Feb 13, 2023 •

edited

Loading

illwieckz commented Feb 21, 2023 •

edited

Loading

illwieckz commented Feb 22, 2023 •

edited

Loading

illwieckz commented Feb 22, 2023 •

edited

Loading