logstash-plugins / logstash-input-snmp

Apache License 2.0
17 stars 22 forks source link

Fixed GAUGE32 integer overflow in base_client.rb #65

Closed pprugger closed 5 years ago

pprugger commented 5 years ago

As discussed here: https://github.com/logstash-plugins/logstash-input-snmp/issues/64

Best regards

pprugger commented 5 years ago

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

colinsurprenant commented 5 years ago

Thanks a lot @pprugger !

colinsurprenant commented 5 years ago

FYI build fixed in #66

pprugger commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

pprugger commented 5 years ago

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

colinsurprenant commented 5 years ago

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

pprugger commented 5 years ago

Fixed!

Will you do a new release after the merge?

colinsurprenant commented 5 years ago

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 commented 5 years ago

v1.2.1 published.