API change support and whole rework to find a Windows crash before.#86
Conversation
As the fische is a independent code where not access Kodi's API can it be in own folder to have more clean and prepared for further changes. Further becomes it easier to take the code by someone to places outside of Kodi.
As this goes with ASM deep in hardware, makes it sense for me to leave as "C" and not as "C++". From my side hold the code there as small as possible. Further is it independent to all other places.
This done to have better usable in coming changes. As the fische code by add-on was on begin defined as a "C" code only and over the year some "C++" parts implemented to them. About this and to become a usable "C" ABI support back to them are the headers renamed and to identify in furture which relates to "C" and whicht to "C++".
To makes it a bit cleaner and not have the #ifdef and #endif.
Kodiai Review SummaryWhat ChangedThis PR addresses a Windows crash caused by std::fstream reading vector field data incorrectly, replacing it with Kodi VFS API for cross-platform binary file I/O. The code was also reorganized, moving files into a src/fische/ directory structure. Requested minimal review; timeout risk auto-reduced the effective review to minimal. Reviewed: core logic, file I/O Strengths
ObservationsImpact[CRITICAL] src/fishbmc_addon.cpp (418-419): Unchecked malloc causes null pointer dereference when system is out of memory [CRITICAL] src/fische/vectorfield.cpp (57-58): Unchecked malloc leads to crash in vector generation [MAJOR] src/fische/vectorfield.cpp (44-52): Loaded vector data lacks validation, risking buffer overflow Suggestions
Verdict🔴 Address before merging -- 3 blocking issue(s) found (2 CRITICAL, 1 MAJOR) The two critical malloc checks are essential to prevent crashes under low-memory conditions. The major validation issue would prevent future file corruption bugs. Review Details
|
Relates to:
```
CMake Deprecation Warning at CMakeLists.txt:2 (cmake_policy):
Compatibility with CMake < 3.10 will be removed from a future version of
CMake.
```
And to have equal to Kodi's used version.
Some parts in mathematic processes are leaved on the old "C" way. Other parts are in C++ now, reason why done is to have more OS independent and more clean in Code. Seen that fische in his Version 3 was based complete upon C++, but done here from scratch without code take from his old version. See here version 3: https://sources.debian.org/src/fische/3.2.2-8/src And here version 4: https://github.com/maysl/fische
Normally not needed on Kodi, but thought to have it as lib usable in "C" too. Tested temporary by set add-on code to use as "C" of it and has worked.
|
@kodiai review this please |
![kodiai[bot]](https://avatars.githubusercontent.com/in/2832814?s=60&v=4){kind=small}
Kodiai Review SummaryWhat ChangedThis PR updates the add-on for Kodi API changes and fixes a Windows crash related to ReadVectors file I/O by replacing std::fstream with Kodi's VFS API. Reviewed: core logic, docs Strengths
ObservationsImpact[CRITICAL] src/fische/vector.cpp (29-34): Division by zero when vector length is zero [CRITICAL] src/fische/analyst.cpp (51): Division by zero when no beat gaps match criteria [CRITICAL] src/fische/vectorfield.cpp (45-51): Missing validation for ReadVectors return value [MAJOR] src/fische/vectorfield.cpp (103): Thread-safety issue with rand() [MAJOR] src/fishbmc_addon.cpp (419): Memory leak when hash validation fails Preference[MEDIUM] src/fische/vectorfield.cpp (124): Boundary validation inconsistency [MEDIUM] src/fische/vectorfield.cpp (133): Magic number in pointer arithmetic Verdict🔴 Address before merging -- 5 blocking issue(s) found (CRITICAL/MAJOR) |
|
@kodiai review this please |
Kodiai Review SummaryWhat ChangedAPI update for Kodi changes and comprehensive rework to fix Windows crash related to vector file reading using std::fstream, now replaced with kodi::vfs::CFile for cross-platform compatibility. Reviewed: core logic, docs Strengths
ObservationsImpact[CRITICAL] src/fische/vectorfield.cpp (42-58): Missing validation for loaded vector data causes potential buffer overflow [CRITICAL] src/fische/blurengine.cpp (123-135): Missing bounds validation enables buffer overflow from vector data [MAJOR] src/fische/blurengine.cpp (96): Incorrect type for vector components causes wrong value interpretation [MAJOR] src/fische/vectorfield.cpp (54-57): Missing nullptr assignment after free on error path [MAJOR] src/fische/vectorfield.cpp (71-92): Null pointer dereference risk after malloc failure Preference[MEDIUM] src/fishbmc_addon.cpp (415-421): Missing size validation allows excessive memory allocation [MEDIUM] src/fische/vectorfield.cpp (73-90): WriteVectors called even when vector generation was cancelled Suggestions
Verdict🔴 Address before merging -- 5 blocking issue(s) found (CRITICAL/MAJOR) |
|
The last AI notes I'm mostly ignored now, some of them makes not directly sense and some of them go into too much detail for me. I'll leave it as it is now. Gotta finish it. |
For a for me unknown reason was by this "std::fstream vectorsfile(filename.str().c_str(), std::fstream::in);" something partly wrong readed where creates after few seconds to one minute by playback a crash of Kodi. Due a strange reason this comes only on Windows. By fault search I was not come on begin to them, thought on begin maybe relates to threads and why rewritten the code to find why it crash. With use of our Kodi filesystem API has it worked correct.
This the reason why started on add-on and to have usable on coming API change of Kodi.
Wanted by our AI
1 participant
Add-on update related to Kodi API change defined on xbmc/xbmc#28278
Thought about making the API change and noticed that the add-on always crashes under Windows. At the beginning I couldn't figure out where this crash was coming from and reworked the code to find the cause at some point.
At the end I realized that the crash came from reading "ReadVectors" and
std::fstream vectorsfile(filename.str().c_str(), std::fstream::in);. Somehow the data read was not correct on Windows and led to a crash in mathematical processes (in blurengine.cpp).