Manual poisoning for pool/heap when using address sanitizer #4584

dipinhora · 2025-01-14T23:41:56Z

So the address sanitizer can better detect issues

Including adding an address and undefined behavior sanitizer CI build and a couple of minor fixes identified by the address sanitizer

SeanTAllen · 2025-01-15T01:23:38Z

src/libponyrt/gc/cycle.c

@@ -1322,6 +1322,7 @@ void ponyint_cycle_terminate(pony_ctx_t* ctx)
  ponyint_become(ctx, cycle_detector);
  final(ctx, cycle_detector);
  ponyint_destroy(ctx, cycle_detector);
+  ponyint_become(ctx, NULL);


What's the change. I'm on my phone so maybe I missed it, but I don't see it used.

ponyint_become saves the cycle_detector pointer into the ctx on line 1322.. then final runs the finaliser/cleanup for the cycle detector.. then ponyint_destroy destroys the cycle_detector.. the new ponyint_become(ctx, NULL) is to ensure that the ctx don't retain a reference to the now invalid cycle_detector..

dipinhora · 2025-01-15T06:35:23Z

i believe the missing header file:

/__w/ponyc/ponyc/src/libponyrt/mem/heap.c:12:10: fatal error: 'sanitizer/asan_interface.h' file not found
   12 | #include <sanitizer/asan_interface.h>

error can be resolved by installing the libclang-rt-dev package in the docker image ghcr.io/ponylang/ponyc-ci-x86-64-unknown-linux-ubuntu24.04-builder:20240425..

.github/workflows/pr.yml

SeanTAllen · 2025-01-15T16:57:44Z

i believe the missing header file:
/__w/ponyc/ponyc/src/libponyrt/mem/heap.c:12:10: fatal error: 'sanitizer/asan_interface.h' file not found
   12 | #include <sanitizer/asan_interface.h>
error can be resolved by installing the libclang-rt-dev package in the docker image ghcr.io/ponylang/ponyc-ci-x86-64-unknown-linux-ubuntu24.04-builder:20240425..

There's a different error now with that in a new builder that I ran with. Shall I push that change and you can continue working on getting this to pass CI?

dipinhora · 2025-01-15T17:04:24Z

i believe the missing header file:
/__w/ponyc/ponyc/src/libponyrt/mem/heap.c:12:10: fatal error: 'sanitizer/asan_interface.h' file not found
   12 | #include <sanitizer/asan_interface.h>
error can be resolved by installing the libclang-rt-dev package in the docker image ghcr.io/ponylang/ponyc-ci-x86-64-unknown-linux-ubuntu24.04-builder:20240425..
There's a different error now with that in a new builder that I ran with. Shall I push that change and you can continue working on getting this to pass CI?

@SeanTAllen one more docker image change would be useful.. currently, ubsan can't find a symbolizer and the backtraces aren't very useful:

2025-01-15T16:48:07.3281792Z ==16475==WARNING: invalid path to external symbolizer!
2025-01-15T16:48:07.3282410Z ==16475==WARNING: Failed to use and restart external symbolizer!

i believe the llvm package includes the llvm-symbolizer for ubsan to use so it can print more useful backtraces..

as for the failure, i have a feeling that the ASAN_OPTIONS environment variable isn't being properly set.. that shouldn't hold up the docker image changes (with or without the symbolizer)..

SeanTAllen · 2025-01-15T17:17:23Z

@dipinhora i wary of installing llvm in the image and then accidentally picking up the wrong one. if we build it as part of our llvm build, that should be able to work, yes?

dipinhora · 2025-01-15T17:20:34Z

@dipinhora i wary of installing llvm in the image and then accidentally picking up the wrong one. if we build it as part of our llvm build, that should be able to work, yes?

i would assume so.. not sure how ubsan finds a symbolizer to use but i'm guessing it looks for it via the path.. maybe it's documented somewhere... 8*/

SeanTAllen · 2025-01-15T17:21:55Z

@dipinhora i wary of installing llvm in the image and then accidentally picking up the wrong one. if we build it as part of our llvm build, that should be able to work, yes?

i would assume so.. not sure how ubsan finds a symbolizer to use but i'm guessing it looks for it via the path.. maybe it's documented somewhere... 8*/

can you see if you can run that down? i think it is probably a better, less foot-gunny approach.

dipinhora · 2025-01-15T17:23:34Z

@dipinhora i wary of installing llvm in the image and then accidentally picking up the wrong one. if we build it as part of our llvm build, that should be able to work, yes?

i would assume so.. not sure how ubsan finds a symbolizer to use but i'm guessing it looks for it via the path.. maybe it's documented somewhere... 8*/

can you see if you can run that down? i think it is probably a better, less foot-gunny approach.

yep.. the ubsan docs (https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#stack-traces-and-report-symbolization) say:

Make sure llvm-symbolizer binary is in PATH.

SeanTAllen · 2025-01-15T17:58:10Z

It appears we don't build llvm-sanitizer at the moment. If you need help figuring that part out, @chalcolith would be a good resource.

dipinhora · 2025-01-15T18:11:33Z

It appears we don't build llvm-sanitizer at the moment. If you need help figuring that part out, @chalcolith would be a good resource.

we do build llvm-symbolizer already:

2025-01-15T16:46:14.9006145Z -- Installing: /__w/ponyc/ponyc/build/libs/bin/llvm-symbolizer

SeanTAllen · 2025-01-15T18:12:55Z

I misread your comment. In particular, the binary name.

SeanTAllen · 2025-01-16T00:39:14Z

@dipinhora when you have a chance, you should rebase against main which now has the updated ubuntu24 builders across the board.

dipinhora · 2025-01-16T01:53:47Z

@dipinhora when you have a chance, you should rebase against main which now has the updated ubuntu24 builders across the board.

done

SeanTAllen · 2025-01-21T19:34:10Z

@dipinhora is this good now? I believe everything we discussed is in place now. I wanted to verify.

jemc · 2025-01-21T19:37:20Z

src/libponyrt/mem/pool.h

@@ -43,6 +49,28 @@ size_t ponyint_pool_used_size(size_t index);

 size_t ponyint_pool_adjust_size(size_t size);

+#ifndef POOL_USE_DEFAULT


I'd like to see the order reversed here.

Can you make this say #ifdef POOL_USE_DEFAULT and make the default case come first?

SeanTAllen · 2025-01-21T19:41:45Z

src/libponyc/codegen/genfun.c

+    // must be at least the size of 3 or else we get a heap buffer overflow
+    if((2 * sizeof(LLVMTypeRef)) == tparam_size)
+      tparam_size += tparam_size + sizeof(LLVMTypeRef);
+  }


what's going on here. Joe and I are trying to sort it out and we are very confused at the moment.

is this, "if tparam_size was 0 so that it is now 2, we want to make it 3"?

i assume this was found by the sanitizer?

either way, i find the logic confusing. is there a reason it feels so "obfuscated"?

going to ignore you @SeanTAllen because @jemc rewrote the code already..

src/libponyc/codegen/genfun.c

dipinhora · 2025-01-21T20:55:22Z

@dipinhora is this good now? I believe everything we discussed is in place now. I wanted to verify.

kind of.. the CI is no longer failing.. but ubsan is still having trouble with finding the symbolizer (https://github.com/ponylang/ponyc/actions/runs/12802214635/job/35693500329#step:11:451):

==1344==WARNING: invalid path to external symbolizer!
==1344==WARNING: Failed to use and restart external symbolizer!

even though i explicitly checked and it should be in the path correctly (https://github.com/ponylang/ponyc/actions/runs/12802214635/job/35693500329#step:11:70):

OVERVIEW: llvm-symbolizer

USAGE: llvm-symbolizer [options] addresses...

i'm planning on giving it another try to get it working but if it doesn't i'm going to give up on the symbolization from the sanitizers for now..

So the address sanitizer can better detect issues Including adding an address and undefined behavior sanitizer CI build and a couple of minor fixes identified by the address sanitizer

Co-authored-by: Joe Eli McIlvain <[email protected]>

dipinhora · 2025-01-22T02:26:56Z

@dipinhora is this good now? I believe everything we discussed is in place now. I wanted to verify.

@SeanTAllen this is now ready from my perspective..

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 14, 2025

SeanTAllen reviewed Jan 15, 2025

View reviewed changes

.github/workflows/pr.yml Outdated Show resolved Hide resolved

dipinhora force-pushed the asan_poisoning branch from f1b429c to f9db5d4 Compare January 16, 2025 01:53

jemc reviewed Jan 21, 2025

View reviewed changes

SeanTAllen reviewed Jan 21, 2025

View reviewed changes

jemc reviewed Jan 21, 2025

View reviewed changes

src/libponyc/codegen/genfun.c Outdated Show resolved Hide resolved

dipinhora and others added 9 commits January 21, 2025 18:39

Manual poisoning for pool/heap when using address sanitizer

db727f6

So the address sanitizer can better detect issues Including adding an address and undefined behavior sanitizer CI build and a couple of minor fixes identified by the address sanitizer

Update .github/workflows/pr.yml

a265ab4

fix CI failure..

a7cd1e1

fix CI failure.. again..

2ab7170

fix CI failure.. again.. again...

6aefd11

fix CI failure.. again.. again... again.

5de6767

fix CI failure.. again.. again... again. and again..

22053eb

make dipin's crappy code less confusing

c50c42d

Co-authored-by: Joe Eli McIlvain <[email protected]>

remove extra )

499decd

dipinhora added 3 commits January 21, 2025 18:39

pool default switcharoo

441e7d8

symbolization "fun"

581328a

don't set path

75b17ca

dipinhora force-pushed the asan_poisoning branch from 4cb5799 to 75b17ca Compare January 22, 2025 00:20

SeanTAllen merged commit f8a2fee into ponylang:main Jan 22, 2025
22 checks passed

ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manual poisoning for pool/heap when using address sanitizer #4584

Manual poisoning for pool/heap when using address sanitizer #4584

dipinhora commented Jan 14, 2025

SeanTAllen Jan 15, 2025

dipinhora Jan 15, 2025

SeanTAllen Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025 •

edited

Loading

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

SeanTAllen commented Jan 16, 2025

dipinhora commented Jan 16, 2025

SeanTAllen commented Jan 21, 2025

jemc Jan 21, 2025

dipinhora Jan 21, 2025

SeanTAllen Jan 21, 2025

SeanTAllen Jan 21, 2025

SeanTAllen Jan 21, 2025

dipinhora Jan 21, 2025

dipinhora commented Jan 21, 2025

dipinhora commented Jan 22, 2025

		@@ -43,6 +49,28 @@ size_t ponyint_pool_used_size(size_t index);

		size_t ponyint_pool_adjust_size(size_t size);

		#ifndef POOL_USE_DEFAULT

Manual poisoning for pool/heap when using address sanitizer #4584

Manual poisoning for pool/heap when using address sanitizer #4584

Conversation

dipinhora commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025 • edited Loading

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

dipinhora commented Jan 15, 2025

SeanTAllen commented Jan 15, 2025

SeanTAllen commented Jan 16, 2025

dipinhora commented Jan 16, 2025

SeanTAllen commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dipinhora commented Jan 21, 2025

dipinhora commented Jan 22, 2025

SeanTAllen commented Jan 15, 2025 •

edited

Loading