search

FairRootGroup / FairRoot

C++ simulation, reconstruction and analysis framework for particle physics experiments
http://fairroot.gsi.de
Other
57 stars 96 forks source link

Fix FairRootManager::GetObject #1557

Closed YanzhaoW closed 2 months ago

YanzhaoW commented 2 months ago

Describe your proposal.

Fix the UB found in the issue #1556 related to FairRootManager::GetObject.

Cause of the issue

The branch address of the TTree must be fixed during the data reading. In other words, if an object of type T needs to be read from a tree, the value T** must be fixed and cannot be changed during the whole reading process. However, in the GetObject function, the variable newentry is a reference to the T* pointer stored in the std::vector<T*>. If the size of the vector grows beyond its capacity, the whole vector will be moved to another memory location, which means the T** value will be changed.

This makes sense as in my real practice from R3BRoot, I didn't get all zeros but rather direct segmentation fault.

Let newentry refer to the T* in the object of std::map<TString, TObject*> fixes the issue. std::map doesn't reallocate its entries during the runtime.

TODO

  1. AddMemoryBranch now returns TObject*&, which is ugly and asking for troubles in the future. Unfortunately, there isn't any better way to deal with this issue if we still use TTree raw API for the data reading. It must take a TObject** for calling SetBranchAddress.
  2. As I have suggested from #1556, TTreeReader is a much better choice as we don't need to deal with T** situation anymore.

Checklist:

coderabbitai[bot] commented 2 months ago
Walkthrough ## Walkthrough The `FairRootManager` class has undergone updates to enhance branch handling efficiency. The `ActivateBranch` method's logic has improved to better handle existing branches and the `AddMemoryBranch` now returns a reference to the added branch object, instead of void, to provide more control and information back to the calling context. ## Changes | Files | Change Summary | |---------------------------------------------------|----------------| | `fairroot/base/steer/FairRootManager.cxx`, `fairroot/base/steer/FairRootManager.h` | Updated `ActivateBranch` logic for better efficiency in handling branches and modified `AddMemoryBranch` to return a reference to `TObject*`. |
Recent review details **Configuration used: CodeRabbit UI** **Review profile: CHILL**
Commits Files that changed from the base of the PR and between 94c52ab9b2a2adba4b7c5ba77ff615a821bb5b85 and 3f1b6ab8523edd557719d40f923fd5f0a9cf0c2a.
Files selected for processing (1) * fairroot/base/steer/FairRootManager.h (2 hunks)
Files skipped from review as they are similar to previous changes (1) * fairroot/base/steer/FairRootManager.h
--- Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
ChristianTackeGSI commented 2 months ago

Thanks for analyzing this!

(We came to an alike result).

Our quick fix would be to replace the std::vector by a std::list. This is probably the fix that we would like to deploy for 19.0.1.

YanzhaoW commented 2 months ago

Hi, @ChristianTackeGSI

I don't quite understand the purpose of fObj2. When you call GetObject, if the branch has not been added and does't exist in the tree, you push nullptr to the vector. If the branch exists, you push the same pointer twice into the vector. On the other hand, you also have fMap for the book-keeping of the branches.

ChristianTackeGSI commented 2 months ago

Right, fObj2 is a bit strange!

We would like to have a minimal fix for 19.0.1 first. And the minimal one is to use std::list.

For 19.1 we can then slowly work on further refactoring and improving GetObject.

And: Thanks for the reference to TTreeReader!

YanzhaoW commented 2 months ago

Ok, Fair enough.

Should I close this PR and you create another one using std::list? Or I should just change it to std::list here?

ChristianTackeGSI commented 2 months ago

Ok, Fair enough.

Should I close this PR and you create another one using std::list? Or I should just change it to std::list here?

How ever you like. :-)

@dennisklein wanted to create a proper test first anyway.

YanzhaoW commented 2 months ago

@ChristianTackeGSI

Is this minimum change OK for you?

YanzhaoW commented 2 months ago

There is another issue about branch IO of FairRootManger:

There are two different, independent branch registries: fAnyBranchMap and fMap. These are used by different APIs of FairRootManager:

  1. fAnyBranchMap is used by RegisterAny and InitObjectAs.
  2. fMap is used by Register and GetObject.

Any branch registered in one way cannot be read by another way.

What's even more "interesting" is that the event header (online run) is always registered in fMap but not in fAnyBranchMap. So you may find that you can get a valid event header from GetObject but mysteriously nullptr from InitObjectAs.

@ChristianTackeGSI @dennisklein

ChristianTackeGSI commented 2 months ago

It looks like it will take a bit longer to create proper tests for this. So merging now, so that the fix is in place at least.

ChristianTackeGSI commented 2 months ago

BTW: I think, we need a ChangeLog entry for this?