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

Prevent ClassCastException with invalid timestamp in clone_message() #192

Merged
merged 4 commits into from Jun 29, 2017

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Jun 28, 2017

If the "timestamp" field of a message is invalid, clone_message() will fail with a ClassCastException because Message#getTimestamp() tries to cast this field to a DateTime object.

With the changes in this commit, the type of the "timestamp" field will be checked before accessing it and if it is invalid (i. e. not a DateTime object), the current date and time will be used and the original content will be stored in the "_original_timestamp" field of the new message.

Fixes Graylog2/graylog2-server#3880

Prevent ClassCastException with invalid timestamp in clone_message()
If the "timestamp" field of a message is invalid, `clone_message()` will fail because
`Message#getTimestamp()` tries to cast this field to a `DateTime` object.

With the changes in this commit, the type of the "timestamp" field will be checked
before accessing it and if it is invalid (i. e. not a `DateTime` object), the current
date and time will be used and the original content will be stored in the "_original_timestamp"
field of the new message.

Fixes Graylog2/graylog2-server#3880

@joschi joschi added this to the 2.3.0 milestone Jun 28, 2017

clonedMessage = new Message(currentMessage.getMessage(), currentMessage.getSource(), currentMessage.getTimestamp());
clonedMessage.addFields(currentMessage.getFields());
} else {
final DateTime now = DateTime.now(DateTimeZone.UTC);

This comment has been minimized.

@bernd

bernd Jun 28, 2017

Member

How about trying to convert the value into a DateTime first?

This comment has been minimized.

@joschi

joschi Jun 29, 2017

Contributor

I thought about that but decided against it because of two reasons:

  • Performance. Trying to convert a string into a valid DateTime object is expensive.
  • Which date format and locale would you use? There's unfortunately no obviously correct one and trying multiple formats degrades performance.
// Message#addFields() overwrites the "timestamp" field.
clonedMessage.addField("timestamp", now);
clonedMessage.addField("_original_timestamp", tsField);

This comment has been minimized.

@bernd

bernd Jun 28, 2017

Member

I am not a fan of adding new fields as error indicators. The problem I see here is that we don't know what the type of the wrong timestamp field is and it might be different for every message. This can result in indexing errors because the _original_timestamp in ES might be of the wrong type.

As mentioned above, I think we should try to convert the timestamp into a DateTime and if that fails we could log an error or warning and use the current time. (without adding the _original_timestamp field)

This comment has been minimized.

@joschi

joschi Jun 29, 2017

Contributor

Valid points, but retaining the original field would at least give people the chance to investigate what went wrong.

To prevent indexing failures in Elasticsearch, I'd simply convert the original timestamp field into a string and log a warning (which I forgot to do in the current iteration).

joschi added some commits Jun 29, 2017

// Message#addFields() overwrites the "timestamp" field.
clonedMessage.addField("timestamp", now);
clonedMessage.addField("_original_timestamp", String.valueOf(tsField));

This comment has been minimized.

@bernd

bernd Jun 29, 2017

Member

Please check if the _original_timestamp is set in the message so we do not overwrite it if a user is using this field. Not sure what to do in that case, though.

@bernd

bernd approved these changes Jun 29, 2017

LGTM 👍

@bernd bernd merged commit 5f7b5a5 into master Jun 29, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 288 has succeeded
Details

@bernd bernd deleted the graylog2-server-issue-3880 branch Jun 29, 2017

@bernd bernd removed the ready-for-review label Jun 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment