Repository navigation
Fix crash when receive a BigDecimal inside _@timestamp - #67
Conversation
|
💚 CLA has been signed |
4c9b413 to
572852f
Compare
572852f to
1ececbd
Compare
|
@jsvd any chance that this PR gets merged? |
|
Hi folks, what if we just handled this special case: if the stripped key is "@timestamp", convert the value to LogStash::Timestamp, and if that fails, convert it to BigDecimal? For example: def strip_leading_underscore(event)
# Map all '_foo' fields to simply 'foo'
event.to_hash.keys.each do |key|
next unless key[0,1] == "_"
new_key = key[1..-1]
value = event.get(key)
if new_key == LogStash::Event::TIMESTAMP
event.set(new_key, coerce_timestamp_carefully(value))
else
event.set(new_key, value)
end
event.remove(key)
end
end
def coerce_timestamp_carefully(value)
LogStash::Timestamp.at(value)
rescue TypeError => e
# maybe it's a BigDecimal?
coerced_value = BigDecimal.new(value)
LogStash::Timestamp.at(coerced_value)
end |
|
I think could be the right path but also the coercion of the coerced_value = BigDecimal.new(value)if |
| strip_leading_underscore(event) if @strip_leading_underscore | ||
| rescue => e | ||
| if !stop? | ||
| @logger.error("Caught exception while striping leading underscores", :exception => e) |
There was a problem hiding this comment.
I think that here we should be more consistent with the TCP counter part, so I would output warn log in form:
@logger.warn("Gelf (udp): striping leading underscores failed.", :exception => ex, :backtrace => ex.backtrace
karenzone
left a comment
There was a problem hiding this comment.
Left a wording suggestion for consideration for the changelog entry. This is the text that will be picked up and added to our release notes.
There was a problem hiding this comment.
While it would be good to avoid crashing the input/pipeline, I would prefer an approach that doesn't drop the offending events, perhaps by making the strip_leading_underscores more granular.
The following, for example, will emit the event with fields that could not be renamed in-tact, along with an actionable event tag.
def strip_leading_underscore(event)
event.to_hash.keys.each do |key|
move_field(event, key, key.slice(1..-1)) if key.starts_with?('_')
end
end
def move_field(event, source_field, destination_field)
value = event.get(source_field)
value = coerce_timestamp_carefully(value) if new_field == LogStash::Event::TIMESTAMP
event.set(destination_field, value)
event.remove(source_field)
rescue => e
logger.warn("Failed to move field `#{source_field}` to `#{destination_field}`: #{e.message}")
event.tag("_gelf_move_field_failure", :exception => e.message)
end|
|
||
| def coerce_timestamp_carefully(value) | ||
| LogStash::Timestamp.at(value) | ||
| rescue TypeError => e |
There was a problem hiding this comment.
Can we possibly be less reactive, routing to BigDecimal if we have a reasonable expectation that doing so will succeed?
There was a problem hiding this comment.
So, if I understand correctly what you mean, instead of reaching a TypeError raised when value is a String and then fallback to BigDecimal, convert it directly to BigDecimal like:
def coerce_timestamp_carefully(value)
coerced_value = BigDecimal.new(value)
LogStash::Timestamp.at(coerced_value.to_i, coerced_value.frac * 1000000)
endThere was a problem hiding this comment.
Is it possible to pre-classify inputs that will fail when given to LogStash::Timestamp::at, but will succeed at being parsed into a BigDecimal, without being reactive to exceptions?
What inputs are we actually receiving that are causing the crash (that is now mitigated by the safe field-by-field moving)?
For example, we know that big decimal numbers are often encoded as strings to avoid the loss of precision associated with encoding them as floats/doubles, and that strings will be rejected by LogStash::Timestamp::at. We could use this information to wrap epoch-looking strings in BigDecimal proactively, which would result in defining our coerce_timestamp_carefully as something like:
def coerce_timestamp_carefully(value)
value = BigDecimal.new(value) if value.kind_of?(String) && value.match(/\A[0-9]+(\.[0-9]+)?\z/)
LogStash::Timestamp.at(value)
endNote that the reason we had to split a BigDecimal up into its whole-seconds and fractions was due to a very old bug in JRuby that was mitigated in Logstash as early as 5.0.0.
There was a problem hiding this comment.
I updated with the suggestion, switched the regexp to accept also numbers in exponential notation like 0.123456e3
adding the part:
0\.[0-9]+e[0-9]
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
|
@yaauie I like the idea to tag the event instead of dropping with log. |
9ef80ad to
5e0b4a1
Compare
|
|
||
| def coerce_timestamp_carefully(value) | ||
| LogStash::Timestamp.at(value) | ||
| rescue TypeError => e |
There was a problem hiding this comment.
Is it possible to pre-classify inputs that will fail when given to LogStash::Timestamp::at, but will succeed at being parsed into a BigDecimal, without being reactive to exceptions?
What inputs are we actually receiving that are causing the crash (that is now mitigated by the safe field-by-field moving)?
For example, we know that big decimal numbers are often encoded as strings to avoid the loss of precision associated with encoding them as floats/doubles, and that strings will be rejected by LogStash::Timestamp::at. We could use this information to wrap epoch-looking strings in BigDecimal proactively, which would result in defining our coerce_timestamp_carefully as something like:
def coerce_timestamp_carefully(value)
value = BigDecimal.new(value) if value.kind_of?(String) && value.match(/\A[0-9]+(\.[0-9]+)?\z/)
LogStash::Timestamp.at(value)
endNote that the reason we had to split a BigDecimal up into its whole-seconds and fractions was due to a very old bug in JRuby that was mitigated in Logstash as early as 5.0.0.
|
|
||
| before(:each) do | ||
| subject.register | ||
| Thread.new { subject.run(queue) } |
There was a problem hiding this comment.
I'm not seeing our input get shut down anywhere.
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
ef0a46f to
55c6d68
Compare
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
…te method used from in before block
…type to checking if it can safely be parsed
yaauie
left a comment
There was a problem hiding this comment.
This PR has become several distinct things:
- prevent a crash when renaming
_@timestampto@timestamp: this is mitigated by our field-by-field rename, where we catch exceptions and emit the event with a tag - allow a source
_@timestampencoded as aStringto be handled as aBigDecimal: this is mostly-handled, although I have left a comment about avoiding adding support for e-notation until we need it. - prevent
Float-encoded timestamps from injecting false precision: this is not addressed in the implementation at all, but instead this PR changes our specification, removing the spec for how we should behave withFloats (that we do receive in practice) and substituting for it a spec for how we should behave withRationals (which we do not receive in practice). I've left notes in how we can address this.
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
72c79e2 to
590b30e
Compare
Co-authored-by: Ry Biesemeyer <yaauie@users.noreply.github.com>
|
This fix very likely breaks logstash: #70 |
The UDP server crashes when receives a GELF message like that
{"full_message": "my message", "_@timestamp": 12345678889}I wrote a test case to show that behavior.
Thanks
Release notes
Safe parsing of
_@timestampto avoid crashing the plugin and LogstashWhat does this PR do?
This PR safely cast the values of
_@timestampfield in GELF and in case of error skip the event, avoiding Logstash crashWhy is it important/What is the impact to the user?
Permit to handle correctly the numerical values in
_@timestampwithout stopping Logstash process in case of error.Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
Gemfile:bin/logstash-plugin install --no-verifypipeline.confas:Related issues
Use cases
A user send GELF messages in the form:
{ "version": "1.1", "host": "example.org", "short_message": "A short message", "level": 5, "_@timestamp": "foo" }and the parsing of such payload can't crash Logstash.
Logs