Skip to content

SNIConfig: tunnel_route - Change the way we extract matched subgroups from the servename#8589

Merged
brbzull0 merged 1 commit intoapache:masterfrom
brbzull0:dmeden/issue_8564
Jan 19, 2022
Merged

SNIConfig: tunnel_route - Change the way we extract matched subgroups from the servename#8589
brbzull0 merged 1 commit intoapache:masterfrom
brbzull0:dmeden/issue_8564

Conversation

@brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Jan 7, 2022 •
edited
Loading

This PR is created to fix the leak around the non-freed memory happening when there is more than one matched group detected when we try to rearrange the servername destination base on the SNI tunnel_route configuration.

The allocation for the subgroups is removed and instead this now uses the provided offsets to read each matched sub-group from the server name, this avoids allocating(by pcre_get_substring) memory for each substring.
This change also removes the std::vector<std::string> to avoid an extra allocation for the string and instead it now holds only a view of the subgroup string.

Note: servername gets allocated and copied inside the TLSSNISupport object, this chunk of memory is kept around for the durations of the perform_sni_action call. With the string around it looks safe to hold the views instead of a copy of the substrings inside the ActionItem::Context.

This fixes #8564

@brbzull0 brbzull0 requested a review from bryancall as a code owner January 7, 2022 11:52
@brbzull0 brbzull0 self-assigned this Jan 7, 2022
@brbzull0 brbzull0 added this to the 10.0.0 milestone Jan 7, 2022
@brbzull0 brbzull0 changed the title SNIConfig: Change the way we extract matched subgroups from the servename. SNIConfig: Change the way we extract matched subgroups from the servename. fqdn->tunnel_route Jan 7, 2022
@brbzull0 brbzull0 changed the title SNIConfig: Change the way we extract matched subgroups from the servename. fqdn->tunnel_route SNIConfig: Change the way we extract matched subgroups from the servename(fqdn -> tunnel_route) Jan 7, 2022
@brbzull0 brbzull0 changed the title SNIConfig: Change the way we extract matched subgroups from the servename(fqdn -> tunnel_route) SNIConfig: tunnel_route - Change the way we extract matched subgroups from the servename Jan 7, 2022
@brbzull0 brbzull0 marked this pull request as draft January 7, 2022 18:56
@brbzull0
Copy link
Contributor Author

brbzull0 commented Jan 7, 2022

After some comments from @bneradt there could be another issue in here. I may need to change this a little bit more. Changing it to draft till I make more changes.

@brbzull0 brbzull0 marked this pull request as ready for review January 10, 2022 15:15
SolidWallOfCode
SolidWallOfCode reviewed Jan 10, 2022
View reviewed changes
iocore/net/SSLSNIConfig.cc Outdated
return {&retval.actions, context};
groups.emplace_back(servername.data() + start, length);
}
return {&retval.actions, {groups}};
Copy link
Member

@SolidWallOfCode SolidWallOfCode Jan 10, 2022

Choose a reason for hiding this comment

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

Shouldn't that be {std::move(groups)}?

@brbzull0
SNIConfig (tunnel_route): Change the way we extract matched subgroups…
2e485a1 
… from the server name.

This now uses the provided offsets from pcre_exec to read each matched group, this avoids
allocating memory for the subgroups. There was a memory leak here as well which is now eliminated.
This also changes the ActionItem::Context vector of strings to a vector of views to keep each matched group.
bneradt
bneradt reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks Damian.

@brbzull0 brbzull0 merged commit 4f0c4f2 into apache:master Jan 19, 2022
zwoop pushed a commit that referenced this pull request Jan 25, 2022
@brbzull0 @zwoop
SNIConfig (tunnel_route): Change the way we extract matched subgroups…
d4a7ab0 
… from the server name. (#8589)

This now uses the provided offsets from pcre_exec to read each matched group, this avoids
allocating memory for the subgroups. There was a memory leak here as well which is now eliminated.
This also changes the ActionItem::Context vector of strings to a vector of views to keep each matched group.

(cherry picked from commit 4f0c4f2)
@zwoop
Copy link
Contributor

zwoop commented Jan 25, 2022

Cherry-picked to v9.2.x

@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jan 25, 2022
@pbchou pbchou mentioned this pull request Mar 15, 2022
Closed
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 17, 2022
@zwoop
Merge remote-tracking branch 'asf/9.2.x' into 9.2.x
8ec090e 
* asf/9.2.x:
  Updated ChangeLog
  Add SSLSessionDup for older OpenSSL and BoringSSL (apache#8578)
  use shared pointer to help with high memory utilization (apache#8498)
  Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (apache#8621)
  check size of session, and free sessions the ATS way (apache#8330)
  free sessions when timeout (apache#8356)
  Fix 32bit build failure on Odroid Xu-4 (apache#8626)
  TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (apache#8617)
  SNIConfig (tunnel_route): Change the way we extract matched subgroups from the server name. (apache#8589)
  fix for collapsed forwarding ink_abort for CacheHitFresh fail (apache#8613)
  Do not turn off cache for internal requests (apache#8266)
  Rate Limit Plugin: Re-enable VConnection when SNI is empty (apache#8625)
  Removes hard dependency on having perl installed (apache#8611)
bryancall pushed a commit that referenced this pull request Jun 15, 2022
@brbzull0 @bryancall
SNIConfig (tunnel_route): Change the way we extract matched subgroups…
281416a 
… from the server name. (#8589)

This now uses the provided offsets from pcre_exec to read each matched group, this avoids
allocating memory for the subgroups. There was a memory leak here as well which is now eliminated.
This also changes the ActionItem::Context vector of strings to a vector of views to keep each matched group.

(cherry picked from commit 4f0c4f2)

 Conflicts:
	iocore/net/SSLSNIConfig.cc
@bryancall bryancall modified the milestones: 9.2.0, 9.1.3 Jun 23, 2022
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Jul 26, 2022
@randall @traeak
@bneradt @pbchou @brbzull0
[9.0.x] Various memory leak fixes (apache#351)
76d1d32 
* change MemArena::make test to remove memory leak (apache#8352)

(cherry picked from commit 2a6156f)

* Fix leaks in ConfigManager::configName (apache#8269)

This fixes an ASan reported leak of ConfigManager::configName. It used
to be strdup'd but not freed in the destructor. This simply changes it
to a std::string.

ASan also reported a leak in AddConfigFilesHere which is fixed with an
ats_free as well.

(cherry picked from commit ee820c7)

* Lua plugin memory leak on remap configuration reloads (apache#8764)

This fix adds reference counting for the Lua plugin remap instance handles. The reference counting
allows us to eliminate an existing memory leak of the instance handles. In addition, this means
that the old Lua memory allocated by LuaJIT may also be freed via LuaJIT garbage collection.

This fix also adds the '--ljgc' remap instance plugin parameter to the Lua plugin. This paramter
enables on-demand LuaJIT garbage collection while the remap instances are created and deleted.
This is useful when operating close to the LuaJIT memory limit, which is currently 2GB on Linux
using LuaJIT v2.1.0-beta3 from 2017.

Fixes apache#8728

(cherry picked from commit b6f83f1)

* Fixes leak of SNI config filename on load

(cherry picked from commit e99f33c)

* Fixes leak of ssl_ocsp_response_path_only on reload

(cherry picked from commit 18c5404)

* SNIConfig (tunnel_route): Change the way we extract matched subgroups from the server name. (apache#8589)

This now uses the provided offsets from pcre_exec to read each matched group, this avoids
allocating memory for the subgroups. There was a memory leak here as well which is now eliminated.
This also changes the ActionItem::Context vector of strings to a vector of views to keep each matched group.

(cherry picked from commit 4f0c4f2)

Conflicts:
    iocore/net/P_SSLSNI.h
    iocore/net/TLSSNISupport.cc

* Fixes leak in SNIAction name globbing (apache#8827)

pcre_compile allocated object is never pcre_free-ed

(cherry picked from commit efaf441)

Conflicts:
    iocore/net/P_SSLSNI.h

Co-authored-by: Brian Olsen <bnolsen@gmail.com>
Co-authored-by: Brian Neradt <brian.neradt@gmail.com>
Co-authored-by: pbchou <pbchou@labs.att.com>
Co-authored-by: Damian Meden <damian.meden@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bneradt bneradt bneradt left review comments

@bryancall bryancall Awaiting requested review from bryancall

+1 more reviewer

@SolidWallOfCode SolidWallOfCode SolidWallOfCode approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@brbzull0 brbzull0

Labels

Bug Leak

Projects

None yet

Milestone

9.1.3

Development

Successfully merging this pull request may close these issues.

SNIConfigParams::get seems to be leaking in some scenarios. (tunnel_route)

5 participants

@brbzull0 @zwoop @SolidWallOfCode @bneradt @bryancall