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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore types unrelated to the configuration in the summarize plugin #2258

Merged
merged 5 commits into from May 3, 2022

Conversation

dominiklohmann
Copy link
Member

This fixes a segfault in the summarize transform step when its configuration caused all fields to be dropped from the table.

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

Run this on our testbed and check that we encountered that same issue there. The unit test this adds crashes without the additional changes to the plugin and to libvast.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label May 3, 2022
@dominiklohmann dominiklohmann requested review from lava and tobim May 3, 2022 13:19
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

I'm currently deploying this to the testbed; but I'm not sure about including the complete rewrite of record_type::transform() into the release. Would it make sense to split this and get just the bugfix in?

libvast/src/type.cpp Show resolved Hide resolved
@dominiklohmann
Copy link
Member Author

dominiklohmann commented May 3, 2022

Would it make sense to split this and get just the bugfix in?

This actually fixed another bug for record types like this:

type foo = record {
  a: record {
    b: record {
      c: count,
    },
  },
  d: count,
}

Where before this change when removing c we returned the invalid type

type foo = record {
  a: record {
  },
  d: count,
}

rather than the correct:

type foo = record {
  d: count,
}

So no, I think we actually need to get that fix in as well. Users were theoretically able to trigger it with carefully crafted data.

Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Tbh, I didn't 100% read through the changed logic in record_type::transform, but approving anyways since unit test coverage seems good and it's definitely fixing the segfault that used to appear in the testbed.

EDIT: The second bugfix still needs a changelog entry, though.

@dominiklohmann
Copy link
Member Author

The second bugfix still needs a changelog entry, though.

Good point. I've added a second one.

Thanks for testing on the testbed @lava.

@dominiklohmann dominiklohmann merged commit 2ad635c into master May 3, 2022
@dominiklohmann dominiklohmann deleted the topic/summarize-crash-unrelated-types branch May 3, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
2 participants