fix a bug in variable length short calculation which can cause corrupt keyvi files #32

hendrikmuhs · 2018-01-31T07:13:44Z

fixes a off by 1 miscalculation between the length calculated by getVarshortLength and encodeVarshort, while the 1st returned 2 for 0x7fff the 2nd correctly returned 1 as 0x7fff fits in 1 byte (0111 1111 1111 1111 ). The same bug existed for the next boundary 0x3fffffff.

As a result of this miscalculation it was possible - although rather unlikely - that a bit in a bitvector was set, marking a written cell while it did not got written as the required space was 1 instead of 2. The consequence of the unwritten cell is a default to '\0', this '\0' caused a mismatch starting with version 0.2 where '\0' became a valid transition. This broken transition caused a segfault due to a broken pointer.

…t keyvi files.

hendrikmuhs · 2018-01-31T07:17:36Z

keyvi/tests/keyvi/util/vint_test.cpp

@@ -58,16 +58,33 @@ BOOST_AUTO_TEST_CASE(VShortLength) {
  BOOST_CHECK_EQUAL(77777, decodeVarshort(buffer));

  encodeVarshort(32767, buffer, &size);
-  BOOST_CHECK_EQUAL(util::getVarshortLength(1), size);


Note: a unit test was in place but the check was rubbish. This should have been:

BOOST_CHECK_EQUAL(util::getVarshortLength(32767), size);

or

BOOST_CHECK_EQUAL(1, size);

coveralls · 2018-01-31T08:05:42Z

Coverage remained the same at 91.07% when pulling 6fe59e2 on hendrikmuhs:fix-vint into f25d90c on KeyviDev:master.

narekgharibyan · 2018-01-31T10:18:00Z

keyvi/include/keyvi/util/vint.h

@@ -106,7 +106,7 @@ size_t getVarintLength(int_t value) {
 */
 template <typename int_t = uint64_t>
 size_t getVarshortLength(int_t value) {
-  return (value > 0x1fffffffffff) ? 4 : (value < 0x3fffffff) ? (value < 0x7fff) ? 1 : 2 : 3;
+  return (value > 0x1fffffffffff) ? 4 : (value < 0x40000000) ? (value < 0x8000) ? 1 : 2 : 3;


narekgharibyan

LGTM

…hort calculation which can cause corrupt keyvi files)

* - added test case for PR #32, (fix a bug in variable length short calculation which can cause corrupt keyvi files) * - enabled integration-tests on Travis CI * - removed leftover print statement

…t keyvi files (KeyviDev#32) fixes a off by 1 miscalculation between the length calculated by getVarshortLength and encodeVarshort, while the 1st returned 2 for 0x7fff the 2nd correctly returned 1 as 0x7fff fits in 1 byte (0111 1111 1111 1111 ). The same bug existed for the next boundary 0x3fffffff. As a result of this miscalculation it was possible - although rather unlikely - that a bit in a bitvector was set, marking a written cell while it did not got written as the required space was 1 instead of 2. The consequence of the unwritten cell is a default to '\0', this '\0' caused a mismatch starting with version 0.2 where '\0' became a valid transition. This broken transition caused a segfault due to a broken pointer.

* - added test case for PR KeyviDev#32, (fix a bug in variable length short calculation which can cause corrupt keyvi files) * - enabled integration-tests on Travis CI * - removed leftover print statement

…..15e4f77f 15e4f77f Corrected test names in .gitignore 93edd949 Fix typo in destination of the CMake config file 7b445fa7 Removed whitespaces at end of lines e886c605 Add support to build shared libraries on Window ca2fcd1a Merge branch 'patch-1' of https://gitlab.com/traversaro/tiny-process-library f9795d1c Increased minimum cmake version to 3.1 as suggested in #47 14a9f2e7 Add support to consume the library with find_package(tiny-process-library REQUIRED) 2e218295 Fixes #47: CMake minimal version required set to 3.0 85831b7c Add prefix to common test names d1356e66 Fixed comment 38e060e7 process_unix: Add new signal(signum) method 1b293821 No longer forces BUILD_TESTING to be ON ec5a78c1 Applied style format c5b0028f Fixed compilation on Windows a28099ab Minor cleanup of Config struct 7fea58d4 Merge branch 'win-show-window' into 'master' bf4e87ad Windows: expose the CreateProcess lpStartupInfo.wShowWindow choices 662be25a Added missing newline at end of file 40f5405c Cleanup: replaced NULL with nullptr 578466e4 Cleanup of get_exit_status and try_get_exit_status cd4f4fb6 Merge branch 'win-repeat-get-exit-status' of https://gitlab.com/ryanjmulder/tiny-process-library c9c8bf81 Do not run add_compile_options or add_definitions when tiny-process-library is a sub-project c5f8fbb7 add exit_status to windows 155706fb fix calling get_exit_status mutilple times on windows b7a49cf1 add tests for repeated call to get_exit_status df7c86bd Fixes #43: MSVC shadow warning e352957a Fixes #42: corrected include in process_win.cpp 953b19c4 No longer resolves symbolic links when Process(const std::string &command, ...) is used 93e0ed51 Cleanup of STREQUAL expression f7e76220 Update license year 3a82259c Squash -Wshorten compiler warning a6773276 Windows: CREATE_NO_WINDOW no longer set when stdout or stderr is redirected to parent process 05d9756e Unix, optimization: limit number of close calls on systems with high _SC_OPEN_MAX value bf455272 Config::inherit_file_descriptors is now supported on Windows 40a64207 Reverted last commit, and disabled config.inherit_file_descriptors on Windows due to test error in juCi++ 4091acdf Removed CREATE_NO_WINDOW flag in Windows's CreateProcess due to jucipp test fail. Will investigate further. ab8e4121 Removed windows specific settings, CREATE_NO_WINDOW is now always passed to CreateProcess. 7172fede Fixes #32 : added option to prevent inheriting file descriptors from parent process. ea3ede87 Default value for Config::windows::no_window is now true for consistent behavior across OSs 225a05f2 Fixes #35 : added Config struct for Process-constructors with no_window setting for Windows. Breaking change: constructor parameters buffer_size moved to Config struct. 6e52608b Merge branch 'poll' of https://gitlab.com/eidheim/tiny-process-library f14a022a fcntl returns 0 on success when the F_SETFL operation is used c3a25d9e Fixed tests on MacOS: do a last read on a file descriptor when it is closed 6293cf7a Unix: make read() non-blocking 5c8a3fbe Unix: simplify read loop condition, mark FDs as closed 15207735 Unix: further cleanup of async_read. Added errno == EINTR check to the poll-loop, and no longer stopps after just one of the stdout/stderr-fds are closed. Based on comments in !32 a90ea0cf Regarding #34: fixes compilation error for UNICODE as suggested by Aleksandr 38f39443 Unix: cleanup of async_read. Also no longer breaks/returns on EINTR errors. af5dd2de Unix: changed Data::exit_status to type int since the get_exit_status-functions are not thread safe, and minor improvements to the get_exit_status-functions 7d2777d3 process_unix: keep exit status (as on windows) aac992b1 process_unix: improve waitpid() error handling 13b74d3f process_unix: correctly handle waitpid() errors e3e4744e Unix: made use of poll(), running stdout and stderr readers in one thread instead of two. 273270d0 Comment cleanup 67ff6dca Windows: fixed CreateProcess call when UNICODE is used 0201606b Unix-like systems: added exits to Process::open functions b19bf49c Corrected path tests. Sometimes /bin is linked to for instance /usr/bin df1f1d9c Added arguments example a856ecc5 Added working path tests 3f103bba Added possibility to use arguments directly when creating processes b4f48df9 Fixes #31: UNICODE compilation error c6909dd9 Simplified examples link 54b95a21 Fix compilation error on Windows 92bb05ff Style format using custom clang-format a225f8cf Cleanup of comments fa54129d Improvements to environment implementation, and shorter sleeps in examples b88faa76 Added passing of environment variables for windows and linux. fae75dcd Removed build status from README.md. Using GitLab badge instead. 605fca5b Updated juCi++ link 614c0517 Migrated to GitLab 79d7ae48 Updated license year e384e4cd Added std::move to constructor member initializer lists 92ffe6b7 Fixes #29: CloseHandle no longer called on invalid handles a0348126 Updated license year 72790542 Fixed MSVC definition 88536b95 try_get_exit_status now tested on Windows git-subtree-dir: keyvi/3rdparty/tiny-process-library git-subtree-split: 15e4f77f8254e4b093f6be128db50fe4b6bee120

hendrikmuhs added 2 commits January 31, 2018 07:57

fix a bug in variable length short calculation which can cause corrup…

2ee4a96

…t keyvi files.

slight doc correction

6fe59e2

hendrikmuhs added the bug label Jan 31, 2018

hendrikmuhs requested a review from narekgharibyan January 31, 2018 07:13

hendrikmuhs commented Jan 31, 2018

View reviewed changes

narekgharibyan reviewed Jan 31, 2018

View reviewed changes

narekgharibyan approved these changes Jan 31, 2018

View reviewed changes

hendrikmuhs merged commit 3f8cfcf into KeyviDev:master Jan 31, 2018

narekgharibyan added a commit to narekgharibyan/keyvi-1 that referenced this pull request Feb 1, 2018

- added test case for PR KeyviDev#32, (fix a bug in variable length s…

9ef2b33

…hort calculation which can cause corrupt keyvi files)

narekgharibyan mentioned this pull request Feb 1, 2018

Test case for PR #32 #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bug in variable length short calculation which can cause corrupt keyvi files #32

fix a bug in variable length short calculation which can cause corrupt keyvi files #32

hendrikmuhs commented Jan 31, 2018

hendrikmuhs Jan 31, 2018

coveralls commented Jan 31, 2018

narekgharibyan Jan 31, 2018

narekgharibyan left a comment

fix a bug in variable length short calculation which can cause corrupt keyvi files #32

fix a bug in variable length short calculation which can cause corrupt keyvi files #32

Conversation

hendrikmuhs commented Jan 31, 2018

hendrikmuhs Jan 31, 2018

Choose a reason for hiding this comment

coveralls commented Jan 31, 2018

narekgharibyan Jan 31, 2018

Choose a reason for hiding this comment

narekgharibyan left a comment

Choose a reason for hiding this comment