Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

y9v · 2024-10-02T14:56:51Z

Ruby 3.3.0 and above has a memory leak when using libddwaf-rb with ffi gem older than 1.16.0.

We want to avoid forcing our customers to update ffi gem by setting the minimum version in libddwaf-rb, since prebuilt ffi extension is incompatible with rubygems version supplied with ruby 2.7 and below.

The memory leak happens because ffi gem used untyped DATA API prior to this commit (which is part of 1.16.0 release):
ffi/ffi@ca2744a

Motivation:
Memory leak that occurs when enabling appsec for applications running on ruby 3.3.x and an old version of ffi gem locked in Gemfile.lock (older than 1.16.0).

codecov-commenter · 2024-10-02T15:14:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (1f1afc1) to head (03f3bc9).
Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3969      +/-   ##
==========================================
- Coverage   97.87%   97.86%   -0.01%     
==========================================
  Files        1313     1313              
  Lines       78352    78366      +14     
  Branches     3886     3888       +2     
==========================================
+ Hits        76684    76690       +6     
- Misses       1668     1676       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-10-02T15:30:01Z

Benchmarks

Benchmark execution time: 2024-10-07 09:43:42

Comparing candidate commit 03f3bc9 in PR branch disable-appsec-when-using-ruby-3-3-with-old-ffi with baseline commit 1f1afc1 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:profiler - profiler gc

🟥 throughput [-24005.450op/s; -23740.872op/s] or [-6.631%; -6.558%]

Strech · 2024-10-02T15:39:07Z

lib/datadog/appsec/component.rb

+ 'Appsec is not supported in Ruby versions above 3.3.0 when using `ffi` versions older than 1.16.0, ' \
+ 'and will be forcibly disabled. This is due to a memory leak in `ffi` versions lower than 1.16.0.' \
+ 'Please upgrade your `ffi` version to 1.16.0 or higher.'


WDYT if we join the reason to the main sentence?

Suggested change

'Appsec is not supported in Ruby versions above 3.3.0 when using `ffi` versions older than 1.16.0, ' \

'and will be forcibly disabled. This is due to a memory leak in `ffi` versions lower than 1.16.0.' \

'Please upgrade your `ffi` version to 1.16.0 or higher.'

'AppSec is not supported in Ruby versions above 3.3.0 when using `ffi` versions older than 1.16.0, ' \

'and will be forcibly disabled due to a memory leak in `ffi`. ' \

'Please upgrade your `ffi` version to 1.16.0 or higher.'

yes, this is definitely better, thanks!

fixed: 51a803f

Strech · 2024-10-02T15:50:22Z

lib/datadog/appsec/component.rb

@@ -28,6 +29,22 @@ def build_appsec_component(settings, telemetry:)

 private

+ def enable_appsec?(settings)
+ return false unless settings.respond_to?(:appsec) && settings.appsec.enabled


Do you think inverting unless && makes sense here? I find it hard to understand (negate logic with addition to &&)

Suggested change

return false unless settings.respond_to?(:appsec) && settings.appsec.enabled

return false if !settings.respond_to?(:appsec) || !settings.appsec.enabled

I agree, this will improve readability

For the record I think the unless & && version was easier to follow.

Strech · 2024-10-02T15:53:56Z

lib/datadog/appsec/component.rb

@@ -12,6 +12,7 @@ class Component
 class << self
 def build_appsec_component(settings, telemetry:)
 return unless settings.respond_to?(:appsec) && settings.appsec.enabled
+ return unless enable_appsec?(settings)


I think the method with ? is doing more than 1 thing. WDYT on splitting it on something like this

Suggested change

return unless enable_appsec?(settings)

return unless appsec_enabled?(settings)

return if incompatible_ffi_version?

I removed appsec_enabled? method and added incompatible_ffi_version? here: 51a803f

Ruby 3.3 and newer has a memory leak when using libddwaf with `ffi` gem older than 1.16.0. We want to avoid forcing our customers to update `ffi` gem, by setting the minimum version in `libddwaf-rb`, since prebuilt `ffi` extension is incompatible with rubygems version supplied with ruby 2.7 and below.

p-datadog · 2024-10-03T14:51:33Z

lib/datadog/appsec/component.rb

@@ -28,6 +29,19 @@ def build_appsec_component(settings, telemetry:)

 private

+ def incompatible_ffi_version?
+ ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version
+ return false unless RUBY_VERSION >= '3.3.0' && ffi_version < '1.16.0'


I think you would need to parse the FFI version correctly, string comparison works for Ruby versions only because their components are kept to single digits.

I would also check against 3.3 not 3.3.0.

Added Gem::Version for all version comparisons here: 03f3bc9

p-datadog · 2024-10-03T14:51:48Z

spec/datadog/appsec/component_spec.rb

+ context 'when using old ffi version with Ruby 3.3.x' do
+ before do
+ stub_const('RUBY_VERSION', '3.3.0')
+ allow(Gem).to receive(:loaded_specs).and_return('ffi' => double(version: Gem::Version.new('1.15.4')))


If you use 1.9.0 for the FFI version the test will fail right?

no, it is still green:

Gem::Version.new('1.9.0') < '1.16.0' => true

I think this is because we are comparing Gem::Version with a String (not sure this will work correctly in older ruby versions though)

We can dismantle version on major, minor and test piece-by-piece (just in case)

Yep, https://github.com/rubygems/rubygems/blob/master/lib/rubygems/version.rb#L358, added in rubygems/rubygems@7e0dbb7 2 years ago, which sounds like this functionality is probably not going to exist in Ruby 2.5?

rails/rails#47480 conveniently says that the threshold for this feature is Ruby 3.1.

Strech · 2024-10-07T09:23:19Z

lib/datadog/appsec/component.rb

+ ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version
+ return false unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.3') &&
+ ffi_version < Gem::Version.new('1.16.0')


Not sure how it could be relevant and don't want to over complicate it, but if we check that Gem.loaded_specs['ffi'] not nil, what if it's nil?

Then entire ffi_version is nil, right? And it could fail next check

❯❯❯ irb irb(main):001> ffi_version = Gem.loaded_specs['ffi'] && Gem.loaded_specs['ffi'].version => nil irb(main):002* Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.3') && irb(main):003> ffi_version < Gem::Version.new('1.16.0') (irb):3:in `<main>': undefined method `<' for nil (NoMethodError) ffi_version < Gem::Version.new('1.16.0') ^ from <internal:kernel>:187:in `loop'``` But I made it up, maybe it never happen

I will still add a guard clause for this, just in case

although I'm not even sure what to do in this case - we don't want to create a processor, and we probably don't want to use the same warning message. Do we want to silently return true in such case?

I think it would be fine to just not warn in this situation and carry on to subsequent code.

I think we can consider it to be "not there" ...

Sorry, I merged to early. Here is a follow-up PR: #3978

Strech

I think it's good, maybe with a few adjustments (if needed)

y9v requested a review from a team October 2, 2024 14:56

y9v self-assigned this Oct 2, 2024

y9v requested a review from a team as a code owner October 2, 2024 14:56

Strech reviewed Oct 2, 2024

View reviewed changes

y9v force-pushed the disable-appsec-when-using-ruby-3-3-with-old-ffi branch from 7c15366 to 51a803f Compare October 2, 2024 16:12

y9v requested a review from Strech October 2, 2024 16:14

Add guard clause to AppSec::Component#incompatible_ffi_version?

b4f6641

p-datadog reviewed Oct 3, 2024

View reviewed changes

Use Gem::Version for comparing Ruby and ffi versions

03f3bc9

y9v requested a review from p-datadog October 7, 2024 09:10

Strech reviewed Oct 7, 2024

View reviewed changes

Strech approved these changes Oct 7, 2024

View reviewed changes

p-datadog approved these changes Oct 7, 2024

View reviewed changes

y9v merged commit 12b5808 into master Oct 7, 2024
248 checks passed

y9v deleted the disable-appsec-when-using-ruby-3-3-with-old-ffi branch October 7, 2024 12:50

github-actions bot added this to the 2.4.0 milestone Oct 7, 2024

This was referenced Oct 7, 2024

Fix AppSec::Component building when ffi gem is not loaded #3978

Merged

Bump to version 2.4.0 #3994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

y9v commented Oct 2, 2024 •

edited

Loading

codecov-commenter commented Oct 2, 2024 •

edited

Loading

pr-commenter bot commented Oct 2, 2024 •

edited

Loading

Strech Oct 2, 2024

y9v Oct 2, 2024

y9v Oct 2, 2024 •

edited

Loading

Strech Oct 2, 2024

y9v Oct 2, 2024

p-datadog Oct 3, 2024

Strech Oct 2, 2024

y9v Oct 2, 2024

p-datadog Oct 3, 2024

y9v Oct 7, 2024

p-datadog Oct 3, 2024

y9v Oct 7, 2024

Strech Oct 7, 2024

p-datadog Oct 7, 2024

p-datadog Oct 7, 2024

Strech Oct 7, 2024 •

edited

Loading

y9v Oct 7, 2024

y9v Oct 7, 2024

p-datadog Oct 7, 2024

Strech Oct 7, 2024

y9v Oct 7, 2024

Strech left a comment

	return false unless settings.respond_to?(:appsec) && settings.appsec.enabled
	return false if !settings.respond_to?(:appsec) \|\| !settings.appsec.enabled

	return unless enable_appsec?(settings)
	return unless appsec_enabled?(settings)
	return if incompatible_ffi_version?

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Add a force disable of appsec when using Ruby >= 3.3 with old ffi #3969

Conversation

y9v commented Oct 2, 2024 • edited Loading

codecov-commenter commented Oct 2, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Oct 2, 2024 • edited Loading

Benchmarks

scenario:profiler - profiler gc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y9v Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

y9v commented Oct 2, 2024 •

edited

Loading

codecov-commenter commented Oct 2, 2024 •

edited

Loading

pr-commenter bot commented Oct 2, 2024 •

edited

Loading

y9v Oct 2, 2024 •

edited

Loading

Strech Oct 7, 2024 •

edited

Loading