From 69ad36a9ae0d8886ddbf61fe02dc5d2a7c2e4002 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 8 Oct 2021 20:40:23 -0700 Subject: [PATCH 1/2] scpfluxsource: Support more than 5 revolutions This prevents uninitialized memory reads for files with many revolutions. See #322. --- lib/fluxsource/scpfluxsource.cc | 26 +++++++++++++++++++------- lib/scp.h | 21 ++++++++++++++------- lib/utils.h | 2 ++ 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/lib/fluxsource/scpfluxsource.cc b/lib/fluxsource/scpfluxsource.cc index 97499ce2..86e9591a 100644 --- a/lib/fluxsource/scpfluxsource.cc +++ b/lib/fluxsource/scpfluxsource.cc @@ -2,6 +2,7 @@ #include "fluxmap.h" #include "kryoflux.h" #include "lib/fluxsource/fluxsource.pb.h" +#include "lib/utils.h" #include "fluxsource/fluxsource.h" #include "scp.h" #include "proto.h" @@ -57,20 +58,31 @@ public: std::unique_ptr readFlux(int track, int side) { int strack = strackno(track, side); + if (strack >= ARRAY_SIZE(_header.track)) + return std::unique_ptr(); uint32_t offset = Bytes(_header.track[strack], 4).reader().read_le32(); if (offset == 0) return std::unique_ptr(); - ScpTrack trackheader; + ScpTrackStart trackstart; _if.seekg(offset, std::ios::beg); - _if.read((char*) &trackheader, sizeof(trackheader)); + _if.read((char*) &trackstart, sizeof(trackstart)); check_for_error(); - if ((trackheader.track_id[0] != 'T') - || (trackheader.track_id[1] != 'R') - || (trackheader.track_id[2] != 'K')) + if ((trackstart.track_id[0] != 'T') + || (trackstart.track_id[1] != 'R') + || (trackstart.track_id[2] != 'K')) Error() << "corrupt SCP file"; + std::vector revs(_header.revolutions); + for (int revolution = 0; revolution < _header.revolutions; revolution++) + { + ScpTrackRevolution trackrev; + _if.read((char*) &trackrev, sizeof(trackrev)); + check_for_error(); + revs[revolution] = trackrev; + } + std::unique_ptr fluxmap(new Fluxmap); nanoseconds_t pending = 0; unsigned inputBytes = 0; @@ -79,8 +91,8 @@ public: if (revolution != 0) fluxmap->appendIndex(); - uint32_t datalength = Bytes(trackheader.revolution[revolution].length, 4).reader().read_le32(); - uint32_t dataoffset = Bytes(trackheader.revolution[revolution].offset, 4).reader().read_le32(); + uint32_t datalength = Bytes(revs[revolution].length, 4).reader().read_le32(); + uint32_t dataoffset = Bytes(revs[revolution].offset, 4).reader().read_le32(); Bytes data(datalength*2); _if.seekg(dataoffset + offset, std::ios::beg); diff --git a/lib/scp.h b/lib/scp.h index 7df3be31..77c373a4 100644 --- a/lib/scp.h +++ b/lib/scp.h @@ -27,17 +27,24 @@ enum SCP_FLAG_FOOTER = (1<<5) }; +struct ScpTrackStart +{ + char track_id[3]; // 'TRK' + uint8_t strack; // SCP track number +}; + +struct ScpTrackRevolution +{ + uint8_t index[4]; // time for one revolution + uint8_t length[4]; // number of bitcells + uint8_t offset[4]; // offset to bitcell data, relative to track header +}; + struct ScpTrack { char track_id[3]; // 'TRK' uint8_t strack; // SCP track number - struct - { - uint8_t index[4]; // time for one revolution - uint8_t length[4]; // number of bitcells - uint8_t offset[4]; // offset to bitcell data, relative to track header - } - revolution[5]; + struct ScpTrackRevolution revolution[5]; }; #endif diff --git a/lib/utils.h b/lib/utils.h index 75efce02..76505017 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -1,6 +1,8 @@ #ifndef UTILS_H #define UTILS_H +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0])) + extern bool beginsWith(const std::string& value, const std::string& beginning); extern bool endsWith(const std::string& value, const std::string& ending); From 181f2f38d80e16ed83f76a1ab5ced315d2fe3841 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Sat, 30 Oct 2021 10:29:17 -0700 Subject: [PATCH 2/2] s/ScpTrackStart/ScpTrackHeader/ and use in ScpTrack --- lib/fluxsink/scpfluxsink.cc | 8 ++++---- lib/fluxsource/scpfluxsource.cc | 10 +++++----- lib/scp.h | 7 +++---- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/lib/fluxsink/scpfluxsink.cc b/lib/fluxsink/scpfluxsink.cc index a4755dca..ab4984a4 100644 --- a/lib/fluxsink/scpfluxsink.cc +++ b/lib/fluxsink/scpfluxsink.cc @@ -88,10 +88,10 @@ public: int strack = strackno(cylinder, head); ScpTrack trackheader = {0}; - trackheader.track_id[0] = 'T'; - trackheader.track_id[1] = 'R'; - trackheader.track_id[2] = 'K'; - trackheader.strack = strack; + trackheader.header.track_id[0] = 'T'; + trackheader.header.track_id[1] = 'R'; + trackheader.header.track_id[2] = 'K'; + trackheader.header.strack = strack; FluxmapReader fmr(fluxmap); Bytes fluxdata; diff --git a/lib/fluxsource/scpfluxsource.cc b/lib/fluxsource/scpfluxsource.cc index 86e9591a..bac2582c 100644 --- a/lib/fluxsource/scpfluxsource.cc +++ b/lib/fluxsource/scpfluxsource.cc @@ -64,14 +64,14 @@ public: if (offset == 0) return std::unique_ptr(); - ScpTrackStart trackstart; + ScpTrackHeader trackheader; _if.seekg(offset, std::ios::beg); - _if.read((char*) &trackstart, sizeof(trackstart)); + _if.read((char*) &trackheader, sizeof(trackheader)); check_for_error(); - if ((trackstart.track_id[0] != 'T') - || (trackstart.track_id[1] != 'R') - || (trackstart.track_id[2] != 'K')) + if ((trackheader.track_id[0] != 'T') + || (trackheader.track_id[1] != 'R') + || (trackheader.track_id[2] != 'K')) Error() << "corrupt SCP file"; std::vector revs(_header.revolutions); diff --git a/lib/scp.h b/lib/scp.h index 77c373a4..01a96a8e 100644 --- a/lib/scp.h +++ b/lib/scp.h @@ -27,7 +27,7 @@ enum SCP_FLAG_FOOTER = (1<<5) }; -struct ScpTrackStart +struct ScpTrackHeader { char track_id[3]; // 'TRK' uint8_t strack; // SCP track number @@ -42,9 +42,8 @@ struct ScpTrackRevolution struct ScpTrack { - char track_id[3]; // 'TRK' - uint8_t strack; // SCP track number - struct ScpTrackRevolution revolution[5]; + ScpTrackHeader header; + ScpTrackRevolution revolution[5]; }; #endif