Fixing bug that triggered undefined behavior when writing to FAT chip

.gitignore

@@ -2,3 +2,4 @@
 tniasm.sym
 tniasm.tmp
 build/*
+debug/*

gui/src/config.h

@@ -22,7 +22,7 @@
 #define _CONFIG_H
 
 #define PROGRAM_NAME "P2000T FAT READER"
-#define PROGRAM_VERSION "0.2.1"
+#define PROGRAM_VERSION "0.3.0"
 #define ICON_PATH ":/assets/icon/icon_128px.png"
 
 #define UNUSED(x) (void)(x)

gui/src/fileallocationtable.cpp

@@ -289,7 +289,7 @@ unsigned int FileAllocationTable::get_number_occupied_blocks() const {
  * @param header
  * @param data
  */
-void FileAllocationTable::add_file(const QByteArray& header, const QByteArray& data) {
+bankblock FileAllocationTable::add_file(const QByteArray& header, const QByteArray& data) {
     qDebug() << "Adding file";
 
     char filename[16];
@@ -318,6 +318,7 @@ void FileAllocationTable::add_file(const QByteArray& header, const QByteArray& d
     qInfo() << "Starting looking for free blocks";
     emit(read_operation(0, nrblocks));
     auto newbankblock = this->find_next_free_block();
+    bankblock startpos = newbankblock;
 
     // loop over program blocks
     for(uint8_t i=0; i<nrblocks; i++) {
@@ -382,6 +383,58 @@ void FileAllocationTable::add_file(const QByteArray& header, const QByteArray& d
     }
 
     emit(signal_sync_needed());
+    return startpos;
+}
+
+/**
+ * @brief Check wether a file is valid
+ * @param startpos
+ */
+std::vector<std::pair<uint16_t, uint16_t>> FileAllocationTable::check_file(const bankblock& startpos) {
+    qDebug() << "Obtaining linked list for program";
+    auto bankblocks = this->build_linked_list_bankblock(startpos);
+
+    // invalidate cache for these blocks
+    for(const auto& bankblock : bankblocks) {
+        uint8_t bank = bankblock.first;
+        uint8_t block = bankblock.second;
+        qDebug() << "Invalidating bank " << bank << ", block " << block;
+        unsigned int dataoffset = 0x10000 * bank + 0x1000 + block * 0x400;
+        for(unsigned int i=0; i<4; i++) {
+            this->update_cache_status(dataoffset / 0x100 + i, CACHE_UNKNOWN);
+        }
+    }
+
+    // read blocks anew
+    for(const auto& bankblock : bankblocks) {
+        uint8_t bank = bankblock.first;
+        uint8_t block = bankblock.second;
+        qDebug() << "Reloading bank " << bank << ", block " << block;
+        unsigned int dataoffset = 0x10000 * bank + 0x1000 + block * 0x400;
+        for(unsigned int i=0; i<4; i++) {
+            this->read_block(dataoffset / 0x100 + i);
+        }
+    }
+
+    // perform checksum validation
+    std::vector<std::pair<uint16_t, uint16_t>> checksums;
+    for(const auto& bankblock : bankblocks) {
+        // retrieve stored checksum
+        uint8_t bank = bankblock.first;
+        uint8_t block = bankblock.second;
+        unsigned int meta_addr = bank * 0x10000 + block * 0x40 + 0x100;
+        this->read_block(meta_addr / 100);
+        uint16_t checksum = *((uint16_t*)&this->contents.data()[meta_addr + 0x06]);
+
+        // calculate actual checksum
+        unsigned int addr = bank * 0x10000 + 0x1000 + block * 0x400;
+        QByteArray data = QByteArray(&this->contents[addr], 0x400);
+        uint16_t crc16 = this->crc16_xmodem(data, 0x400);
+
+        checksums.emplace_back(checksum, crc16);
+    }
+
+    return checksums;
 }
 
 /**
@@ -584,6 +637,45 @@ void FileAllocationTable::build_linked_list(unsigned int id) {
     }
 }
 
+/**
+ * @brief Extract linked list of file
+ * @param vector of bank/block pairs
+ */
+std::vector<bankblock> FileAllocationTable::build_linked_list_bankblock(const bankblock& startpos) {
+    uint8_t bank = startpos.first;
+    uint8_t block = startpos.second;
+
+    std::vector<bankblock> blocks;
+    unsigned int counter = 0;
+
+    while(bank != 0xFF) {
+        counter++;
+        if(counter > 64 * 4) {
+            throw std::runtime_error("Error occured while collecting linked list. Most likely a cyclic reference.");
+        }
+
+        qDebug() << "Collecting: " << bank << " / " << block;
+
+        // insert into list
+        blocks.emplace_back(bank, block);
+
+        // set address locations for metadata of current block
+        unsigned int addr = bank * 0x10000 + 0x100 + block * 0x40;
+        unsigned int saddr = addr / 0x100;
+        unsigned int offset = addr % 0x100;
+
+        // read metadata of current block
+        QByteArray meta = this->read_block(saddr);
+        QByteArray metadata = meta.mid(offset, 0x40);
+
+        // retrieve data of new block
+        bank = metadata[0x03];  // next bank
+        block = metadata[0x04]; // next block
+    }
+
+    return blocks;
+}
+
 /**
  * @brief Attach file data to file object
  * @param file id

gui/src/fileallocationtable.h

@@ -25,6 +25,8 @@
 #include <memory>
 #include "serial_interface.h"
 
+typedef std::pair<uint8_t, uint8_t> bankblock;
+
 /**
  * @brief Class containing file metadata and data
  */
@@ -35,7 +37,7 @@ class File {
     uint16_t size;
     uint8_t startblock;
     uint8_t startbank;
-    std::vector<std::pair<uint8_t, uint8_t>> blocks;
+    std::vector<bankblock> blocks;
     std::vector<uint16_t> metachecksums;    // checksums as supplied in metadata
     std::vector<uint16_t> checksums;        // checksums from data
     QByteArray data;
@@ -173,14 +175,20 @@ class FileAllocationTable : public QObject {
      * @param header
      * @param data
      */
-    void add_file(const QByteArray& header, const QByteArray& data);
+    bankblock add_file(const QByteArray& header, const QByteArray& data);
 
     /**
      * @brief Remove a file from the ROM
      * @param file_id
      */
     void delete_file(unsigned int file_id);
 
+    /**
+     * @brief Check wether a file is valid
+     * @param startpos
+     */
+    std::vector<std::pair<uint16_t, uint16_t>> check_file(const bankblock& startpos);
+
     /**
      * @brief Get cached contents of the ROM chip
      * @return
@@ -205,6 +213,14 @@ class FileAllocationTable : public QObject {
         this->cache_status = _cache_status;
     }
 
+    /**
+     * @brief get_nr_banks
+     * @return
+     */
+    inline uint8_t get_nr_banks() const {
+        return this->nrbanks;
+    }
+
 signals:
     /**
      * @brief signal when a read operation is conducted
@@ -263,6 +279,12 @@ class FileAllocationTable : public QObject {
      */
     void build_linked_list(unsigned int id);
 
+    /**
+     * @brief Construct linked list from start position
+     * @param startpos
+     */
+    std::vector<bankblock> build_linked_list_bankblock(const bankblock& startpos);
+
     /**
      * @brief Attach file data to file object
      * @param file id

gui/src/mainwindow.cpp

@@ -93,6 +93,10 @@ MainWindow::MainWindow(const std::shared_ptr<QStringList> _log_messages, QWidget
  * @brief Default destructor method
  */
 MainWindow::~MainWindow() {
+    if(this->syncthread) {
+        this->syncthread->terminate();
+        this->syncthread->wait();
+    }
 }
 
 /**
@@ -624,8 +628,9 @@ void MainWindow::slot_add_program() {
             qDebug() << "Disabling all buttons";
             this->disable_all_buttons();
 
+            // add data to flash chip
             qDebug() << "Start adding data";
-            this->fat->add_file(header, romdata);
+            this->lastbankblock = this->fat->add_file(header, romdata);
         } else {
             return;
         }
@@ -922,6 +927,7 @@ void MainWindow::slot_start_sync() {
     qInfo() << "Received synchronization request";
     this->disable_all_buttons();
     this->syncthread = std::make_unique<SyncThread>(this->serial_interface);
+    this->syncthread->set_sectors(this->fat->get_nr_banks() * 0x10);
     syncthread->set_contents(this->fat->get_contents());
     syncthread->set_cache_status(this->fat->get_cache_status());
     connect(this->syncthread.get(), SIGNAL(sync_item_done(int, int)), this, SLOT(read_operation(int, int)));
@@ -934,14 +940,34 @@ void MainWindow::slot_sync_complete() {
     qInfo() << "Synchronization process completed";
     this->fat->set_cache_status(this->syncthread->get_cache_status());
     this->index_files();
-    this->enable_all_buttons();
+
     this->syncthread.release();
+
+    if(this->lastbankblock.first != 0xFF) {
+        auto checksums = this->fat->check_file(this->lastbankblock);
+        unsigned int wrongblocks = 0;
+        for(const auto& i : checksums) {
+            qDebug() << i.first << "\t" << i.second;
+            if(i.first != i.second) {
+                wrongblocks++;
+            }
+        }
+
+        if(wrongblocks > 0) {
+            this->raise_error_window(QMessageBox::Critical,
+                                     tr("%1 blocks were not flashed correctly. Please ensure the chip is properly socketed in the ZIF"
+                                     " socket, delete the last written file and try again.").arg(wrongblocks));
+        }
+    }
+
+    this->enable_all_buttons();
+    this->lastbankblock = {0xFF,0xFF};
 }
 
 /**
  * @brief Update synchronization map
  */
 void MainWindow::slot_update_syncmap(const std::vector<uint8_t> _cache_status) {
-    qDebug() << "Updating syncmap";
+//    qDebug() << "Updating syncmap";
     this->syncmap->set_cache(_cache_status);
 }

gui/src/mainwindow.h

@@ -111,6 +111,7 @@ class MainWindow : public QMainWindow
     QLabel* label_checksums;
     BlockMap* blockmap;
     BlockMap* syncmap;
+    bankblock lastbankblock = {0xFF,0xFF};
 
     static const unsigned int FILETABLE_OPEN_COLUMN = 4;
     static const unsigned int FILETABLE_DELETE_COLUMN = 5;

gui/src/serial_interface.cpp

@@ -47,7 +47,7 @@ void SerialInterface::open_port() {
     this->port->open(QIODevice::ReadWrite);
     this->port->setDataTerminalReady(true);
 
-    qDebug() << QObject::tr("Opening COM port:") + QObject::tr(this->portname.c_str());
+    qDebug() << "Opening COM port: " + QObject::tr(this->portname.c_str());
 }
 
 /**

gui/src/serial_interface.h

@@ -41,8 +41,8 @@
 class SerialInterface {
 
 private:
-    static const unsigned int SERIAL_TIMEOUT = 100;             // timeout for regular serial communication
-    static const unsigned int SERIAL_TIMEOUT_SECTOR = 0;        // timeout when reading sector data (0x1000 bytes)
+    static const unsigned int SERIAL_TIMEOUT = 120;             // timeout for regular serial communication
+    static const unsigned int SERIAL_TIMEOUT_SECTOR = 10;       // timeout when reading sector data (0x1000 bytes)
     static const unsigned int SERIAL_TIMEOUT_BLOCK = 3000;      // timeout when reading sector data (0x1000 bytes)
     std::string portname;                                       // communication port address
     std::unique_ptr<QSerialPort> port;                          // pointer to QSerialPort object

gui/src/syncthread.cpp

@@ -23,13 +23,11 @@
 /**
  * @brief run cart flash routine
  */
-void SyncThread::run() {
-    // maximum number of sectors
-    static const unsigned int nrsects = 128;
-
+void SyncThread::run() {   
+    qDebug() << "Starting syncthread";
     // determine number of sectors to write
     unsigned int nrsectowrite = 0;
-    for(unsigned int i=0; i<nrsects; i++) { // loop over sectors
+    for(unsigned int i=0; i<this->nr_sectors; i++) { // loop over sectors
         for(unsigned int j=0; j<0x10; j++) {
             if(this->cache_status[i * 0x10 + j] == 0x02 ||
                this->cache_status[i * 0x10 + j] == 0x03) {
@@ -39,8 +37,10 @@ void SyncThread::run() {
         }
     }
 
+    qDebug() << "Where?";
+
     unsigned int nrsects_written = 0;
-    for(unsigned int i=0; i<nrsects; i++) { // loop over sectors
+    for(unsigned int i=0; i<this->nr_sectors; i++) { // loop over sectors
         for(unsigned int j=0; j<0x10; j++) {
 
             // if at least one sector page has a - to be written - flag, write
@@ -59,6 +59,7 @@ void SyncThread::run() {
                 this->serial_interface->open_port();
                 this->serial_interface->burn_sector(i, QByteArray(&this->contents[i * 0x1000], 0x1000));
                 this->serial_interface->close_port();
+                qDebug() << "Done writing sector: " << i;
 
                 // emit status update
                 emit(sync_item_done(++nrsects_written, nrsectowrite));
@@ -78,3 +79,7 @@ void SyncThread::run() {
     qDebug() << "Done syncing.";
     emit(sync_complete());
 }
+
+SyncThread::~SyncThread() {
+    qDebug() << "Destroying Synchronization Thread";
+}

gui/src/syncthread.h

@@ -41,6 +41,7 @@ class SyncThread : public IOWorker {
     // enable caching
     std::vector<char> contents;
     std::vector<uint8_t> cache_status;
+    unsigned int nr_sectors = 0;
 
 public:
     /**
@@ -67,11 +68,17 @@ class SyncThread : public IOWorker {
         return this->cache_status;
     }
 
+    inline void set_sectors(unsigned int _nr_sectors) {
+        this->nr_sectors = _nr_sectors;
+    }
+
     /**
      * @brief run cart flash routine
      */
     void run() override;
 
+    ~SyncThread();
+
 private:
 
 signals: