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

Check if the data passed to the frame model is not a nullptr #578

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

  • Check if the data passed to the frame model is not a nullptr before doing anything with it.

ENDRELEASENOTES

It looks like a good idea to check if the pointer is null or not regardless. This makes opening corrupted or wrong files throw

terminate called after throwing an instance of 'std::invalid_argument'
  what():  Building a Frame failed; if you are reading from a file it may be corrupted

instead of crashing.

m_parameters(std::move(m_data->getParameters())) {
m_mapMtx(std::make_unique<std::mutex>()), m_dataMtx(std::make_unique<std::mutex>()) {
if (!data) {
throw std::invalid_argument("Building a Frame failed; if you are reading from a file it may be corrupted");
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
throw std::invalid_argument("Building a Frame failed; if you are reading from a file it may be corrupted");
throw std::runtime_error("Building a Frame failed; if you are reading from a file it may be corrupted");

Or this, initially I was going to have a message about FrameModel but no one will know what that is so it would be a bit useless.

@tmadlener
Copy link
Collaborator

In principle you could also be reading past the available number of entries, as I originally envisioned the user to check whether the uniqe_ptr<FramaData> they retrieve from the reader is a valid pointer. However, I see it's not documented, so we should also add something to the docstring of the constructor here:

podio/include/podio/Frame.h

Lines 149 to 155 in b94cc63

/// Frame constructor from (almost) arbitrary raw data.
///
/// @tparam FrameDataT Arbitrary data container that provides access to the
/// collection buffers as well as the metadata, when
/// requested by the Frame.
template <typename FrameDataT>
Frame(std::unique_ptr<FrameDataT>);

I would then make the exception message mainly about receiving a nullptr for the FrameData.

@jmcarcell
Copy link
Member Author

Changed. I prefer a more verbose error message because it's a public one and easy to get either by reading more entries than there are or by unhealthy files (I don't remember exactly how but I was able to create files that reach until there but then the data is nullptr) and with the exception you don't get any information where it happened while when it's crashing with debug symbols you get some information, so it should try to not be worse than the crash.

include/podio/Frame.h Outdated Show resolved Hide resolved
@tmadlener
Copy link
Collaborator

Not entirely sure why the dev4 workflows are failing now. The issue looks like root-project/root#15117 but this should have been fixed. Merging this for now.

@tmadlener tmadlener merged commit eb24243 into AIDASoft:master Apr 11, 2024
16 of 18 checks passed
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