SmartPtr: Support detect memory leak by valgrind. #4102

winlinvip · 2024-06-21T10:05:55Z

Support detect memory leak by valgrind.
Free the http handler entry.
Free the stack of ST.

suzp1984 · 2024-06-27T13:30:18Z

trunk/3rdparty/st-srs/Makefile

@@ -191,6 +191,9 @@ endif
 #
 # make EXTRA_CFLAGS=-DDEBUG_STATS
 #
+# or cache the stack and reuse it:
+# make EXTRA_CFLAGS=-DMD_CACHE_STACK


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

suzp1984 · 2024-06-27T14:31:45Z

Here is my env to testing & verify valgrind memory leak, there are a lot of outputs need to analysis, so I align my test env here.

Because I didn't find an image with valgrind in https://hub.docker.com/r/ossrs/srs/tags, I make my own one based on ossrs/srs:ubuntu20: start this image, running apt update -y && apt install -y valgrind, tag this container to an image, then I have the docker image to do the testing.

I would suggest tag an image with all the testing tools to the srs public docker hub.

Then build srs: (ossrs/srs:valgrind is the docker image in my local env)

docker run -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind bash -c "make clean_st"
docker run -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind bash -c "./configure --valgrind=on && make"

and run the srs with valgrind:

docker run -p 1935:1935 -p 1985:1985 -p 8080:8080 -p 8085:8085 --env CANDIDATE=$(ifconfig en0 inet| grep 'inet '|awk '{print $2}') -p 8000:8000/udp -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind valgrind --leak-check=full ./objs/srs -c conf/rtmp2rtc.conf

do live streams publish and play, close the srs by ctrl + \ or ctrl + c, check the output.

suzp1984 · 2024-06-27T14:47:21Z

trunk/3rdparty/st-srs/stk.c

@@ -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;


is it a mistake? returnt -> return.

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.

suzp1984 · 2024-06-27T15:53:45Z

Here is my env to testing & verify valgrind memory leak, there are a lot of outputs need to analysis, so I align my test env here.

Because I didn't find an image with valgrind in https://hub.docker.com/r/ossrs/srs/tags, I make my own one based on ossrs/srs:ubuntu20: start this image, running apt update -y && apt install -y valgrind, tag this container to an image, then I have the docker image to do the testing.

I would suggest tag an image with all the testing tools to the srs public docker hub.

Then build srs: (ossrs/srs:valgrind is the docker image in my local env)

docker run -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind bash -c "make clean_st"
docker run -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind bash -c "./configure --valgrind=on && make"

and run the srs with valgrind:

docker run -p 1935:1935 -p 1985:1985 -p 8080:8080 -p 8085:8085 --env CANDIDATE=$(ifconfig en0 inet| grep 'inet '|awk '{print $2}') -p 8000:8000/udp -it --rm -v pwd:/srs -w /srs ossrs/srs:valgrind valgrind --leak-check=full ./objs/srs -c conf/rtmp2rtc.conf

do live streams publish and play, close the srs by ctrl + \ or ctrl + c, check the output.

By this way, I found another definitely memory leak:

==1== 48 bytes in 1 blocks are definitely lost in loss record 48 of 145
==1==    at 0x484A3C4: operator new(unsigned long) (in /usr/lib/aarch64-linux-gnu/valgrind/vgpreload_memcheck-arm64-linux.so)
==1==    by 0x5BB90B: SrsPithyPrint::create_ingester() (srs_app_pithy_print.cpp:261)
==1==    by 0x5D842B: SrsIngester::SrsIngester() (srs_app_ingest.cpp:97)
==1==    by 0x517BE3: SrsServer::SrsServer() (srs_app_server.cpp:361)
==1==    by 0x51E4DB: SrsServerAdapter::SrsServerAdapter() (srs_app_server.cpp:1343)
==1==    by 0x6B842F: run_hybrid_server(void*) (srs_main_server.cpp:494)
==1==    by 0x6333F7: SrsThreadPool::start(void*) (srs_app_threads.cpp:939)
==1==    by 0x4881623: start_thread (pthread_create.c:477)
==1==    by 0x4C3049B: thread_start (clone.S:78)

srs/trunk/src/app/srs_app_ingest.cpp

Lines 89 to 106 in ea7e2c2

    
           SrsIngester::SrsIngester() 
        
           { 
        
               _srs_config->subscribe(this); 
        
               expired = false; 
        
               disposed = false; 
        
               trd = new SrsDummyCoroutine(); 
        
               pprint = SrsPithyPrint::create_ingester(); 
        
           } 
        
           SrsIngester::~SrsIngester() 
        
           { 
        
               _srs_config->unsubscribe(this); 
        
               srs_freep(trd); 
        
               clear_engines(); 
        
           }

srs_freep(pprint), maybe rename pprint to pprint_, if there is a private member naming convention in this way.

suzp1984 · 2024-06-27T16:00:46Z

trunk/3rdparty/st-srs/stk.c

+        _st_delete_stk_segment(ts->vaddr, ts->vaddr_size);
+        free(ts);
+    }
+#endif


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.

suzp1984 · 2024-06-27T16:14:36Z

trunk/3rdparty/st-srs/stk.c

@@ -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)


remove the no longer correct /* Not used */ comment.

_st_delete_stk_segment only used when #ifndef MD_CACHE_STACK, but when #ifdef MD_CACHE_STACK, is the stk segment released? or where to call _st_delete_stk_segment?

suzp1984 · 2024-06-27T16:25:58Z

trunk/src/app/srs_app_server.cpp

@@ -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>


This seems a useless include.

suzp1984 · 2024-06-28T03:41:58Z

trunk/3rdparty/st-srs/stk.c

+#endif
+
+    extra = _st_randomize_stacks ? _ST_PAGE_SIZE : 0;
+#ifndef MD_CACHE_STACK


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.

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.

TRANS_BY_GPT4

winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jun 21, 2024

winlinvip added 2 commits June 21, 2024 18:07

SmartPtr: Support detect memory leak by valgrind.

30eded4

Fix build fail for thread local debug stat.

adcfb37

winlinvip force-pushed the feature/smart-ptr-valgrind branch from 9476ab2 to adcfb37 Compare June 21, 2024 10:07

winlinvip added 2 commits June 24, 2024 12:34

SmartPtr: Free the http handler entry.

69f1998

SmartPtr: Free the stack of ST.

ca3eaf1

winlinvip marked this pull request as ready for review June 27, 2024 11:09

winlinvip requested review from suzp1984 and duiniuluantanqin June 27, 2024 11:10

suzp1984 reviewed Jun 27, 2024

View reviewed changes

suzp1984 reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SmartPtr: Support detect memory leak by valgrind. #4102

SmartPtr: Support detect memory leak by valgrind. #4102

winlinvip commented Jun 21, 2024 •

edited

Loading

suzp1984 Jun 27, 2024

suzp1984 commented Jun 27, 2024 •

edited

Loading

suzp1984 Jun 27, 2024

suzp1984 Jun 27, 2024

suzp1984 commented Jun 27, 2024

suzp1984 Jun 27, 2024

suzp1984 Jun 27, 2024

suzp1984 Jun 27, 2024

suzp1984 Jun 28, 2024

winlinvip Jun 28, 2024 •

edited

Loading

	# 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

SmartPtr: Support detect memory leak by valgrind. #4102

Are you sure you want to change the base?

SmartPtr: Support detect memory leak by valgrind. #4102

Conversation

winlinvip commented Jun 21, 2024 • edited Loading

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 commented Jun 27, 2024 • edited Loading

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 commented Jun 27, 2024

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 Jun 27, 2024

Choose a reason for hiding this comment

suzp1984 Jun 28, 2024

Choose a reason for hiding this comment

winlinvip Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

winlinvip commented Jun 21, 2024 •

edited

Loading

suzp1984 commented Jun 27, 2024 •

edited

Loading

winlinvip Jun 28, 2024 •

edited

Loading