Skip to content
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

Fixed GAUGE32 integer overflow in base_client.rb #65

Merged
merged 4 commits into from
Aug 1, 2019
Merged

Fixed GAUGE32 integer overflow in base_client.rb #65

merged 4 commits into from
Aug 1, 2019

Conversation

pprugger
Copy link
Contributor

As discussed here:
#64

Best regards

@pprugger
Copy link
Contributor Author

I changed the dist to trusty, only way i found to get it building with oraclejdk8 again.

@colinsurprenant
Copy link
Contributor

Thanks a lot @pprugger !

  • Could you remove the .travis.yml change, this is really non related to this PR; I will push a new PR shortly which will solve this, you can then rebase to pick it up and tests should pass.
  • Would you mind adding a test/spec for this? you could add it into spec/inputs/snmp/base_client_spec.rb in the coercion section. Just add a test with a value that overflows without the patch and works with it. Let me know if you need help for that!

@colinsurprenant
Copy link
Contributor

FYI build fixed in #66

@pprugger pprugger changed the base branch from master to SNMPv3_support July 31, 2019 18:59
@pprugger pprugger changed the base branch from SNMPv3_support to master July 31, 2019 18:59
@pprugger
Copy link
Contributor Author

I hope it's not too much of chaos now, had some problems with rebaseing.
To be honest i have no clue how to add the test.
I would test with uint_max so 4294967295, which should be 4294967295 if it works, otherwise it should be -1. (if the overflow occurs)

@colinsurprenant
Copy link
Contributor

@pprugger thanks for the dist revert. I'll send an example spec which will fail locally for me without patch and works for you, you can then include it in your PR!

@colinsurprenant
Copy link
Contributor

colinsurprenant commented Aug 1, 2019

@pprugger here, you can add these 2 tests in `spec/inputs/snmp/base_client_spec.rb" in the "coercion" context:

      it "should handle max unsigned 32 bits integer GAUGE32" do
        MAX_UNSIGNED_INT_32 = 4294967295
        v = Gauge32.new(MAX_UNSIGNED_INT_32)
        expect(subject.coerce(v)).to eq(MAX_UNSIGNED_INT_32)
      end

      it "should handle max signed 32 bits integer INTEGER32" do
        MAX_SIGNED_INT_32 = 2147483647
        v = Integer32.new(MAX_SIGNED_INT_32)
        expect(subject.coerce(v)).to eq(MAX_SIGNED_INT_32)
      end

The first one for GAUGE32 does not pass for me and returns -1 as you noted and should pass for you, the second for INTEGER32 does pass which is normal; I just added it for the sake of completeness.

@pprugger
Copy link
Contributor Author

pprugger commented Aug 1, 2019

Did you change something else? It seems the names Gauge32 and Integer32 are not known in my context.

@pprugger
Copy link
Contributor Author

pprugger commented Aug 1, 2019

I added the java imports. Works now and can be merged if you want.

@@ -32,6 +34,18 @@ def coerce(*args)
v = AbstractVariable.create_from_syntax(SMIConstants::EXCEPTION_END_OF_MIB_VIEW )
expect(subject).to receive(:logger).exactly(1).times.and_call_original
expect(subject.coerce(v)).to eq("error: unknown variable syntax 130, EndOfMibView")
end

it "should handle max unsigned 32 bits integer GAUGE32" do
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetic: the identation is off

@colinsurprenant
Copy link
Contributor

@pprugger great stuff!! yeah, forgot to mention the java imports but you figured it out 👍
All good, minus a small cosmetic nitpick!

Co-Authored-By: Colin Surprenant <colin.surprenant@gmail.com>
@pprugger
Copy link
Contributor Author

pprugger commented Aug 1, 2019

Fixed!

Will you do a new release after the merge?

@colinsurprenant
Copy link
Contributor

Thanks a lot again for your contribution @pprugger. Feel free to submit a PR to add your information into the CONTRIBUTORS file.
I will merge this, bump the version and then publish it shortly.

@colinsurprenant colinsurprenant merged commit b844019 into logstash-plugins:master Aug 1, 2019
@colinsurprenant
Copy link
Contributor

v1.2.1 published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants