trying to fix handling file storages with extremely long lines#16488
trying to fix handling file storages with extremely long lines#16488alalek merged 4 commits intoopencv:masterfrom
Conversation
alalek
left a comment
There was a problem hiding this comment.
Idea looks good, but it is too hard to validate other parts of persistence code.
| } | ||
| return ptr; | ||
| int count = (int)std::min(buffer.size() - ofs - 16, maxCount); | ||
| char* ptr = getsFromFile( &buffer[ofs], count+1 ); |
There was a problem hiding this comment.
- 16
count + 1
What is this? Why are these offsets different?
There was a problem hiding this comment.
fgets() requires "count+1", because it adds '\0' in the end. but sometimes we need to add '\n' to the end of line. so we might need at least count + 2. "+16" has been added just in case, it's definitely sufficient
| } | ||
| #endif | ||
| CV_Error(CV_StsError, "The storage is not opened"); | ||
| return ofs > 0 ? &buffer[0] : 0; |
There was a problem hiding this comment.
On .resize() all previously returned pointers from .gets() become invalid.
Are you sure that all previously returned pointers are not used anymore? (after next .gets() call)
There was a problem hiding this comment.
yes. I'm sure. when ".gets()" is called by the XML/YAML/JSON parser, it returns the pointer to the read line. The parsers process the line and then call ".gets()" another time to get the next line and so on. Of course, they do not store pointers to the middle of the previously read line, because they consume all the content. ".resize()" can be safely called inside ".gets()".
|
@alalek, the idea is obvious, and the patch is not that big. persistence functions are tested extensively by the test system itself. I suggest to integrate the patch now. 99% sure that it works correctly. If we find any problem with it, we can always revert the patch, because it's very local, touches just one file (+ adds the test that can be disabled). |
* it's now crash-test time! temporarily set the initial parser buffer size to just 40 bytes. let's run all the test and check if the buffer is always correctly resized and handled
| } | ||
|
|
||
| if( !ptr || !*ptr ) | ||
| CV_PARSE_ERROR_CPP( "Unexpected End-Of-File" ); |
There was a problem hiding this comment.
I believe ptr should be reassigned above instead:
ptr = parseMap(ptr, ...);
...
ptr = parseSeq(ptr, ...);
There was a problem hiding this comment.
I believe, those 2 lines just left out by mistake after refactoring. If ptr is reassigned, all the tests fail. Simply remove them.
| const size_t buf_size0 = 1 << 20; | ||
| size_t buf_size = buf_size0; | ||
| const size_t buf_size0 = 40; | ||
| buffer.resize(buf_size0); |
There was a problem hiding this comment.
Do we left this "as is" for merge?
|
@alalek, please, do not merge it! I think, I found some problems with this implementation. Wait a bit. |
trying to fix handling file storages with extremely long lines * trying to fix handling of file storages with extremely long lines: opencv#11061 * * fixed errorneous pointer access in JSON parser. * it's now crash-test time! temporarily set the initial parser buffer size to just 40 bytes. let's run all the test and check if the buffer is always correctly resized and handled * fixed pointer use in JSON parser; added the proper test to catch this case * fixed the test to make it more challenging. generate test json with * ** *** etc. shape
resolves #11061