Ignore inherited methods in BinData field-name guard (#261)

lib/ruby_smb.rb

@@ -1,4 +1,5 @@
 require 'bindata'
+require 'ruby_smb/compatibility'
 require 'net/ntlm'
 require 'net/ntlm/client'
 require 'openssl'

lib/ruby_smb/compatibility.rb

@@ -0,0 +1,37 @@
+module RubySMB
+  # Compatibility shims for the gems RubySMB builds on.
+  module Compatibility
+  end
+end
+
+# BinData rejects a field name when a method of that name is already defined on
+# the struct class. The check in BinData::DSLMixin::DSLFieldValidator uses
+# Module#method_defined? without the second argument, so it also matches methods
+# inherited from ancestors such as Object, Kernel and Hash.
+#
+# That is too broad in practice. When a test suite loads a mocking library that
+# adds helper methods to Object (rspec-mocks defines #stub on BasicObject when
+# `config.mock_with :rspec` is used, mocha defines #stubs, and so on) before
+# ruby_smb is required, perfectly valid BinData definitions start raising
+#   SyntaxError: field 'stub' shadows an existing method
+# even though no struct field actually shadows a method that the struct itself
+# relies on. See https://github.com/rapid7/ruby_smb/issues/261.
+#
+# Field accessors are installed per instance with define_singleton_method, so an
+# inherited method named like a field never gets in the way at runtime; only a
+# method defined directly on the struct class is a genuine collision. Passing
+# `false` to method_defined? restricts the check to methods defined directly on
+# the class, which is the behaviour the guard is actually meant to enforce.
+# Names that clash with Hash/BinData internals remain covered by the separate
+# BinData::Struct::RESERVED list, so this does not loosen that protection.
+if defined?(BinData::DSLMixin::DSLFieldValidator)
+  module BinData
+    module DSLMixin
+      class DSLFieldValidator
+        def name_shadows_method?(name)
+          @the_class.method_defined?(name, false)
+        end
+      end
+    end
+  end
+end

spec/lib/ruby_smb/compatibility_spec.rb

@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+RSpec.describe 'BinData field name compatibility (issue #261)' do
+  # Regression coverage for
+  # https://github.com/rapid7/ruby_smb/issues/261
+  #
+  # Loading a mocking library (rspec-mocks via `config.mock_with :rspec`) adds a
+  # #stub helper to Object before ruby_smb is required. BinData's field-name
+  # guard used method_defined? without restricting it to the struct class, so it
+  # picked up that inherited method and refused otherwise-valid field names such
+  # as :stub (RubySMB::Dcerpc::Request actually has a :stub field).
+
+  context 'when an inherited method shares a name with a BinData field' do
+    before do
+      # Simulate what rspec-mocks/mocha do: define a helper method on Object so
+      # that every class, including a fresh BinData struct, inherits it.
+      Object.send(:define_method, :inherited_helper_261) { :from_object }
+    end
+
+    after do
+      Object.send(:remove_method, :inherited_helper_261)
+    end
+
+    it 'allows the field to be defined without raising' do
+      expect(Object.method_defined?(:inherited_helper_261)).to be true
+
+      expect do
+        Class.new(BinData::Record) do
+          uint8 :inherited_helper_261
+        end
+      end.not_to raise_error
+    end
+  end
+
+  context 'when a field name shadows a method defined directly on the struct' do
+    it 'still raises, so the genuine guard is preserved' do
+      expect do
+        Class.new(BinData::Record) do
+          def direct_method_261; end
+          uint8 :direct_method_261
+        end
+      end.to raise_error(/shadows an existing method/)
+    end
+  end
+
+  context 'when a field name collides with a BinData reserved name' do
+    it 'still raises, so the RESERVED guard is preserved' do
+      expect do
+        Class.new(BinData::Record) do
+          uint8 :keys # Hash#keys, part of BinData::Struct::RESERVED
+        end
+      end.to raise_error(/reserved name/)
+    end
+  end
+end