drop lsan suppressions by nshy · Pull Request #10207 · tarantool/tarantool

nshy · 2024-07-05T15:29:47Z

Required small changes - tarantool/small#91
EE part - https://github.com/tarantool/tarantool-ee/pull/835

coveralls · 2024-07-05T16:03:07Z

coverage: 87.166% (+0.08%) from 87.089%
when pulling b710328 on nshy:suppressions-prepare
into 7db4de7
on tarantool:master.

coveralls · 2024-07-08T12:15:11Z

coverage: 87.3% (+0.08%) from 87.219%
when pulling 4bfa9c8 on nshy:suppressions-prepare
into 218fc46
on tarantool:master.

p7nov · 2024-07-16T04:39:38Z

changelogs/unreleased/fix-memory-leak-in-iconv.md

@@ -0,0 +1,3 @@
+## bugfix/lua
+
+- Fixed iconv object memory leak.


Suggested change

- Fixed iconv object memory leak.

- Fixed an iconv object memory leak.

p7nov · 2024-07-16T04:39:48Z

changelogs/unreleased/fix-memory-leak-in-vinyl-on-dump-failure.md

@@ -0,0 +1,3 @@
+## bugfix/vinyl
+
+- Fixed memory leak in vinyl on dump/compaction failure.


Suggested change

- Fixed memory leak in vinyl on dump/compaction failure.

- Fixed a memory leak in vinyl on dump/compaction failure.

p7nov · 2024-07-16T04:40:51Z

changelogs/unreleased/fix-misc-memory-leaks.md

+- Fixed memory leak on disconnectiong from replica.
+- Fixed memory leak on user ddl on access check failure.
+- Fixed memory leak on xlog open failure.
+- Fixed memory leak on space ddl on certain condition.
+- Fixed memory leak on iproto override usage (since 3.1).
+- Fixed memory leak on ddl failed due to foreign key constraint check.
+- Fixed memory leak on space drop with sequence bound to json path.


Suggested change

- Fixed memory leak on disconnectiong from replica.

- Fixed memory leak on user ddl on access check failure.

- Fixed memory leak on xlog open failure.

- Fixed memory leak on space ddl on certain condition.

- Fixed memory leak on iproto override usage (since 3.1).

- Fixed memory leak on ddl failed due to foreign key constraint check.

- Fixed memory leak on space drop with sequence bound to json path.

- Fixed a memory leak on disconnection from a replica.

- Fixed a memory leak on user DDL on access check failure.

- Fixed a memory leak on `xlog` open failure.

- Fixed a memory leak on space ddl on certain condition.

- Fixed a memory leak on iproto override usage (since 3.1).

- Fixed a memory leak on DDL failed due to foreign key constraint check.

- Fixed a memory leak on space drop with sequence bound to json path.

locker · 2024-07-17T08:41:52Z

changelogs/unreleased/fix-misc-memory-leaks.md

+- Fixed memory leak on ddl failed due to foreign key constraint check.
+- Fixed memory leak on space drop with sequence bound to json path.
+
+## bugfix/core


You can't mix release notes for different subsystems in the same file - gen-release-notes won't combine them properly.

locker · 2024-07-17T09:45:56Z

src/box/alter.cc

-	alter->new_space->sequence_path = alter->old_space->sequence_path;
+	if (alter->old_space->sequence_path != NULL)
+		alter->new_space->sequence_path =
+			(char *)xstrdup(alter->old_space->sequence_path);


Let's split this patch into different commits, for example:

Leak on DDL access check failure.

Leak on xlog open failure.

Leak on iproto override.

Leak on replication reconfiguration.

Leak on network address resolution error.

Also, it's preferable to submit some fixes in separate PRs because a PR with > 5-7 commits is difficult to review.

locker · 2024-07-17T09:58:23Z

src/trivia/util.h

 #endif

+/** If false then LSAN will not be run on Tarantool exit. */
+extern bool lsan_enabled;


I think this should be defined only if ENABLE_ASAN is set.

locker · 2024-07-17T09:59:05Z

src/lib/core/util.c

 }
+
+int
+__lsan_is_turned_off(void)


Please add a comment explaining that this a function called internally by ASAN to check if it should report leaks.

locker · 2024-07-17T10:00:25Z

src/lib/swim/swim.c

 void
 swim_gc(struct swim *swim)
 {
+	if (swim->event_handler == NULL) {


Please reference the commit that introduced the leak in the commit message.

locker · 2024-07-17T10:03:31Z

src/box/lua/space.cc

+		/* tarantool_L is freed on Tarantool shutdown. */
+		if (L == NULL)
+			return 0;


Shouldn't box be destroyed before Lua? I don't like that there are so many places now where we have to check tarantool_L.

locker · 2024-07-17T10:05:34Z

src/box/memtx_engine.cc

+		bool stop = false;
+		while (!stop)
+			memtx_engine_run_gc(memtx, &stop);


I think this should be done when the engine is destroyed, not when we schedule garbage collection.

locker · 2024-07-17T10:10:26Z

src/box/txn_limbo.h

+ * Denitialize qsync engine.
+ */
+void
+txn_limbo_destroy();


Should be named txn_limbo_free according to our naming convention. Anyway, there are so many susbystems touched in this commit with so different cases so it may be better to split this commit as well and submit it in a different PR.

locker · 2024-07-17T10:13:20Z

asan/lsan.supp

-# TODO(gh-9213): remove when the issue is fixed
-leak:luaT_push_key_def
-
-# TODO(gh-8890): remove when the issue is fixed


Let's add Closes #XXXX to commit messages where applicable. Also, it'd be better if you removed the suppressions in the commits that actually fix the leaks.

GC fiber is dead at this point. We don't need to free tuples anyway for non ASAN builds on shutdown. For ASAN builds let's free tuples in memtx_engine_free(). Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

Otherwise we got a lot of FP leak reports. We also need to add check if global Lua state is closed in several places which can be executed after closing Lua state. Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

It was introduced in the commit 446201b ("asan: turn ASAN allocators on finally"). I probably added `ibuf_destroy` to fix a memory leak report but `ibuf` is passed from Lua so it is garbage collected by Lua. We don't have issues with double free probably because we don't close Lua on Tarantool exit until now. Part-of tarantool#10211 NO_CHANGELOG=internal NO_DOC=internal

On Tarantool shutdown first swim worker fiber is finished as managed on shutdown one. So later when swim is collected we got crash in swim_gc() because `event_handler` is NULL at this point. There is no worker fiber so we need to free swim synchronously. The issue was hidden because we do not call lua_close() earlier. It had little change to occur. Now it is revealed. Closes tarantool#10495 Part-of tarantool#10211 NO_TEST=covered by existing tests NO_DOC=bugfix

Introduced in the commit ab3746b ("gc: persist WAL GC consumers"). Part-of tarantool#10211 NO_TEST=covered by existing tests NO_CHANGELOG=not released NO_DOC=bugfix

Closes tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

nshy · 2024-09-04T11:07:58Z

Closed as superseded by #10475 #10491 #10500 #10505 #10516.

nshy requested a review from a team as a code owner July 5, 2024 15:29

nshy force-pushed the suppressions-prepare branch from 4f16a56 to b710328 Compare July 5, 2024 15:49

nshy added the asan-ci Enables asan build tests for a pull request label Jul 5, 2024

nshy force-pushed the suppressions-prepare branch from b710328 to 15e588f Compare July 8, 2024 11:57

nshy force-pushed the suppressions-prepare branch 4 times, most recently from 7470d5e to 3279431 Compare July 12, 2024 15:25

nshy force-pushed the suppressions-prepare branch from 3279431 to 58d8951 Compare July 15, 2024 15:38

nshy changed the title ~~draft: drop lsan suppressions~~ drop lsan suppressions Jul 15, 2024

nshy added the do not merge Not ready to be merged label Jul 15, 2024

nshy mentioned this pull request Jul 15, 2024

fix LSAN memory leak reports tarantool/small#91

Merged

p7nov reviewed Jul 16, 2024

View reviewed changes

p7nov approved these changes Jul 16, 2024

View reviewed changes

nshy force-pushed the suppressions-prepare branch from 58d8951 to c8ee2a4 Compare July 16, 2024 08:10

nshy requested a review from locker July 16, 2024 09:34

nshy assigned locker Jul 16, 2024

locker reviewed Jul 17, 2024

View reviewed changes

locker assigned nshy and unassigned locker Jul 17, 2024

This was referenced Aug 26, 2024

Fix memory leaks reported by ASAN (with suppressions turned off) part 1 #10475

Merged

Memory leak in in coio_connect_timeout #10482

Closed

nshy removed the asan-ci Enables asan build tests for a pull request label Aug 28, 2024

nshy force-pushed the suppressions-prepare branch 2 times, most recently from 91dfe4c to ff5e624 Compare August 28, 2024 13:14

nshy added the asan-ci Enables asan build tests for a pull request label Aug 28, 2024

nshy force-pushed the suppressions-prepare branch from ff5e624 to 1f7fbd6 Compare September 2, 2024 12:16

nshy mentioned this pull request Sep 2, 2024

Prepare to drop LSAN suppressions, part 2 #10505

Merged

nshy force-pushed the suppressions-prepare branch 2 times, most recently from 06eacc3 to cd541ec Compare September 3, 2024 12:01

nshy added 13 commits September 3, 2024 15:16

box: free users cache on exit

89c234a

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: free space cache on exit

d6539d9

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: free space sql triggers on exit

98457e5

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: free coll_id cache on exit

884e005

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: free replication on exit

2898774

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: free txn limbo on exit

65767c4

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

memtx: free extents on exit

0d23425

Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

box: close Lua state on exit

9bfcb02

Otherwise we got a lot of FP leak reports. We also need to add check if global Lua state is closed in several places which can be executed after closing Lua state. Part-of tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

gc: fix LSAN leak report in gc_load_consumers

59c014a

Introduced in the commit ab3746b ("gc: persist WAL GC consumers"). Part-of tarantool#10211 NO_TEST=covered by existing tests NO_CHANGELOG=not released NO_DOC=bugfix

lsan: remove most of suppressions

4bfa9c8

Closes tarantool#10211 NO_TEST=internal NO_CHANGELOG=internal NO_DOC=internal

nshy force-pushed the suppressions-prepare branch from cd541ec to 4bfa9c8 Compare September 3, 2024 12:31

nshy mentioned this pull request Sep 4, 2024

Drop LSAN suppressions (almost) #10516

Merged

nshy closed this Sep 4, 2024

nshy deleted the suppressions-prepare branch September 9, 2024 15:13

		@@ -0,0 +1,3 @@
		## bugfix/lua

		- Fixed iconv object memory leak.

	- Fixed iconv object memory leak.
	- Fixed an iconv object memory leak.

		@@ -0,0 +1,3 @@
		## bugfix/vinyl

		- Fixed memory leak in vinyl on dump/compaction failure.

	- Fixed memory leak in vinyl on dump/compaction failure.
	- Fixed a memory leak in vinyl on dump/compaction failure.

Conversation

nshy commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 5, 2024

Uh oh!

coveralls commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nshy commented Sep 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nshy commented Jul 5, 2024 •

edited

Loading

coveralls commented Jul 8, 2024 •

edited

Loading