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

bthread: bthread_setspecific may cause memory leak #1449

Open
lorinlee opened this issue Jun 26, 2021 · 5 comments
Open

bthread: bthread_setspecific may cause memory leak #1449

lorinlee opened this issue Jun 26, 2021 · 5 comments
Labels
bug the code does not work as expected

Comments

@lorinlee
Copy link
Contributor

Describe the bug (描述bug)
In the implementation of bthread_setspecific, if the current bthread has no KeyTable, a new KeyTable will be created. When the bthread ends, the created KeyTable will not be destroyed but returned to a KeyTablePool, which causes a memory leak. If we call bthread_getspecific before bthread_setspecific, the memory leak will not happen because bthread_getspecific may borrow a KeyTable from KeyTablePool and make bthread_setspecific reuse it.

This memory leak can be reproduced except these situations:

  1. using brpc::thread_local_data() before bthread_setspecific. brpc::thread_local_data() calls bthread_getspecific which may borrow a KeyTable from KeyTablePool, after that, all bthread_setspecific will use this KeyTabe instead of creating new one.
  2. using LOG() before bthread_setspecific, the reason is the same as above, the implementation of LOG() in brpc creates a LogStream which uses bthread_local, and it calls bthread_getspecific at first.
  3. calling bthread_getspecific before bthread_setspecific, also the same as above.

To Reproduce (复现方法)
we can simply modify the echo_c++ in example to reproduce this behavior, the key code just like this.

brpc::ClosureGuard done_guard(done);

brpc::Controller* cntl =
    static_cast<brpc::Controller*>(cntl_base);

if (bthread_setspecific(key1, static_cast<void*>(100)) != 0) {
    //
}

and we can add log in bthread_setspecific to see if a KeyTable created or not, the result is yes.

Expected behavior (期望行为)
users can just call bthread_setspecific, instead of calling bthread_getspecific at first.

Versions (各种版本)
OS: Ubuntu 18.04.5 LTS
Compiler: g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
brpc: master branch, commit id: 0650582
protobuf: 3.0.0-9.1ubuntu1

Additional context/screenshots (更多上下文/截图)
The bthread_setspecific will create a new KeyTable and not borrow one from KeyTablePool
image
The KeyTable will return to KeyTablePool when bthread ends.
image
The bthread_getspecific may borrow a KeyTable from KeyTablePool.
image

@lorinlee
Copy link
Contributor Author

There may have another scenario that can make memory leaked. First, we call bthread_getspecific(key1) which borrows a KeyTable from KeyTablePool, and it has old values in slot key1 and key2 in this KeyTable, then we call bthread_setspecific(key2), the old value in slot key2 may leak.

So, at now, we must call bthread_getspecific and check if there already have an old value before calling bthread_setspecific, otherwise, a memory leak may happen.

@cdjingit
Copy link
Contributor

cdjingit commented Jul 1, 2021

so, this memory leak is by design as the code comments.

@lorinlee
Copy link
Contributor Author

lorinlee commented Jul 1, 2021

@cdjingit There are some differences, the code comments just explain that memory leaks may occur if borrowing KeyTable in bthread_setspecific, so bthread_setspecific creates a new one KeyTable instead of borrowing, but this implementation needs special usage. For example, we must call bthread_getspecific before bthread_setspecific, otherwise infinite KeyTables will be created, which causes KeyTables memory to leak.

@cdjingit
Copy link
Contributor

cdjingit commented Jul 1, 2021

@lorinlee As doc description, seems we MUST call bthread_getspecific to check KeyTables before bthread_setspecific.
Otherwise, how to fix it? Add a bthread_getspecific check logic in bthread_setspecific.

@wwbmmm
Copy link
Contributor

wwbmmm commented Jul 2, 2021

I had encountered this problem before, finally I created a wrapper class that ensure calling bthread_getspecific before bthread_setspecific:

template<typename T>
class BThreadLocal {
    public:
        BThreadLocal() {
            bthread_key_create(&m_key,
                [](void* p) {  delete (T*)p; });
        }

        T* get() const { 
            return (T*)bthread_getspecific(m_key); 
        }

        T* get_or_create() {
            T *p = get();
            if (p == nullptr) {
                p = new T();
                set(p);
            }
            return p;
        }

        T *operator->() {
            return get_or_create();
        }

        T &operator*() {
            return *get_or_create();
        }

    private:
        // User can't call set(), that may cause memory leak
        int set(T* p) {
            return bthread_setspecific(m_key, p);
        }

        bthread_key_t m_key;
};

@wwbmmm wwbmmm added the bug the code does not work as expected label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug the code does not work as expected
Projects
None yet
Development

No branches or pull requests

3 participants