Added a copy constructor and an overloaded assignment operator to FileStorage

[EricFlorin]

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: a cv::FileStorage object that was assigned by another cv::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 that fs was being assigned to a temporary cv::FileStorage object. I went through the execution path of the assignment using my debugger, and noticed that fs was not being assigned a deep copy of the temporary cv::FileStorage object.

Since fs was not given a deep copy of the temporary cv::FileStorage object, it loses access to the file reference in the temporary cv::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 to cv::FileStorage that creates deep copies of cv::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 at https://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

[asmorkalov]

@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]

[asmorkalov]

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

[EricFlorin]

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 for cv::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]

XML/JSON/YAML formats handling is done by different cv::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]

@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]

[EricFlorin]

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 are in 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!

[alalek]

Thank you for the contribution!

Please avoid implementing of "deep-copy" logic for FileStorage.

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

[alalek]

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 for FileStorage (especially in case of write mode), so let avoid that completely.

[asmorkalov]

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

[EricFlorin]

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 temporary cv::FileStorage object is created. When the temporary cv::FileStorage object is created, p (a shared pointer) is initialize a new cv::FileStorage::Impl object. The cv::FileStorage::Impl object contains an vector of cv::FileNode called roots. Each cv::FileNode in roots contains a constant cv::FileStorage pointer, whose address is that of the temporary cv::FileStorage object.
2. The fs cv::Filestorage object is assigned the temporary cv::FileStorage object. The assignment causes fs.p to be assigned to the temporary cv::FileStorage object's p.
3. After assignment is complete, the temporary cv::FileStorage object is destroyed. The destruction does not affect fs.p since p is a shared pointer. However, since the p in the temporary cv:FileStorage object is emptied via the destructor, any cv::FileNode with a pointer to the address of the temporary cv::FileStorage object will have access to an empty p.
4. From there, fs["string"] >> s is called. The segfault occurs after res = p->roots[i][key]; is called in FileNode FileStorage::operator [](const std::string& key) const. In the case of the reproducer code, this statement is uses the cv::FileNode located at roots[0]. The constant cv::FileStorage pointer in this cv::FileNode points to a cv::FileStorage object whose p is empty. The "contents" that p contained was attempted to be accessed in const uchar* FileNode::ptr() const after IsMap() was called in FileNode FileNode::operator[](const std::string& nodename) const; which failed since p is empty.

From there, I am not sure what should I do. I was thinking of having the const FileStorage* in cv::FileStorage be a cv::Ptr<FileStorage>, but I feel that it will not fix the problem as the p in the temporary cv::FileStorage object will still be deleted.

Any thoughts?

[vpisarev]

@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

[EricFlorin]

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