Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Added a copy constructor and an overloaded assignment operator to FileStorage#17458

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
EricFlorin wants to merge5 commits intoopencv:masterfromEricFlorin:FileStorage_Assignment_Operator
Closed

Added a copy constructor and an overloaded assignment operator to FileStorage#17458

EricFlorin wants to merge5 commits intoopencv:masterfromEricFlorin:FileStorage_Assignment_Operator

Conversation

@EricFlorin
Copy link

tl:dr ->Fixes#17412

Hello everyone,

This is going to be my first pull request, so I'm sorry if I missed something in my submission. I appreciate any feedback as this is my first time contributing to an open source project.

Quick summary of issue#17412: acv::FileStorage object that was assigned by anothercv::FileStorage object was unusable as it was causing segfaults every time an access occurs.

I look over the issue replication code provided in#17412, and I noticed thatfs was being assigned to a temporarycv::FileStorage object. I went through the execution path of the assignment using my debugger, and noticed thatfs was not being assigned a deep copy of the temporarycv::FileStorage object.

Sincefs was not given a deep copy of the temporarycv::FileStorage object, it loses access to the file reference in the temporarycv::FileStorage object after the temporary object is destroyed upon assignment completion.

I fixed the issue by adding a copy constructor and an overloaded assignment operator tocv::FileStorage that creates deep copies ofcv::FileStorage objects. I feel that there is probably a better way of fixing this issue, but this is what I came up with for now.

Thank you for reading!
Eric

Pull Request Readiness Checklist

See details athttps://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under OpenCV (BSD) License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@asmorkalovasmorkalov requested review fromalalek andvpisarev and removed request forvpisarevJune 4, 2020 13:20
@asmorkalov
Copy link
Contributor

@EricFlorin Thanks for the patch! Please take a look on CI status, buildbot reports compiler warnings:

/build/precommit_linux64/opencv/modules/core/src/persistence.cpp:1792:83: warning: declaration of 'encoding' shadows a member of 'cv::FileStorage' [-Wshadow]/build/precommit_linux64/opencv/modules/core/src/persistence.cpp:1862:81: warning: declaration of 'encoding' shadows a member of 'cv::FileStorage' [-Wshadow]

@asmorkalovasmorkalov added category: core bug pr: needs testNew functionality requires minimal tests set labelsJun 4, 2020
@asmorkalov
Copy link
Contributor

asmorkalov commentedJun 4, 2020•
edited
Loading

Could you also add simple test for the new functions to ensure stability in future.

@EricFlorin
Copy link
Author

Hey@asmorkalov! I wanted to say thanks for the review!

I also want to ask about what kinds of tests you may be looking for when it comes to the copy and assignment operators forcv::FileStorage? I was thinking of using a sample XML/YAML/JSON file and just doing different types of assignment operations with them, but I am not sure if that will be enough.

@asmorkalov
Copy link
Contributor

XML/JSON/YAML formats handling is done by differentcv::FileStorage backends and your fixes work in the same way for all formats. You can test only one of them. The reproducer code in the ticket is enough, just replace final cout by assert.

@asmorkalov
Copy link
Contributor

@EricFlorin Please take a look on CI status:https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/26297/steps/compile%20release/logs/warnings%20%282%29. CI bot reports build warnings:

/build/precommit_linux64/opencv/modules/core/src/persistence.cpp:1792:83: warning: declaration of 'encoding' shadows a member of 'cv::FileStorage' [-Wshadow]/build/precommit_linux64/opencv/modules/core/src/persistence.cpp:1862:81: warning: declaration of 'encoding' shadows a member of 'cv::FileStorage' [-Wshadow]

…d added new checks to copy constructor and assignment operator
@EricFlorin
Copy link
Author

Hey@asmorkalov,

I got around to fixing the warnings on my side (we will see what the CI bot says). I also modified the reproducer code to act as a test, as well as create my own test script to test different types of cases. The scripts and support files arein this repository that I created.

  • SimpleTest.cpp is the modified reproducer code
  • ExtensiveNewFunctionalityTests.cpp is my custom test script

If I am missing a case in my custom test script, please let me know and I will add it!

asmorkalov reacted with thumbs up emoji

Copy link
Member

@alalekalalek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

Please avoid implementing of "deep-copy" logic forFileStorage.

Could you please add simple tests (including from the original issue) to test_io.cpp?

/** @brief Copy constructor for FileStorage
Copy constructor that creates deep copies of passed in cv::FileStorage objects. Based off the full
constructor.
Copy link
Member

Choose a reason for hiding this comment

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

Almost all non-trivial OpenCV classes are designed as a smart pointers (this behavior is aligned with other language bindings):

  • assignment operation means increasing object lifetime (through refcounter)
  • optional deep-copy is through.clone() operations

Deep copy is over-complicated thing forFileStorage (especially in case of write mode), so let avoid that completely.

vpisarev reacted with thumbs up emoji
@asmorkalov
Copy link
Contributor

@EricFlorin Do you have chance to implement proposal from@alalek

@EricFlorin
Copy link
Author

Hey@asmorkalov and@alalek,

Sorry for the long silence this week as I was having final exams.

I did look at the proposal made by@alalek and I did roll back the deep-copy implementation on my end. However, I am stuck on coming up with a solution using smart pointers.

Here's what I found so far using the reproducer code given in#17412 :

  1. The temporarycv::FileStorage object is created. When the temporarycv::FileStorage object is created,p (a shared pointer) is initialize a newcv::FileStorage::Impl object. Thecv::FileStorage::Impl object contains an vector ofcv::FileNode calledroots. Eachcv::FileNode inrootscontains a constantcv::FileStorage pointer, whose address is that of the temporary cv::FileStorage object.
  2. Thefscv::Filestorage object is assigned the temporarycv::FileStorage object. The assignment causesfs.p to be assigned to the temporarycv::FileStorage object'sp.
  3. After assignment is complete, the temporarycv::FileStorage object is destroyed. The destruction does not affectfs.p sincep is a shared pointer. However, since thep in the temporarycv:FileStorage object is emptied via the destructor, anycv::FileNode with a pointer to the address of the temporarycv::FileStorage object will have access to an emptyp.
  4. From there,fs["string"] >> s is called. The segfault occurs afterres = p->roots[i][key]; is called inFileNode FileStorage::operator [](const std::string& key) const. In the case of the reproducer code, this statement is uses thecv::FileNode located atroots[0]. The constantcv::FileStorage pointer in thiscv::FileNode points to acv::FileStorage object whosep is empty. The "contents" thatp contained was attempted to be accessed inconst uchar* FileNode::ptr() const afterIsMap() was called inFileNode FileNode::operator[](const std::string& nodename) const; which failed sincep is empty.

From there, I am not sure what should I do. I was thinking of having theconst FileStorage* incv::FileStorage be acv::Ptr<FileStorage>, but I feel that it will not fix the problem as thep in the temporarycv::FileStorage object will still be deleted.

Any thoughts?

@vpisarev
Copy link
Contributor

@EricFlorin, thank you for the detailed research. I fixed the problem in#17841. The solution was simple – just remove "p.release()" from FileStorage destructor, because otherwise the smart pointer was released twice

@vpisarevvpisarev mentioned this pull requestJul 14, 2020
6 tasks
@EricFlorin
Copy link
Author

Thanks for letting me know@vpisarev! I was wondering what the error could be.

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

Reviewers

@alalekalalekalalek left review comments

Assignees

No one assigned

Labels

bugcategory: corepr: needs testNew functionality requires minimal tests set

Projects

None yet

Milestone

4.4.0

Development

Successfully merging this pull request may close these issues.

FileStorage access segfaults after assignment (assignment operator invalidates object)

4 participants

@EricFlorin@asmorkalov@vpisarev@alalek
[8]ページ先頭
©2009-2025 Movatter.jp