Skip to content

Fix memory leak in streams #2715

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

Merged
merged 2 commits into from
Jun 7, 2022

Conversation

vloup
Copy link

@vloup vloup commented Apr 14, 2022

As suggested by @JamesColeman-LW, here is a PR that fixes #2208.

All the credits goes to @marcstern for finding the root cause of the memory leak, I just did share my memory graph and added an extra ifdef.

@martinhsv martinhsv added the 2.x Related to ModSecurity version 2.x label Apr 14, 2022
@vloup vloup force-pushed the memory-leak-fix-2208 branch from ae98302 to c8d8e26 Compare May 18, 2022 12:16
@vloup
Copy link
Author

vloup commented May 18, 2022

Any news on this PR? I did a rebase since CHANGES was in conflict with v2/master. I know that @JamesColeman-LW is interested to get this merged.

@JamesColeman-LW
Copy link

I pushed this to our fleet and we definitely saw a large drop in memory usage.

Valgrind ms traces before:

[1336][root@esg-test02 valgrind]$ ms_print massif.out.657
--------------------------------------------------------------------------------
Command:            /usr/sbin/httpd -DSSL -DFOREGROUND
Massif arguments:   (none)
ms_print arguments: massif.out.657
--------------------------------------------------------------------------------


    MB
144.9^                                                                       #
     |                                                                 @ @:@:#
     |                                                           @ @:@:@:@:@:#
     |                                                    :@ @:@:@:@:@:@:@:@:#
     |                                              :@:@:@:@:@:@:@:@:@:@:@:@:#
     |                                        :@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |                                  :@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |                          :@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |                    :@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |                    @@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |       :@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |       :@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |       :@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |      @:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |     @@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |     @@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |    @@@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |    @@@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |   @@@@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
     |  @@@@@:@::::::::@::@@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:@:#
   0 +----------------------------------------------------------------------->Gi
     0                                                                   6.584

Number of snapshots: 94
 Detailed snapshots: [3, 4, 5, 7, 8, 9, 12, 23, 26, 28, 30, 32, 34, 36, 38, 40, 42, 44, 46, 48, 51, 54, 57, 60, 63, 66, 69, 72, 75, 78, 81, 84, 87, 90, 93 (peak)]

After patch applied:

[1312][root@esg-test02 valgrind]$ ms_print massif.out.13391
--------------------------------------------------------------------------------
Command:            /usr/sbin/httpd -DSSL -DFOREGROUND
Massif arguments:   (none)
ms_print arguments: massif.out.13391
--------------------------------------------------------------------------------


    MB
87.29^                                              :
     |                   @#::::::::::@:::::::::@::::::::::@:::::::::@::::::::@
     |                   @#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |          :     :::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |       ::::::::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      :: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      :: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |      @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |    @@@: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |    @ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |    @ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |    @ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |   @@ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |   @@ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |   @@ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |   @@ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
     |  :@@ @: ::: ::::::@#::::::::::@:::::::::@::::::::::@:::::::::@:::: : :@
   0 +----------------------------------------------------------------------->Gi
     0                                                                   6.969

Number of snapshots: 74
 Detailed snapshots: [3, 5, 6, 18, 19 (peak), 31, 41, 53, 63, 73]

@vloup vloup force-pushed the memory-leak-fix-2208 branch from c8d8e26 to bc8662b Compare May 30, 2022 14:16
@martinhsv martinhsv merged commit dd2d3f7 into owasp-modsecurity:v2/master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants