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.

Sign up for GitHub

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

Jump to bottom

Call je_dallocx with flags when needed #8547

Merged
merged 1 commit into from
Jan 18, 2022
Merged

Call je_dallocx with flags when needed #8547

merged 1 commit into from
Jan 18, 2022

Conversation

duke8253
Copy link
Contributor

@duke8253 duke8253 commented Dec 7, 2021

Have been seeing some weird ASan outputs like this:

=================================================================
==21402==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x7f23e542d000 in thread T52 ([LOG_PREPROC 0]) 
    #0 0x668e70 in __interceptor_free (/opt/oath/trafficserver/9.1/bin/traffic_server+0x668e70)
    #1 0x7f2405d1f76c in ink_freelist_free ../../../../../../_scm/trafficserver9.1_asan/src/tscore/ink_queue.cc:281
    #2 0xb336f7 in LogBuffer::destroy(LogBuffer*&) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/LogBuffer.h:205
    #3 0xb336f7 in LogFile::preproc_and_try_delete(LogBuffer*) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/LogFile.cc:497
    #4 0xb5414e in LogBufferManager::preproc_buffers(LogBufferSink*) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/LogObject.cc:79
    #5 0xb5c27d in LogObject::preproc_buffers(int) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/LogObject.h:146
    #6 0xb5c27d in LogObjectManager::preproc_buffers(int) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/LogObject.cc:1084
    #7 0xafc42b in Log::preproc_thread_main(void*) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/Log.cc:1305
    #8 0xb0894c in LoggingPreprocContinuation::mainEvent(int, void*) ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/Log.cc:266
    #9 0x107d687 in Continuation::handleEvent(int, void*) ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/I_Continuation.h:219
    #10 0x107d687 in Continuation::handleEvent(int, void*) ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/I_Continuation.h:215
    #11 0x107d687 in EThread::execute() ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/UnixEThread.cc:348
    #12 0x1078345 in spawn_thread_internal ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/Thread.cc:92
    #13 0x7f2404303ea4 in start_thread /usr/src/debug/glibc-2.17-c758a686/nptl/pthread_create.c:307
    #14 0x7f24034009fc in clone (/lib64/libc.so.6+0xfe9fc)

Address 0x7f23e542d000 is a wild pointer.
SUMMARY: AddressSanitizer: bad-free (/opt/oath/trafficserver/9.1/bin/traffic_server+0x668e70) in __interceptor_free
Thread T52 ([LOG_PREPROC 0]) created by T0 here:
    #0 0x5ccd0f in pthread_create (/opt/oath/trafficserver/9.1/bin/traffic_server+0x5ccd0f)
    #1 0x10794be in ink_thread_create /sd/workspace/src/git.vzbuilders.com/Edge/build/_build/build_release_posix-x86_64_gcc_8/trafficserver9.1_asan/build/../../../../_scm/trafficserver9.1_asan/include/tscore/ink_thread.h:159
    #2 0x10794be in Thread::start(char const*, void*, unsigned long, std::function<void ()> const&) ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/Thread.cc:109
    #3 0x10879cb in EventProcessor::spawn_thread(Continuation*, char const*, unsigned long) ../../../../../../_scm/trafficserver9.1_asan/iocore/eventsystem/UnixEventProcessor.cc:497
    #4 0xafe0b3 in Log::create_threads() ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/Log.cc:1110
    #5 0xafe61b in Log::init_when_enabled() ../../../../../../_scm/trafficserver9.1_asan/proxy/logging/Log.cc:1082
    #6 0x54eb0e in main ../../../../../_scm/trafficserver9.1_asan/src/traffic_server/traffic_server.cc:2186
    #7 0x7f2403324554 in __libc_start_main ../csu/libc-start.c:266

==21402==ABORTING
=================================================================

This fixes the problem.

@duke8253 duke8253 added this to the 10.0.0 milestone Dec 7, 2021
@duke8253 duke8253 self-assigned this Dec 7, 2021
bneradt
bneradt reviewed Dec 8, 2021
View reviewed changes
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense. To summarize: just as we do some special stuff with jemalloc with allocate, so we have to do some specific de-allocation logic, right?

@duke8253
Copy link
Contributor Author

duke8253 commented Dec 8, 2021

I think this makes sense. To summarize: just as we do some special stuff with jemalloc with allocate, so we have to do some specific de-allocation logic, right?

More or less, I must have thought the default free from jemalloc was fine for the flags we were using, since the arena_id part isn't used during the free process, so I removed this part during a cleanup, but apparently I was wrong about that.

@bneradt
Copy link
Contributor

bneradt commented Dec 14, 2021

@SolidWallOfCode can give this a review after a quick change to the file we discussed in the PR issue (just pass the flag value to dallocx rather than create a variable).

@apache apache deleted a comment from masaori335 Dec 14, 2021
@apache apache deleted a comment from duke8253 Dec 14, 2021
@duke8253 duke8253 merged commit 59b7081 into apache:master Jan 18, 2022
@bneradt
Copy link
Contributor

bneradt commented Jul 12, 2022

Adding this to 9.2.x since this is a bug fix that we should get there.

zwoop pushed a commit that referenced this pull request Jul 13, 2022
@duke8253 @zwoop
call je_dallocx with flags when needed (#8547)
39f5774 
(cherry picked from commit 59b7081)
@zwoop
Copy link
Contributor

zwoop commented Jul 13, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jul 13, 2022
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@duke8253 @zwoop
call je_dallocx with flags when needed (apache#8547)
ee89658 
(cherry picked from commit 59b7081)
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Feb 21, 2023
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
a754dca 
* asf/9.2.x: (22 commits)
  Updated ChangeLog
  Add 5xx's to be allowed to be used for simple retries (apache#8518)
  Extend milestone api time tracking to remap. (apache#8520)
  Destroy ssl context after use. (apache#8531)
  Proxy Verifier: Update to version 2.4.1 (apache#8965)
  Fixes issue with file size calculation for existing logs (apache#8971)
  Fix reverting PR#7302 (apache#8975)
  add a metric to track how often the range seek bug is detected (apache#8970)
  Updated ChangeLog
  Revert "File change monitoring on s3_auth (apache#8905)"
  .fit/fmt/.clang-format-installed prerequisite (apache#8950)
  Make the autopep8 clang-format targets quieter (apache#8944)
  Add option to disable JIT in lua plugin (apache#8618)
  Fix doc formatting for plugin remap_stats (apache#8942)
  Fix doc formatting for rate_limit plugin (apache#8943)
  Clear lua plugin http context after each hook handler (apache#8607)
  LGTM: Remove function declaration in block (HdrHeap.cc) (apache#8588)
  ESI processing when origin returns 304 response (apache#8563)
  call je_dallocx with flags when needed (apache#8547)
  Updated ChangeLog
  ...

 Conflicts:
	CHANGELOG-9.2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bneradt bneradt bneradt left review comments

@SolidWallOfCode SolidWallOfCode SolidWallOfCode approved these changes

@zwoop zwoop Awaiting requested review from zwoop

Assignees

@duke8253 duke8253

Labels
None yet
Projects
None yet
Milestone
9.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@duke8253 @bneradt @zwoop @SolidWallOfCode