Repository: opencv
Issue: test_io fails on master -> very basic coding error in persistence.cpp found via debugging and code review: no error checking fopen(). WTF?
Labels: bug,category: core


##### System information (version)
- OpenCV => master branch, currently at commit SHA-1: 8c221981bf7e2c8eb87302d70000aa246641487c (but that's not really important as the bug is already there since 2018 😨 )
- Operating System / Platform => Windows 64 Bit
- Compiler => Visual Studio 2019

##### Detailed description

`fopen()` cannot open, nor create, the test output file(s) in the indicated path, which is "io/blablabla.xml", ditto for yaml and json: it's the fact the "io" directory isn't there, obviously. (Test doesn't create the directory and fopen() cannot create the path *to* the file, as per spec. Resulting in the XML/YAML/JSON I/O tests being reported as FAILED by `opencv_test_core`.

The problem is not that the `"io"` directory is not created (**that's a /different/ minor issue, which can be fixed several ways**), it's the fact that `fopen()` sensibly returns NULL, which is checked but **not acted upon as any error checking logic is totally *absent*, making this a silent error**, which is caught luckily by subsequent data mismatches failing the tests.

Diagnosis through debugging and code review + `git blame`:

Relevant code in/at `modules/core/src/persistence.cpp` line 449 onwards (`open()`) originated in commit SHA-1: 0f622206e48f6050536f00d47abf47c4a2d1824a back in 2018. 

`fopen()` call can be found at line 504, where a NULL check happens with consequent `return false`. *'kay.*

Move to caller code, which also happens to be in `modules/core/src/persistence.cpp`, at lines 1792-1799, where call to `open` happens at line 1796 via `p->open(etc etc etc)`. Next line does check the `ok` bool returned, but **does not consider FALSE an error**. 💥  This code is there since 2018, commit SHA-1: 0f622206e48f6050536f00d47abf47c4a2d1824a (`git blame`) making the relevant bit of code originating from the very same commit as the above.

Now I wonder a couple of things:

- **where** is one expected to check and throw/flag errors in here? Should `open()` have thrown an exception? Or, since there clearly is an indirection level in here (the `p->open(...)`), is the higher layer supposed to check for errors and act inside that `FileStorage` constructor? 
 
  Either way, something should have been done in `persistence.cpp` about `fopen()` returning a `NULL`, as the current state of affairs makes *any* subsequent test code writing to the (non-existing = failed) `FileStorage` instance moot. The test failure report (console output) is throwing one curveball for this as the non-matching values are all reported to be zero(0). Well, *obviously*, because they could never be loaded from anywhere! 

- **why** is such a basic mistake of I/O error checking omission not fixed since November 2018 (that's what `git blame` says is the commit's timestamp)?  `persistence.cpp` seems to be an integral part of OpenCV itself and not some potentially shoddy quick&dirty test rigging anyway.

  I am reviewing this code for production purposes and as this is the first thing to encounter upon starting with OpenCV today, it makes me wonder about robustness wholesale and pretty paranoid right off the bat. Fortunately did the rest of the `core` tests pass, but this is a class CS 101 error. Sure it happens someday to everyone, including me, but it doesn't stay around for 2 years unless *absolutely nobody* did a build-from-source and run the tests -- I'm coming up short with other reasonable explanations for this. WTF?
  + of course, this makes the FileStorage class pretty brittle, but then one wonders how many out there actually use it? *Or use anything off the `master` branch, perhaps?*
- am I a dunce? Did **I** do something stupid? ... **Could be.** Or-r-r-r...? 🤔😓 

> **Aside**: I build *all* important stuff from source as a basic policy (and mess around a bit with it), because I've seen too much shite with both "prebuilt" and commercial binary-only libraries to last me a lifetime. I *know* that some day in the future I will need to debug a problem down to some "core code", so better be prepared -- it's happened before often enough, always with "weird shit" reported by customers and very tough diagnostic paths to travel when it happens. So I like to experience the brittleness up front a bit, to get a feel for it. *Prayer wheels are spinning now.*
>
> So I followed the guidelines blogged elsewhere (https://amin-ahmadi.com/2020/05/04/building-opencv-4-3-0-with-visual-studio-2019-community/ -- big *thank you* to that man! 👍 ) and started from a fresh `git clone` of this beast. CMake et al took it from there. OpenCV passed the first landmark of *crappiness-or-not* with flying colours by not giving me a cartload of build errors. 🥳  But then the tests did fail... 😅 


##### Steps to reproduce

Build OpenCV from source (CMake, python, numpy, MSVC2019, *the works*). Run the `opencv_test_core` test executable in the MSVC2019 debug environment (hit [F5] key). All tests pass after 100+ seconds, **except** the XML/YAML/JSON I/O tests are reported as FAILED. 😨 

Make it easier on yourself and copy the generated MSVC project file for opencv_test_core, rename and throw out everything except the basics:

![image](https://user-images.githubusercontent.com/402462/106199462-cb235f80-61b5-11eb-9887-ba76b672ef5d.png)

so the tests run quicker and you get a faster turn-around cycle debugging the bastard. Observe FAILs reported for all three I/O tests. (When you fiddle a bit with the `DISABLED_...` others -- because you *may* wonder if the `rw` tests **maybe** should've come first or what-not -- you'll quickly discover you're betting on the wrong horse. ALL those I/O tests fail, hence better to create a *lean* test binary first and cycle through it while debugging. Which will show `fopen()` deep down in there barfing a hairball due to extra leading diarectory "io" in the `filename` path which does not exist yet. Thus you beget NULL. 💣 


##### Issue submission checklist

- [x] I report the issue, it's not a question

   Heck, this definitely is a bug. Either that or it's my personal January Dunce Day. The questions are a *result* derived from the *bug*.

- [x] I checked the problem with documentation, FAQ, open issues,
       forum.opencv.org, Stack Overflow, etc and have not found solution

  👎 no github search hits for test_io, opencv_test_core, open fail, test fail (oh, *oodles* for them, but not this one, oddly enough, AFAICT from the zillion issue subject lines github search delivered.)

   * OpenCV documentation: https://docs.opencv.org
   * FAQ page: https://github.com/opencv/opencv/wiki/FAQ
   * OpenCV forum: https://forum.opencv.org
   * Stack Overflow branch: https://stackoverflow.com/questions/tagged/opencv
   
  I admit I didn't check *those* for this fundamental thing. Score -1 for the visiting team.

- [x] I updated to latest OpenCV version and the issue is still there
 
   I'm on the effin' bleedin' edge! `*master*`. Oh! Oh! Checked your 3.4 branch **now** and **it is not there**. Apparently 3,.4 is still rockin' the old persistence code. 

   Did a `git bisect` back to oct/2018 on the master line with `grep -e 'open\b' -C 3 modules/core/src/persistence.cpp` as decision criterium (plus eyeballing the grep output) and the first time this got into `master` apparently is commit SHA-1: 0f622206e48f6050536f00d47abf47c4a2d1824a, november 2018. Well, eff me then. Except you say:

> **master branch** for OpenCV 4.x \[...]
> OpenCV team supports only latest release for each branch.

which says to me non-English-*native* dude that I'm still sort of on the money, support-wise, right? 📿🙏 

 - [x] There is reproducer code and related data files: videos, images, onnx, etc

   Your own `opencv_test_core` project/binary (as produced by CMake+MSVC & Partners) serves.