From d5d31292316a05f6bcc2fc1ee4632d869e58b247 Mon Sep 17 00:00:00 2001 From: James Thompson Date: Sat, 9 Dec 2017 21:53:43 +0000 Subject: [PATCH 1/2] Prototype safe wrapper for AisBitset accesses Provides safe a batch-fetch interface for AisBitset bit sub-sequences, erroring on out-of-bounds requests at runtime or dying on an assert if they're enabled. The interface is supposed to match libais's current control flow approach, avoiding requiring major code surgery to integrate. It's currently a bit too wordy, though, and could be improved. This is designed to tackle a semi-regular class of bugs in libais, where a decoder-set message length precondition is narrower than the AisBitset calls made further down the function. Since asserts are the only error handling in this situation we currently get UB in release builds. --- src/libais/ais.cpp | 34 +++++++++++++++++++++++++++ src/libais/ais.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/src/libais/ais.cpp b/src/libais/ais.cpp index e0cbc77d..33b9d0f4 100644 --- a/src/libais/ais.cpp +++ b/src/libais/ais.cpp @@ -234,7 +234,41 @@ const AisPoint AisBitset::ToAisPoint(const size_t start, return AisPoint(lng_deg / divisor, lat_deg / divisor); } +int AisBitset::GetValue(BitGetter getter) const { + assert(getter.target); + assert(getter.start + 1 <= GetNumBits()); + + if (getter.start + 1 > GetNumBits()) { + return -1; + } else { + *getter.target = operator[](getter.start); + return 0; + } +} +int AisBitset::GetValue(StringGetter getter) const { + assert(getter.target); + assert(getter.start + getter.len <= GetNumBits()); + + if (getter.start + getter.len > GetNumBits()) { + return -1; + } else { + *getter.target = ToString(getter.start, getter.len); + return 0; + } +} +int AisBitset::GetValue(UIntGetter getter) const { + assert(getter.target); + assert(getter.start + getter.len <= GetNumBits()); + + if (getter.start + getter.len > GetNumBits()) { + return -1; + } else { + *getter.target = ToUnsignedInt(getter.start, getter.len); + return 0; + } +} + // static private void AisBitset::InitNmeaOrd() { diff --git a/src/libais/ais.h b/src/libais/ais.h index 6d6baa67..8608cc0c 100644 --- a/src/libais/ais.h +++ b/src/libais/ais.h @@ -411,6 +411,50 @@ class AisBitset : protected bitset { // Visible for testing. static bitset<6> Reverse(const bitset<6> &bits); + // Safe batch fetcher for multiple values stored within one AisBitset. + // Provide a list of 'getter' structs (defined below) to define the values to + // fetch and the addresses to write them to. + // These should be in start-position ascending order to preserve + // GetRemaining() etc. + // Note that GetValue() dies on an assert if an out of bounds request is made + // and they're compiled in. + template + AIS_STATUS GetValues(Getters... getters) const { + // NB standard states order of evaluation in initlist is 0, 1, 2... + int rets[] = {GetValue(getters)...}; + for (auto r : rets) { + if (r) + return AIS_ERR_BAD_BIT_COUNT; + } + return AIS_UNINITIALIZED; + } + + // AisBitset sub-sequence 'Getter' objects for use in GetVal()/GetVals(). + // The struct type states what type of value you want to interpret the + // sub-seq as, e.g. UIntGetter fetches unsigned integers (as ints). + // Their member variables define where the sub-seq is in the AisBitset, using + // the same numbering scheme as ToUnsignedInt() etc above. + // 'target' is an out parameter pointing where you want to write the value. + struct BitGetter { + bool *target = nullptr; + const int start = 0; + explicit BitGetter (bool *t, int s) + :target{t}, start{s} {} + }; + struct StringGetter { + string *target = nullptr; + const int start = 0, len = 0; + explicit StringGetter (string *t, int s, int l) + :target{t}, start{s}, len{l} {} + }; + // NB writes to an int target, not uint, fitting AisXXX classes member vars + struct UIntGetter { + int *target = nullptr; + const int start = 0, len = 0; + explicit UIntGetter (int *t, int s, int l) + :target{t}, start{s}, len{l} {} + }; + protected: int num_bits; int num_chars; @@ -429,6 +473,19 @@ class AisBitset : protected bitset { // This field is also used to determine the number of remaining bits after the // last read position. mutable int current_position; + + // Memory safe fetchers for values stored in AisBitset. + // These depend on the above 'getter' structs to define the sub-sequence + // location in the AisBitset to fetch from, the value type to read, and the + // address to write the fetched read value to. + // + // If the requested sub-sequence falls inside the current AisBitset (success), + // sets *getter.target with the fetched value and returns 0. + // Otherwise (i.e. requested out of bounds), if asserts are enabled dies on an + // assert, or if not returns -1 and doesn't modify *getter.target. + int GetValue(BitGetter getter) const; + int GetValue(UIntGetter getter) const; + int GetValue(StringGetter getter) const; }; class AisMsg { From 9637dbc6be0975fb0174e9d0d9c40311337d8d84 Mon Sep 17 00:00:00 2001 From: James Thompson Date: Sat, 9 Dec 2017 22:01:10 +0000 Subject: [PATCH 2/2] Convert Ais20 and Ais26 ctrs to use the new safer AisBitset wrapper --- src/libais/ais20.cpp | 55 ++++++++++++++++++++++-------------- src/libais/ais26.cpp | 67 ++++++++++++++++++++++++++++---------------- 2 files changed, 77 insertions(+), 45 deletions(-) diff --git a/src/libais/ais20.cpp b/src/libais/ais20.cpp index 07a5fabb..4c3ffab7 100644 --- a/src/libais/ais20.cpp +++ b/src/libais/ais20.cpp @@ -18,58 +18,71 @@ Ais20::Ais20(const char *nmea_payload, const size_t pad) status = AIS_ERR_BAD_BIT_COUNT; return; } + using UIGet = AisBitset::UIntGetter; + // 160, but must be 6 bit aligned assert(message_id == 20); bits.SeekTo(38); - spare = bits.ToUnsignedInt(38, 2); - - offset_1 = bits.ToUnsignedInt(40, 12); - num_slots_1 = bits.ToUnsignedInt(52, 4); - timeout_1 = bits.ToUnsignedInt(56, 3); - incr_1 = bits.ToUnsignedInt(59, 11); + status = bits.GetValues(UIGet{&spare, 38, 2}, + UIGet{&offset_1, 40, 12}, + UIGet{&num_slots_1, 52, 4}, + UIGet{&timeout_1, 56, 3}, + UIGet{&incr_1, 59, 11}); + if (!CheckStatus()) return; if (num_bits == 72) { - spare2 = bits.ToUnsignedInt(70, 2); + status = bits.GetValues(AisBitset::UIntGetter{&spare2, 70, 2}); + if (!CheckStatus()) return; + assert(bits.GetRemaining() == 0); status = AIS_OK; return; } group_valid_2 = true; - offset_2 = bits.ToUnsignedInt(70, 12); - num_slots_2 = bits.ToUnsignedInt(82, 4); - timeout_2 = bits.ToUnsignedInt(86, 3); - incr_2 = bits.ToUnsignedInt(89, 11); + status = bits.GetValues(UIGet{&offset_2, 70, 12}, + UIGet{&num_slots_2, 82, 4}, + UIGet{&timeout_2, 86, 3}, + UIGet{&incr_2, 89, 11}); + if (!CheckStatus()) return; + // 100 bits for the message // 104 is the next byte boundary // 108 is the next 6 bit boundary -> 18 characters if (num_bits >= 100 && num_bits <=108) { - spare2 = bits.ToUnsignedInt(100, bits.GetRemaining()); + status = bits.GetValues(UIGet{&spare2, 100, bits.GetRemaining()}); + if (!CheckStatus()) return; + status = AIS_OK; return; } group_valid_3 = true; - offset_3 = bits.ToUnsignedInt(100, 12); - num_slots_3 = bits.ToUnsignedInt(112, 4); - timeout_3 = bits.ToUnsignedInt(116, 3); - incr_3 = bits.ToUnsignedInt(119, 11); + status = bits.GetValues(UIGet{&offset_3, 100, 12}, + UIGet{&num_slots_3, 112, 4}, + UIGet{&timeout_3, 116, 3}, + UIGet{&incr_3, 119, 11}); + if (!CheckStatus()) return; + // 130 bits for the message // 136 is the next byte boundary // 138 is the next 6 bit boundary -> 23 characters if (num_bits >= 130 && num_bits <= 138) { // Makes the result 8 bit / 1 byte aligned. - spare2 = bits.ToUnsignedInt(130, bits.GetRemaining()); + status = bits.GetValues(UIGet{&spare2, 130, bits.GetRemaining()}); + if (!CheckStatus()) return; + status = AIS_OK; return; } group_valid_4 = true; - offset_4 = bits.ToUnsignedInt(130, 12); - num_slots_4 = bits.ToUnsignedInt(142, 4); - timeout_4 = bits.ToUnsignedInt(146, 3); - incr_4 = bits.ToUnsignedInt(149, 11); + status = bits.GetValues(UIGet{&offset_4, 130, 12}, + UIGet{&num_slots_4, 142, 4}, + UIGet{&timeout_4, 146, 3}, + UIGet{&incr_4, 149, 11}); + if (!CheckStatus()) return; spare2 = 0; diff --git a/src/libais/ais26.cpp b/src/libais/ais26.cpp index 81bf1007..92899ce7 100644 --- a/src/libais/ais26.cpp +++ b/src/libais/ais26.cpp @@ -24,83 +24,102 @@ Ais26::Ais26(const char *nmea_payload, const size_t pad) } // TODO(schwehr): Check for off by one. - const size_t comm_flag_offset = num_bits - 20; + const int comm_flag_offset = num_bits - 20; if (num_bits < 52 || num_bits > 1064) { status = AIS_ERR_BAD_BIT_COUNT; return; } - + assert(message_id == 26); bits.SeekTo(38); - const bool addressed = bits[38]; - use_app_id = bits[39]; + + using BitGet = AisBitset::BitGetter; + using UIGet = AisBitset::UIntGetter; + + bool addressed = false; + status = bits.GetValues(BitGet{&addressed, 38}, + BitGet{&use_app_id, 39}); + if (!CheckStatus()) return; + if (addressed) { dest_mmsi_valid = true; - dest_mmsi = bits.ToUnsignedInt(40, 30); + status = bits.GetValues(UIGet{&dest_mmsi, 40, 30}); + if (!CheckStatus()) return; + if (use_app_id) { if (num_bits < 86) { status = AIS_ERR_BAD_BIT_COUNT; return; } - dac = bits.ToUnsignedInt(70, 10); - fi = bits.ToUnsignedInt(80, 6); + status = bits.GetValues(UIGet{&dac, 70, 10}, + UIGet{&fi, 80, 6}); + if (!CheckStatus()) return; } // TODO(schwehr): Handle the payload. } else { // broadcast if (use_app_id) { - dac = bits.ToUnsignedInt(40, 10); - fi = bits.ToUnsignedInt(50, 6); + status = bits.GetValues(UIGet{&dac, 40, 10}, + UIGet{&fi, 50, 6}); + if (!CheckStatus()) return; } - // TODO(schwehr): Handle the payload. } bits.SeekTo(comm_flag_offset); - commstate_flag = bits[comm_flag_offset]; - sync_state = bits.ToUnsignedInt(comm_flag_offset + 1, 2); // SOTDMA and TDMA. + status = bits.GetValues(UIGet{&commstate_flag, comm_flag_offset, 1}, + // SOTDMA and TDMA + UIGet{&sync_state, comm_flag_offset + 1, 2}); + if (!CheckStatus()) return; if (!commstate_flag) { // SOTDMA - slot_timeout = bits.ToUnsignedInt(comm_flag_offset + 3, 3); slot_timeout_valid = true; + status = bits.GetValues(UIGet{&slot_timeout, comm_flag_offset + 3, 3}); + if (!CheckStatus()) return; + switch (slot_timeout) { case 0: - slot_offset = bits.ToUnsignedInt(comm_flag_offset + 6, 14); slot_offset_valid = true; + status = bits.GetValues(UIGet{&slot_offset, comm_flag_offset + 6, 14}); + if (!CheckStatus()) return; break; case 1: - utc_hour = bits.ToUnsignedInt(comm_flag_offset + 6, 5); - utc_min = bits.ToUnsignedInt(comm_flag_offset + 11, 7); - utc_spare = bits.ToUnsignedInt(comm_flag_offset + 18, 2); utc_valid = true; + status = bits.GetValues(UIGet{&utc_hour, comm_flag_offset + 6, 5}, + UIGet{&utc_min, comm_flag_offset + 11, 7}, + UIGet{&utc_spare, comm_flag_offset + 18, 2}); + if (!CheckStatus()) return; break; case 2: // FALLTHROUGH case 4: // FALLTHROUGH case 6: - slot_number = bits.ToUnsignedInt(comm_flag_offset + 6, 14); slot_number_valid = true; + status = bits.GetValues(UIGet{&slot_number, comm_flag_offset + 6, 14}); + if (!CheckStatus()) return; break; case 3: // FALLTHROUGH case 5: // FALLTHROUGH case 7: - received_stations = bits.ToUnsignedInt(comm_flag_offset + 6, 14); received_stations_valid = true; + status = bits.GetValues(UIGet{&received_stations, comm_flag_offset + 6, 14}); + if (!CheckStatus()) return; break; default: assert(false); } } else { // ITDMA - slot_increment = bits.ToUnsignedInt(comm_flag_offset + 3, 13); - slot_increment_valid = true; - slots_to_allocate = bits.ToUnsignedInt(comm_flag_offset + 16, 3); + slot_increment_valid = true; slots_to_allocate_valid = true; - - keep_flag = bits[comm_flag_offset + 19]; keep_flag_valid = true; + + status = bits.GetValues(UIGet {&slot_increment, comm_flag_offset + 3, 13}, + UIGet {&slots_to_allocate, comm_flag_offset + 16, 3}, + BitGet{&keep_flag, comm_flag_offset + 19}); + if (!CheckStatus()) return; } // TODO(schwehr): Add assert(bits.GetRemaining() == 0);