Fix ASAN issue in lib/cstring. #4865

matthewtlam · 2024-08-12T18:06:10Z

Signed-off-by: Matthew Lam matthew.lam.qwerty@gmail.com

matthewtlam · 2024-08-12T18:11:34Z

For context, running the ASAN tool on one of our tests revealed an initialization_order_fiasco runtime error.

SanitizerError
AddressSanitizer: initialization-order-fiasco [third_party/p4lang_p4c/lib/json.h:110]:23 in JsonValue;
Details
=================================================================
==4779==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x7f2eaf3ee3c0 at pc 0x7f2eaefdbe2b bp 0x7ffdb7f50ba0 sp 0x7ffdb7f50b98
READ of size 8 at 0x7f2eaf3ee3c0 thread T0
    #0 0x7f2eaefdbe2a in JsonValue [third_party/p4lang_p4c/lib/json.h:110]:23
    #1 0x7f2eaefdbe2a in __cxx_global_var_init.2 [third_party/p4lang_p4c/lib/json.cpp:36]:34
    #2 0x7f2eaefdbe2a in _GLOBAL__sub_I_json.cpp third_party/p4lang_p4c/lib/json.cpp
    #3 0x7f2ecbbade29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7f2ecbbadc8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7f2ecbb99659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

0x7f2eaf3ee3c0 is located 32 bytes before global variable '(anonymous namespace)::cache()::g_cache' defined in 'third_party/p4lang_p4c/lib/cstring.cpp' (0x7f2eaf3ee3e0) of size 56
  registered at:
    #0 0x55fa54125a58 in __asan_register_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:369]:3
    #1 0x55fa54126b6e in __asan_register_elf_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:352]:3
    #2 0x7f2eaef73247 in asan.module_ctor alloc_trace.cpp
    #3 0x7f2ecbbade29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7f2ecbbadc8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7f2ecbb99659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

0x7f2eaf3ee3c0 is located 0 bytes inside of global variable 'cstring::empty' defined in 'third_party/p4lang_p4c/lib/cstring.cpp' (0x7f2eaf3ee3c0) of size 8
  registered at:
    #0 0x55fa54125a58 in __asan_register_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:369]:3
    #1 0x55fa54126b6e in __asan_register_elf_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:352]:3
    #2 0x7f2eaef73247 in asan.module_ctor alloc_trace.cpp
    #3 0x7f2ecbbade29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7f2ecbbadc8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7f2ecbb99659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

SUMMARY: AddressSanitizer: initialization-order-fiasco [third_party/p4lang_p4c/lib/json.h:110]:23 in JsonValue

matthewtlam · 2024-08-12T18:12:01Z

@jonathan-dilorenzo, @smolkaj for visibility

Signed-off-by: Matthew Lam matthew.lam.qwerty@gmail.com

asl · 2024-08-12T18:19:16Z

I actually do have patch for this downstream. This is not a complete solution. It only fixes the problem in the single file. However, the order of static initializers is not defined across the translation units. So the cstring literal could initialized somewhere else.

qobilidop · 2024-08-12T18:19:29Z

Do we understand why this wasn't caught by the existing sanitizer CI workflow on GitHub?

asl · 2024-08-12T18:21:24Z

Do we understand why this wasn't caught by the existing sanitizer CI workflow on GitHub?

Yes. Though I was able to reproduce it with asan on downstream project. The order of static initializers do matter. And this depends on linker.

asl · 2024-08-12T18:27:49Z

I submitted downstream solution in #4866 that should fix the underlying problem

@matthewtlam will you please double-check it fixes asan issues for you?

matthewtlam · 2024-08-12T18:30:55Z

@asl I will double-check that #4866 fixes the ASAN issues on my side

asl · 2024-08-12T18:34:00Z

Do we understand why this wasn't caught by the existing sanitizer CI workflow on GitHub?

To add more on this. We might want to try ASAN_OPTIONS=check_initialization_order=true:strict_init_order=true. Though it might be overly-aggressive :)

matthewtlam · 2024-08-12T18:39:05Z

@asl Unfortunately, the ASAN check failed with the changes in #4866 as shown below. I believe we use the most strict form of checking for ASAN.

SanitizerError
AddressSanitizer: initialization-order-fiasco [third_party/p4lang_p4c/lib/json.h:110]:23 in JsonValue (//third_party/pins_infra/p4testgen/bmv2:p4testgen_bmv2_integration_test); see [http://go/sanitizer-blaze-warning]
Details
=================================================================
==1324==ERROR: AddressSanitizer: initialization-order-fiasco on address 0x7fd667be8800 at pc 0x7fd6677baceb bp 0x7ffe123e3380 sp 0x7ffe123e3378
READ of size 8 at 0x7fd667be8800 thread T0
    #0 0x7fd6677bacea in JsonValue [third_party/p4lang_p4c/lib/json.h:110]:23
    #1 0x7fd6677bacea in __cxx_global_var_init.2 [third_party/p4lang_p4c/lib/json.cpp:36]:34
    #2 0x7fd6677bacea in _GLOBAL__sub_I_json.cpp third_party/p4lang_p4c/lib/json.cpp
    #3 0x7fd683b9ae29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7fd683b9ac8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7fd683b86659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

0x7fd667be8800 is located 32 bytes before global variable '(anonymous namespace)::cache()::g_cache' defined in 'third_party/p4lang_p4c/lib/cstring.cpp' (0x7fd667be8820) of size 8
  registered at:
    #0 0x558a85125c98 in __asan_register_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:392]:3
    #1 0x558a85126ece in __asan_register_elf_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:375]:3
    #2 0x7fd6677803b7 in asan.module_ctor alloc_trace.cpp
    #3 0x7fd683b9ae29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7fd683b9ac8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7fd683b86659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

0x7fd667be8800 is located 0 bytes inside of global variable 'cstring::empty' defined in 'third_party/p4lang_p4c/lib/cstring.cpp' (0x7fd667be8800) of size 8
  registered at:
    #0 0x558a85125c98 in __asan_register_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:392]:3
    #1 0x558a85126ece in __asan_register_elf_globals [third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:375]:3
    #2 0x7fd6677803b7 in asan.module_ctor alloc_trace.cpp
    #3 0x7fd683b9ae29 in call_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1be29) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #4 0x7fd683b9ac8a in _dl_init (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x1bc8a) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)
    #5 0x7fd683b86659 in _dl_start_user (/usr/grte/v5/lib64/ld-linux-x86-64.so.2+0x7659) (BuildId: 3c4ff280c4f5e542ce622870f20d94f2)

SUMMARY: AddressSanitizer: initialization-order-fiasco [third_party/p4lang_p4c/lib/json.h:110]:23 in JsonValue

asl · 2024-08-12T18:48:42Z

@asl Unfortunately, the ASAN check failed with the changes in #4866 as shown below. I believe we use the most strict form of checking for ASAN.

Ah, actually this is a different issue!

#4866 fixes the problem when cstring cache is not live before any of cstring literals around.
#4865 fixes the problem where cstrings (cstring::empty and cstring::newline) are not live before json

So, it looks like we need to use both. And actually reduce the number of static objects around, this makes p4c-as-a-library usage hard :(

@vlstill Should we sunset cstring::empty and cstring::newline in the presence of _cs literals?

lib/cstring.h

DCO Remediation Commit for Matthew Lam <matthew.lam.qwerty@gmail.com> I, Matthew Lam <matthew.lam.qwerty@gmail.com>, hereby add my Signed-off-by to this commit: a19e22e I, Matthew Lam <matthew.lam.qwerty@gmail.com>, hereby add my Signed-off-by to this commit: 6ccde62 I, Matthew Lam <matthew.lam.qwerty@gmail.com>, hereby add my Signed-off-by to this commit: fe76037 Signed-off-by: Matthew Lam <matthew.lam.qwerty@gmail.com> Signed-off-by: Matthew Lam <matthew.lam.qwerty@gmail.com>

lib/cstring.h

smolkaj

LGTM

Fix ASAN issue in lib/cstring.

a19e22e

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Aug 12, 2024

matthewtlam added 2 commits August 12, 2024 11:14

Remove unneccessary header

6ccde62

Add missing libraries.

fe76037

Signed-off-by: Matthew Lam matthew.lam.qwerty@gmail.com

asl mentioned this pull request Aug 12, 2024

Lazily initialize cstring cache. #4866

Closed

asl reviewed Aug 12, 2024

View reviewed changes

lib/cstring.h Outdated Show resolved Hide resolved

smolkaj reviewed Aug 12, 2024

View reviewed changes

lib/cstring.h Show resolved Hide resolved

smolkaj approved these changes Aug 12, 2024

View reviewed changes

asl added this pull request to the merge queue Aug 12, 2024

Merged via the queue into p4lang:main with commit e89f843 Aug 12, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ASAN issue in lib/cstring. #4865

Fix ASAN issue in lib/cstring. #4865

matthewtlam commented Aug 12, 2024 •

edited

Loading

matthewtlam commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024 •

edited

Loading

qobilidop commented Aug 12, 2024

asl commented Aug 12, 2024

asl commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024 •

edited

Loading

smolkaj left a comment

Fix ASAN issue in lib/cstring. #4865

Fix ASAN issue in lib/cstring. #4865

Conversation

matthewtlam commented Aug 12, 2024 • edited Loading

matthewtlam commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024 • edited Loading

qobilidop commented Aug 12, 2024

asl commented Aug 12, 2024

asl commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024

matthewtlam commented Aug 12, 2024

asl commented Aug 12, 2024 • edited Loading

smolkaj left a comment

Choose a reason for hiding this comment

matthewtlam commented Aug 12, 2024 •

edited

Loading

asl commented Aug 12, 2024 •

edited

Loading

asl commented Aug 12, 2024 •

edited

Loading