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

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

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.

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.

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

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!

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

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?

@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

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