Project

General

Profile

Actions

Bug #4388

closed

bitvec: bitvec_read_field() can never return negative value on error

Added by fixeria about 4 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
libosmocore
Target version:
-
Start date:
02/05/2020
Due date:
% Done:

100%

Spec Reference:

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.

Actions #1

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().

Actions #2

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.

Actions #3

Updated by laforge over 2 years ago

  • Assignee set to fixeria
Actions #4

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]

Actions #5

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.

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)