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

Fixes a number of memory issues reported by AddressSanitizer. #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmbeard
Copy link

@gmbeard gmbeard commented Mar 14, 2019

This PR fixes a number of memory issues reported by AddressSanitizer.

Memory Leaks

ASan detects lots of leaks. They all look similar to the below...

Direct leak of 128 byte(s) in 4 object(s) allocated from:
    #0 0x2b58317fb6f0 in __interceptor_malloc ../../../../gcc-8.1.0/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x2b58326eb506 in make_stack_item /vagrant/toml_parse.rl:731
    #2 0x2b58326ed9b1 in toml_parse /vagrant/toml_parse.rl:322
    #3 0x401beb in main /vagrant/main.c:120
    #4 0x32e301d9f3 in __libc_start_main (/lib64/libc.so.6+0x32e301d9f3)

Indirect leak of 64 byte(s) in 2 object(s) allocated from:
    #0 0x2b58317fb6f0 in __interceptor_malloc ../../../../gcc-8.1.0/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x2b58326eb506 in make_stack_item /vagrant/toml_parse.rl:731
    #2 0x2b58326ee2f2 in toml_parse /vagrant/toml_parse.rl:413
    #3 0x401beb in main /vagrant/main.c:120
    #4 0x32e301d9f3 in __libc_start_main (/lib64/libc.so.6+0x32e301d9f3)

The fixes for these are...

  1. Add free(context) to the POP_CONTEXT(x) macro. (toml_parse.rl)
  2. After the %%write_exec section, loop over the remaining items in the context stack, calling POP_CONTEXT(...) on each. (toml_parse.rl)

Invalid Memory Access

The examples/example.toml file fails ASan, giving the following error...

AddressSanitizer:DEADLYSIGNAL
=================================================================
==6764==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x2b6d54e31272 bp 0x7fffc51990d0 sp 0x7fffc5198808 T0)
==6764==The signal is caused by a READ memory access.
==6764==Hint: address points to the zero page.
    #0 0x2b6d54e31271 in __sanitizer::internal_strlen(char const*) ../../../../gcc-8.1.0/libsanitizer/sanitizer_common/sanitizer_libc.cc:171
    #1 0x2b6d54d7b22c in printf_common ../../../../gcc-8.1.0/libsanitizer/sanitizer_common/sanitizer_common_interceptors_format.inc:544
    #2 0x2b6d54d7bb52 in __interceptor_vfprintf ../../../../gcc-8.1.0/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1505
    #3 0x2b6d54d7bc16 in __interceptor_fprintf ../../../../gcc-8.1.0/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:1550
    #4 0x2b6d55d0353c in _toml_dump /vagrant/toml.c:108
    #5 0x2b6d55d035f3 in _toml_dump /vagrant/toml.c:111
    #6 0x2b6d55d03416 in _toml_dump /vagrant/toml.c:97
    #7 0x2b6d55d03270 in _toml_dump /vagrant/toml.c:81
    #8 0x2b6d55d03d34 in toml_dump /vagrant/toml.c:208
    #9 0x401dc3 in main /vagrant/main.c:141
    #10 0x32e301d9f3 in __libc_start_main (/lib64/libc.so.6+0x32e301d9f3)
    #11 0x401158  (/vagrant/build/main+0x401158)

It appears to be a problem with lists. Ensuring that, if there is no name present, setting item->node.name = NULL in the start_list action appears to fix this. (toml_parse.rl)

NOTE: I've only tested these fixes with the provided example .toml files in the project. However, it looks as though these fixes may go some way to addressing issues #18, #19, #20, #22, #23, and #24.

*ASan* detects lots of leaks relating to the *context stack*. Fixed by
adding `free(context)` to `POP_CONTEXT`, and clearing any remaining
items on the stack after parsing has completed.

The `examples/example.toml` file fails *ASan*, asserting on invalid
memory access. Fixed by ensuring that a list node's name is set to
`NULL` if no name was parsed.
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

Successfully merging this pull request may close these issues.

1 participant