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

ZOOKEEPER-4809: Fix do_completion use-after-free when log level is debug #2139

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

Conversation

fanyang89
Copy link

The log callback needs to be obtained from freed zh when the log level is debug, resulting in used-after-free.

@@ -479,8 +480,9 @@ void *do_completion(void *v)
pthread_mutex_unlock(&zh->completions_to_process.lock);
process_completions(zh);
}
api_epilog(zh, 0);
LOG_DEBUG(LOGCALLBACK(zh), "completion thread terminated");
Copy link
Member

Choose a reason for hiding this comment

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

Is it more conventional to swap the two statements ? I saw several return api_epilog(zh, rc) patterns. Also, line 460 has same problem.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, it would be conventional to log first and then api_epilog().

But api_epilog() does zookeeper_close() when the rc is zero, i.e. the completion thread is still running after the "terminated" output, which could lead to confusion?

…debug

The log callback needs to be obtained from freed zhandle when the log level
is debug, resulting in used-after-free.
@eolivelli
Copy link
Contributor

@ztzg can you please take a look?

Copy link
Contributor

@ztzg ztzg left a comment

Choose a reason for hiding this comment

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

Good catch!

LGTM.

(I am still scared of the whole shutdown logic, though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants