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

SmartPtr: Support detect memory leak by valgrind. v6.0.132 #4102

Merged
merged 6 commits into from
Jun 29, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions trunk/3rdparty/st-srs/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ endif
#
# make EXTRA_CFLAGS=-DDEBUG_STATS
#
# or cache the stack and reuse it:
# make EXTRA_CFLAGS=-DMD_CACHE_STACK
Copy link
Contributor

Choose a reason for hiding this comment

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

why not add configure args to bypass MD_CACHE_STACK to st-srs, like MD_VALGRIND?

Copy link
Member Author

@winlinvip winlinvip Jun 29, 2024

Choose a reason for hiding this comment

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

I want to not cache the stack by default, while this macro is used to enable the stack cache.

Won't fix.

#
# or enable the coverage for utest:
# make UTEST_FLAGS="-fprofile-arcs -ftest-coverage"
#
Expand Down
32 changes: 26 additions & 6 deletions trunk/3rdparty/st-srs/stk.c
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,15 @@ __thread int _st_num_free_stacks = 0;
__thread int _st_randomize_stacks = 0;

static char *_st_new_stk_segment(int size);
static void _st_delete_stk_segment(char *vaddr, int size);

_st_stack_t *_st_stack_new(int stack_size)
{
_st_clist_t *qp;
_st_stack_t *ts;
int extra;


#ifdef MD_CACHE_STACK
for (qp = _st_free_stacks.next; qp != &_st_free_stacks; qp = qp->next) {
ts = _ST_THREAD_STACK_PTR(qp);
if (ts->stk_size >= stack_size) {
Expand All @@ -72,14 +74,34 @@ _st_stack_t *_st_stack_new(int stack_size)
_st_num_free_stacks--;
ts->links.next = NULL;
ts->links.prev = NULL;
return ts;
returnt ts;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a mistake? returnt -> return.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a mistake.
To reproduce, I need to add -DMD_CACHE_STACK to auto/depends.sh, run make clean_st, then ./configure && make.

srs/trunk/auto/depends.sh

Lines 283 to 297 in ea7e2c2

# Patched ST from https://github.com/ossrs/state-threads/tree/srs
if [[ -f ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st/libst.a ]]; then
rm -rf ${SRS_OBJS}/st && cp -rf ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st ${SRS_OBJS}/ &&
echo "The state-threads is ok."
else
echo "Building state-threads." &&
rm -rf ${SRS_OBJS}/${SRS_PLATFORM}/st-srs ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st ${SRS_OBJS}/st &&
cp -rf ${SRS_WORKDIR}/3rdparty/st-srs ${SRS_OBJS}/${SRS_PLATFORM}/ &&
env EXTRA_CFLAGS="${_ST_EXTRA_CFLAGS}" make -C ${SRS_OBJS}/${SRS_PLATFORM}/st-srs ${_ST_MAKE_ARGS} &&
mkdir -p ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st &&
cp -f ${SRS_OBJS}/${SRS_PLATFORM}/st-srs/${_ST_OBJ}/st.h ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st/ &&
cp -f ${SRS_OBJS}/${SRS_PLATFORM}/st-srs/${_ST_OBJ}/libst.a ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st/ &&
cp -rf ${SRS_OBJS}/${SRS_PLATFORM}/3rdparty/st ${SRS_OBJS}/ &&
echo "The state-threads is ok."
fi

st-srs never spend too much time to build, maybe need to consider whether it's a good idea to build st-srs in this inconvenient way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

BTW: You can use configure to enable the macro:

./configure -h |grep flags
#  --extra-flags=<EFLAGS>    Set EFLAGS as CFLAGS and CXXFLAGS. Also passed to ST as EXTRA_CFLAGS.

}
}
#endif

extra = _st_randomize_stacks ? _ST_PAGE_SIZE : 0;
#ifndef MD_CACHE_STACK
Copy link
Contributor

Choose a reason for hiding this comment

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

if don't cache stack, why not free _st_stack_t in _st_stack_free, don't use _st_free_stacks to store the freed stack?
The stack still not freed in _st_free_stacks, else a new coroutine started and request a new stack here.

Copy link
Member Author

@winlinvip winlinvip Jun 28, 2024

Choose a reason for hiding this comment

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

The crash appears to occur when the st is freed within the _st_stack_free function, which seems to be happening while the stack is still in use. Although it is added to the _st_free_stacks structure, and the previous st is released before the next coroutine starts, this approach should not result in a significant memory leak issue.

Won't fix.

TRANS_BY_GPT4

for (qp = _st_free_stacks.next; qp != &_st_free_stacks;) {
ts = _ST_THREAD_STACK_PTR(qp);
// Before qp is freed, move to next one, because the qp will be freed when free the ts.
qp = qp->next;

ST_REMOVE_LINK(&ts->links);
_st_num_free_stacks--;

#if defined(DEBUG) && !defined(MD_NO_PROTECT)
mprotect(ts->vaddr, REDZONE, PROT_READ | PROT_WRITE);
mprotect(ts->stk_top + extra, REDZONE, PROT_READ | PROT_WRITE);
#endif

_st_delete_stk_segment(ts->vaddr, ts->vaddr_size);
free(ts);
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

only rarely a few part of codes in st-srs, add comments to denote which #if to match.

Suggested change
#endif
#endif /* MD_CACHE_STACK */

I would suggest add comments in this way.


/* Make a new thread stack object. */
if ((ts = (_st_stack_t *)calloc(1, sizeof(_st_stack_t))) == NULL)
return NULL;
extra = _st_randomize_stacks ? _ST_PAGE_SIZE : 0;
ts->vaddr_size = stack_size + 2*REDZONE + extra;
ts->vaddr = _st_new_stk_segment(ts->vaddr_size);
if (!ts->vaddr) {
Expand Down Expand Up @@ -114,7 +136,7 @@ void _st_stack_free(_st_stack_t *ts)
{
if (!ts)
return;

/* Put the stack on the free list */
ST_APPEND_LINK(&ts->links, _st_free_stacks.prev);
_st_num_free_stacks++;
Expand Down Expand Up @@ -153,7 +175,6 @@ static char *_st_new_stk_segment(int size)


/* Not used */
#if 0
void _st_delete_stk_segment(char *vaddr, int size)
winlinvip marked this conversation as resolved.
Show resolved Hide resolved
{
#ifdef MALLOC_STACK
Expand All @@ -162,7 +183,6 @@ void _st_delete_stk_segment(char *vaddr, int size)
(void) munmap(vaddr, size);
#endif
}
#endif

int st_randomize_stacks(int on)
{
Expand Down
1 change: 1 addition & 0 deletions trunk/research/st/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ udp-server
udp-client
cost
cost.log
thread-join
36 changes: 36 additions & 0 deletions trunk/research/st/thread-join.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
g++ thread-join.cpp ../../objs/st/libst.a -g -O0 -o thread-join && ./thread-join
*/
#include <stdio.h>
#include <stdlib.h>
#include "../../objs/st/st.h"

void* pfn(void* arg) {
printf("pid=%d, coroutine is ok\n", ::getpid());
return NULL;
}

int main(int argc, char** argv) {
st_init();

printf("pid=%d, create coroutine #1\n", ::getpid());
st_thread_t thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

st_usleep(100 * 1000);

printf("pid=%d, create coroutine #2\n", ::getpid());
thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

st_usleep(100 * 1000);

printf("pid=%d, create coroutine #3\n", ::getpid());
thread = st_thread_create(pfn, NULL, 1, 0);
st_thread_join(thread, NULL);

printf("done\n");
st_thread_exit(NULL);
return 0;
}

2 changes: 1 addition & 1 deletion trunk/src/app/srs_app_conn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ SrsResourceManager::~SrsResourceManager()
trd->stop();

srs_freep(trd);
srs_cond_destroy(cond);
}
srs_cond_destroy(cond);

clear();

Expand Down
2 changes: 1 addition & 1 deletion trunk/src/app/srs_app_hourglass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ void SrsFastTimer::unsubscribe(ISrsFastTimer* timer)
{
vector<ISrsFastTimer*>::iterator it = std::find(handlers_.begin(), handlers_.end(), timer);
if (it != handlers_.end()) {
it = handlers_.erase(it);
handlers_.erase(it);
}
}

Expand Down
6 changes: 3 additions & 3 deletions trunk/src/app/srs_app_http_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1073,9 +1073,6 @@ void SrsHttpStreamServer::http_unmount(SrsRequest* r)
SrsBufferCache* cache = entry->cache;
SrsAutoFree(SrsBufferCache, cache);

// Unmount the HTTP handler.
mux.unhandle(entry->mount, stream);

// Notify cache and stream to stop.
if (stream->entry) stream->entry->enabled = false;
cache->stop();
Expand All @@ -1089,6 +1086,9 @@ void SrsHttpStreamServer::http_unmount(SrsRequest* r)
srs_usleep(100 * SRS_UTIME_MILLISECONDS);
}

// Unmount the HTTP handler.
mux.unhandle(entry->mount, stream);

srs_trace("http: unmount flv stream for sid=%s, i=%d", sid.c_str(), i);
}

Expand Down
86 changes: 43 additions & 43 deletions trunk/src/app/srs_app_hybrid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,55 +25,55 @@ extern SrsPps* _srs_pps_conn;
extern SrsPps* _srs_pps_dispose;

#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
extern unsigned long long _st_stat_recvfrom;
extern unsigned long long _st_stat_recvfrom_eagain;
extern unsigned long long _st_stat_sendto;
extern unsigned long long _st_stat_sendto_eagain;
extern __thread unsigned long long _st_stat_recvfrom;
extern __thread unsigned long long _st_stat_recvfrom_eagain;
extern __thread unsigned long long _st_stat_sendto;
extern __thread unsigned long long _st_stat_sendto_eagain;
SrsPps* _srs_pps_recvfrom = NULL;
SrsPps* _srs_pps_recvfrom_eagain = NULL;
SrsPps* _srs_pps_sendto = NULL;
SrsPps* _srs_pps_sendto_eagain = NULL;

extern unsigned long long _st_stat_read;
extern unsigned long long _st_stat_read_eagain;
extern unsigned long long _st_stat_readv;
extern unsigned long long _st_stat_readv_eagain;
extern unsigned long long _st_stat_writev;
extern unsigned long long _st_stat_writev_eagain;
extern __thread unsigned long long _st_stat_read;
extern __thread unsigned long long _st_stat_read_eagain;
extern __thread unsigned long long _st_stat_readv;
extern __thread unsigned long long _st_stat_readv_eagain;
extern __thread unsigned long long _st_stat_writev;
extern __thread unsigned long long _st_stat_writev_eagain;
SrsPps* _srs_pps_read = NULL;
SrsPps* _srs_pps_read_eagain = NULL;
SrsPps* _srs_pps_readv = NULL;
SrsPps* _srs_pps_readv_eagain = NULL;
SrsPps* _srs_pps_writev = NULL;
SrsPps* _srs_pps_writev_eagain = NULL;

extern unsigned long long _st_stat_recvmsg;
extern unsigned long long _st_stat_recvmsg_eagain;
extern unsigned long long _st_stat_sendmsg;
extern unsigned long long _st_stat_sendmsg_eagain;
extern __thread unsigned long long _st_stat_recvmsg;
extern __thread unsigned long long _st_stat_recvmsg_eagain;
extern __thread unsigned long long _st_stat_sendmsg;
extern __thread unsigned long long _st_stat_sendmsg_eagain;
SrsPps* _srs_pps_recvmsg = NULL;
SrsPps* _srs_pps_recvmsg_eagain = NULL;
SrsPps* _srs_pps_sendmsg = NULL;
SrsPps* _srs_pps_sendmsg_eagain = NULL;

extern unsigned long long _st_stat_epoll;
extern unsigned long long _st_stat_epoll_zero;
extern unsigned long long _st_stat_epoll_shake;
extern unsigned long long _st_stat_epoll_spin;
extern __thread unsigned long long _st_stat_epoll;
extern __thread unsigned long long _st_stat_epoll_zero;
extern __thread unsigned long long _st_stat_epoll_shake;
extern __thread unsigned long long _st_stat_epoll_spin;
SrsPps* _srs_pps_epoll = NULL;
SrsPps* _srs_pps_epoll_zero = NULL;
SrsPps* _srs_pps_epoll_shake = NULL;
SrsPps* _srs_pps_epoll_spin = NULL;

extern unsigned long long _st_stat_sched_15ms;
extern unsigned long long _st_stat_sched_20ms;
extern unsigned long long _st_stat_sched_25ms;
extern unsigned long long _st_stat_sched_30ms;
extern unsigned long long _st_stat_sched_35ms;
extern unsigned long long _st_stat_sched_40ms;
extern unsigned long long _st_stat_sched_80ms;
extern unsigned long long _st_stat_sched_160ms;
extern unsigned long long _st_stat_sched_s;
extern __thread unsigned long long _st_stat_sched_15ms;
extern __thread unsigned long long _st_stat_sched_20ms;
extern __thread unsigned long long _st_stat_sched_25ms;
extern __thread unsigned long long _st_stat_sched_30ms;
extern __thread unsigned long long _st_stat_sched_35ms;
extern __thread unsigned long long _st_stat_sched_40ms;
extern __thread unsigned long long _st_stat_sched_80ms;
extern __thread unsigned long long _st_stat_sched_160ms;
extern __thread unsigned long long _st_stat_sched_s;
SrsPps* _srs_pps_sched_15ms = NULL;
SrsPps* _srs_pps_sched_20ms = NULL;
SrsPps* _srs_pps_sched_25ms = NULL;
Expand All @@ -96,11 +96,12 @@ SrsPps* _srs_pps_clock_160ms = NULL;
SrsPps* _srs_pps_timer_s = NULL;

#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
extern int _st_active_count;
extern unsigned long long _st_stat_thread_run;
extern unsigned long long _st_stat_thread_idle;
extern unsigned long long _st_stat_thread_yield;
extern unsigned long long _st_stat_thread_yield2;
extern __thread int _st_active_count;
extern __thread int _st_num_free_stacks;
extern __thread unsigned long long _st_stat_thread_run;
extern __thread unsigned long long _st_stat_thread_idle;
extern __thread unsigned long long _st_stat_thread_yield;
extern __thread unsigned long long _st_stat_thread_yield2;
SrsPps* _srs_pps_thread_run = NULL;
SrsPps* _srs_pps_thread_idle = NULL;
SrsPps* _srs_pps_thread_yield = NULL;
Expand Down Expand Up @@ -135,19 +136,20 @@ SrsHybridServer::SrsHybridServer()

SrsHybridServer::~SrsHybridServer()
{
srs_freep(clock_monitor_);

srs_freep(timer20ms_);
srs_freep(timer100ms_);
srs_freep(timer1s_);
srs_freep(timer5s_);

// We must free servers first, because it may depend on the timers of hybrid server.
vector<ISrsHybridServer*>::iterator it;
for (it = servers.begin(); it != servers.end(); ++it) {
ISrsHybridServer* server = *it;
srs_freep(server);
}
servers.clear();

srs_freep(clock_monitor_);

srs_freep(timer20ms_);
srs_freep(timer100ms_);
srs_freep(timer1s_);
srs_freep(timer5s_);
}

void SrsHybridServer::register_server(ISrsHybridServer* svr)
Expand Down Expand Up @@ -237,8 +239,6 @@ void SrsHybridServer::stop()
ISrsHybridServer* server = *it;
server->stop();
}

srs_st_destroy();
}

SrsServerAdapter* SrsHybridServer::srs()
Expand Down Expand Up @@ -372,8 +372,8 @@ srs_error_t SrsHybridServer::on_timer(srs_utime_t interval)
#if defined(SRS_DEBUG) && defined(SRS_DEBUG_STATS)
_srs_pps_thread_run->update(_st_stat_thread_run); _srs_pps_thread_idle->update(_st_stat_thread_idle);
_srs_pps_thread_yield->update(_st_stat_thread_yield); _srs_pps_thread_yield2->update(_st_stat_thread_yield2);
if (_st_active_count > 0 || _srs_pps_thread_run->r10s() || _srs_pps_thread_idle->r10s() || _srs_pps_thread_yield->r10s() || _srs_pps_thread_yield2->r10s()) {
snprintf(buf, sizeof(buf), ", co=%d,%d,%d, yield=%d,%d", _st_active_count, _srs_pps_thread_run->r10s(), _srs_pps_thread_idle->r10s(), _srs_pps_thread_yield->r10s(), _srs_pps_thread_yield2->r10s());
if (_st_active_count > 0 || _st_num_free_stacks > 0 || _srs_pps_thread_run->r10s() || _srs_pps_thread_idle->r10s() || _srs_pps_thread_yield->r10s() || _srs_pps_thread_yield2->r10s()) {
snprintf(buf, sizeof(buf), ", co=%d,%d,%d, stk=%d, yield=%d,%d", _st_active_count, _srs_pps_thread_run->r10s(), _srs_pps_thread_idle->r10s(), _st_num_free_stacks, _srs_pps_thread_yield->r10s(), _srs_pps_thread_yield2->r10s());
thread_desc = buf;
}
#endif
Expand Down
9 changes: 3 additions & 6 deletions trunk/src/app/srs_app_server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ using namespace std;
#include <srs_protocol_log.hpp>
#include <srs_app_latest_version.hpp>
#include <srs_app_conn.hpp>
#include <srs_app_threads.hpp>
winlinvip marked this conversation as resolved.
Show resolved Hide resolved
#ifdef SRS_RTC
#include <srs_app_rtc_network.hpp>
#include <srs_app_rtc_server.hpp>
Expand Down Expand Up @@ -370,8 +371,6 @@ SrsServer::~SrsServer()

void SrsServer::destroy()
{
srs_warn("start destroy server");

srs_freep(trd_);
srs_freep(timer_);

Expand Down Expand Up @@ -869,11 +868,8 @@ void SrsServer::stop()
srs_trace("srs gracefully quit");
}

// This is the last line log of SRS.
srs_trace("srs terminated");

// for valgrind to detect.
srs_freep(_srs_config);
srs_freep(_srs_log);
}

srs_error_t SrsServer::cycle()
Expand Down Expand Up @@ -1406,6 +1402,7 @@ srs_error_t SrsServerAdapter::run(SrsWaitGroup* wg)

void SrsServerAdapter::stop()
{
srs->stop();
}

SrsServer* SrsServerAdapter::instance()
Expand Down
Loading
Loading