Bug #4388
closedbitvec: bitvec_read_field() can never return negative value on error
100%
Description
The doxygen documentation of bitvec_read_field() tells us that it can return a negative on error:
/*! read part of the vector
* \param[in] bv The boolean vector to work on
* \param[in,out] read_index Where reading supposed to start in the vector
* \param[in] len How many bits to read from vector
* \returns read bits or *negative value on error*
*/
uint64_t bitvec_read_field(struct bitvec *bv, unsigned int *read_index, unsigned int len) { ... }
However, as can be seen its return value is unsigned - uint64_t. So actually it returns a huge number on error.
This can be seen in the logs of OsmoPCU:
| Padding = 22|86|86|86|86|18446744073709551594|
where 18446744073709551594 is basically a negative number casted to uint64_t.
Updated by fixeria about 4 years ago
- Priority changed from Normal to High
Not sure how we're supposed to fix this problem. Changing the type of return value to 'int' does not seem to be a good solution. Most likely, we would have to deprecate this function in favor of a new one... or even multiple functions like bitvec_read_u8(), bitvec_read_u16(), bitvec_read_u32() and bitvec_read_u64().
Updated by fixeria about 4 years ago
- % Done changed from 0 to 10
Unit test demonstrating the problem: https://gerrit.osmocom.org/c/libosmocore/+/17220.
Updated by fixeria over 2 years ago
- Status changed from New to In Progress
- % Done changed from 10 to 80
I came up with an idea to use errno in order to indicate errors:
https://gerrit.osmocom.org/c/libosmocore/+/26307 bitvec_read_field(): indicate errors using errno [NEW]
so there is no need to deprecate this function. While working on it, I found and fixed some additional problems:
https://gerrit.osmocom.org/c/libosmocore/+/26308 bitvec_read_field(): fix incorrect bit-shift issue found by UBSan [NEW]
https://gerrit.osmocom.org/c/libosmocore/+/26309 bitvec_read_field(): optimize by expanding bytenum_from_bitnum() [NEW]
https://gerrit.osmocom.org/c/libosmocore/+/26310 tests/testsuite.at: ensure empty stderr for the bitvec_test [NEW]
Updated by fixeria over 2 years ago
- Status changed from In Progress to Resolved
- % Done changed from 80 to 100
All patches have been merged.