From 78eeed2b482829f023d3aaf42a9efac8937ac0c2 Mon Sep 17 00:00:00 2001 From: jbenedictocenteno Date: Fri, 13 Nov 2026 14:13:42 +0100 Subject: [PATCH 1/4] fix(kaos): Write an evicted app's copy under a temporary name The keep round wrote /sdcard/.bin in place. A reset or a pulled card in the middle left a cut image under the app's name, listed as an app to install and taken by the next round for the app's copy. The copy is written as .bin.tmp and put on its name with one new AO_Storage request, STORAGE_PAIR_REPLACE. FAT renames onto no name that is taken, so AO_Storage moves the file there to .bin.bak, puts the copy on the name and unlinks the backup, and puts the original back if a step fails: one step, one answer. AO_AppMgr asks once and reads one answer. The name never holds a cut file. A replacement that is refused, fails or is never answered fails the copy and leaves the file that was there on the name; the temporary is unlinked unless the answer is missing. A reset between the two renames leaves that file as .bin.bak and the copy as .bin.tmp; the next replacement of the name removes the backup, and says so, as it does for any .bin.bak it finds. Closes #104 --- components/kaos/appmgr/src/appmgr_ao.c | 199 +++++++++++++++++----- components/kaos/core/include/kaos.h | 22 ++- components/kaos/storage/src/storage_ao.c | 107 +++++++++++- tests/host/cases/test_appmgr_ao.c | 208 +++++++++++++++++++---- tests/host/cases/test_storage_ao.c | 180 ++++++++++++++++++++ 5 files changed, 633 insertions(+), 83 deletions(-) diff --git a/components/kaos/appmgr/src/appmgr_ao.c b/components/kaos/appmgr/src/appmgr_ao.c index bdaf84e..5fd2732 100644 --- a/components/kaos/appmgr/src/appmgr_ao.c +++ b/components/kaos/appmgr/src/appmgr_ao.c @@ -70,6 +70,7 @@ /* Region bytes read per step while an evicted app is copied out of the * region: the same 32 KB the writes use, so a step stays short (rule 5). */ #define APPMGR_KEEP_CHUNK (32U * 1024U) +#define APPMGR_KEEP_TMP_SUFFIX ".tmp" /**< What a copy's name carries while it is written. */ /* Re-publish the percent this often while the card writes a copy. AO_Power * takes the LED's orange pulse back after three seconds without a report, and * a multi-MB write has none of its own. */ @@ -377,12 +378,14 @@ typedef struct { bool keep_forced; /**< Adoption had dst forced. */ bool keep_writing; /**< AO_Storage holds keep_buf. */ bool keep_stat; /**< A STAT is out, unanswered. */ + bool keep_moving; /**< The copy's replacement of its name is out, unanswered. */ /* Answers still owed for requests given up on at their deadline. * AO_Storage answers in order and its answers carry no path, so the * next that many answers of that kind are the abandoned requests', not * the victim's in hand. Kept across rounds: a late answer can outlive * the install it belonged to. */ uint8_t keep_stale_stat; /**< STATs abandoned, owed. */ + uint8_t keep_stale_move; /**< Replacements abandoned, owed. */ uint8_t keep_stale_write; /**< Writes abandoned, owed. */ uint8_t *keep_orphan; /**< The buffer of the oldest * abandoned write, freed when @@ -952,6 +955,7 @@ static void keep_clear(AppMgr * const me) me->keep_forced = false; me->keep_writing = false; me->keep_stat = false; + me->keep_moving = false; me->keep_count = 0U; me->keep_idx = 0U; me->keep_total = 0U; @@ -3342,6 +3346,7 @@ void AppMgr_ctor(void) me->sink = NULL; me->chunk = NULL; me->keep_stale_stat = 0U; + me->keep_stale_move = 0U; me->keep_stale_write = 0U; me->keep_orphan = NULL; me->ask_who = NULL; @@ -4378,8 +4383,9 @@ static const kapps_entry_t *keep_entry(uint32_t seq) } /** - * @brief Settle an answer owed to a STAT or a write given up on, if @p e is - * one; the caller has checked it is a path or write answer. + * @brief Settle an answer owed to a STAT, a replacement or a write given up + * on, if @p e is one; the caller has checked it is a path or write + * answer. * @return true when @p e was such a late answer and is now spent. */ static bool keep_owed_answer(AppMgr * const me, QEvt const * const e) @@ -4393,6 +4399,13 @@ static bool keep_owed_answer(AppMgr * const me, QEvt const * const e) --me->keep_stale_stat; spent = true; } + else if ( ((QSignal)STORAGE_PATH_RSP_SIG == e->sig) + && ((uint8_t)STORAGE_PAIR_REPLACE == Q_EVT_CAST(StoragePathRspEvt)->op) + && (0U < me->keep_stale_move) ) + { + --me->keep_stale_move; + spent = true; + } else if ( ((QSignal)STORAGE_WRITE_RSP_SIG == e->sig) && (0U < me->keep_stale_write) ) { @@ -4425,6 +4438,7 @@ static void keep_settle(AppMgr * const me, keep_res_t res) } me->keep_writing = false; me->keep_stat = false; + me->keep_moving = false; (void)QTimeEvt_disarm(&me->idle); /* the victim's answer deadline */ ++me->keep_idx; if ( progress_pct(me) > me->reported ) @@ -4445,6 +4459,20 @@ static void card_copy_path(char *out, size_t cap, const char *project) project); } +/** + * @brief Write the path a copy of @p project is written to before it is put + * on its name into @p out. + * @param out Receives `/sdcard/.bin.tmp`. + * @param cap Its size in bytes. + * @param project The app's project name. + */ +static void card_tmp_path(char *out, size_t cap, const char *project) +{ + (void)snprintf(out, cap, + KODE_CARD_APP_PREFIX "%s" KODE_CARD_APP_SUFFIX + APPMGR_KEEP_TMP_SUFFIX, project); +} + /** * @brief Ask AO_Storage whether @p path is on the card, and start the clock * on its answer. @@ -4546,6 +4574,50 @@ static bool keep_next(AppMgr * const me) return false; } +/** @brief Count an answer given up on, never past the count's end. */ +static void stale_add(uint8_t * const count) +{ + if ( UINT8_MAX > *count ) + { + ++*count; + } +} + +/** + * @brief Ask AO_Storage to put the finished copy on the app's name. + * + * One request, whatever is on that name: AO_Storage moves it aside, puts the + * copy there and removes it, and puts it back when a step fails. One answer, + * a path answer of op STORAGE_PAIR_REPLACE; keep_moving says it is awaited. + * + * @param me The AO, with the copy whole under its temporary name. + */ +static void keep_move(AppMgr * const me) +{ + char tmp[KODE_APP_FILE_MAX]; + + card_tmp_path(tmp, sizeof(tmp), me->keep_name); + storage_pair_request(&me->super, (uint8_t)STORAGE_PAIR_REPLACE, tmp, me->keep_path); + me->keep_moving = true; + (void)QTimeEvt_rearm(&me->idle, APPMGR_OPEN_TICKS); +} + +/** + * @brief Ask the card to remove the victim in hand's temporary. + * + * Its answer is not waited for: a temporary that stays is overwritten by the + * next copy of the app and is never listed as one. + * + * @param me The AO. + */ +static void keep_drop_tmp(AppMgr * const me) +{ + char tmp[KODE_APP_FILE_MAX]; + + card_tmp_path(tmp, sizeof(tmp), me->keep_name); + storage_path_request(&me->super, (uint8_t)STORAGE_PATH_UNLINK, tmp); +} + /** @brief Every victim tried: cancel (rule 5) or restart. */ static QState keep_finish(AppMgr * const me) { @@ -4574,11 +4646,22 @@ static QState keep_finish(AppMgr * const me) return Q_TRAN(&AppMgr_idle); } +/** @brief Go on to the next victim after one was settled, or finish the round. */ +static QState keep_on(AppMgr * const me) +{ + if ( !keep_next(me) ) + { + return keep_finish(me); + } + return Q_HANDLED(); +} + /** * @brief Copy each victim to the card, one at a time, then restart. * - * Per victim: STAT its path, and when it is not there, read its extent in - * APPMGR_KEEP_CHUNK steps and hand them to AO_Storage. A failure is + * Per victim: STAT its path, and when its copy is not there, read its extent + * in APPMGR_KEEP_CHUNK steps, hand it to AO_Storage to write under a temporary + * name, and ask AO_Storage to put that file on the app's name. A failure is * KEEP_FAILED, and the round goes on. */ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) @@ -4612,7 +4695,6 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) case APP_RECV_IDLE_SIG: { /* AO_Storage dropped a request on a dry pool, or the card never * answered: the victim is a failed copy and the round goes on. */ - status = Q_HANDLED(); if ( me->keep_writing ) { KLOG_E(TAG, "the card never answered for '%s'; its buffer is " @@ -4632,29 +4714,59 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) ++me->keep_stale_write; } } + else if ( me->keep_moving ) + { + KLOG_W(TAG, "nothing answered about '%s' on the card", + me->keep_name); + stale_add(&me->keep_stale_move); + } else if ( me->keep_stat ) { KLOG_W(TAG, "nothing answered about '%s' on the card", me->keep_name); - if ( UINT8_MAX > me->keep_stale_stat ) - { - ++me->keep_stale_stat; - } + stale_add(&me->keep_stale_stat); } else { + status = Q_HANDLED(); break; /* a stale expiry */ } keep_settle(me, KEEP_FAILED); - if ( !keep_next(me) ) - { - status = keep_finish(me); - } + status = keep_on(me); break; } case STORAGE_PATH_RSP_SIG: { StoragePathRspEvt const * const r = Q_EVT_CAST(StoragePathRspEvt); status = Q_HANDLED(); + if ( keep_owed_answer(me, e) ) + { + break; /* a STAT or replacement given up on, not this victim's */ + } + if ( (uint8_t)STORAGE_PAIR_REPLACE == r->op ) + { + if ( me->keep_moving ) + { + (void)QTimeEvt_disarm(&me->idle); + if ( (uint8_t)KODE_OK == r->rc ) + { + KLOG_N(TAG, "'%s' kept as %s", me->keep_name, me->keep_path); + keep_settle(me, KEEP_SAVED); + } + else + { + /* The file on the name, if there was one, is as it was. */ + KLOG_W(TAG, "'%s' could not be put on its name", me->keep_name); + keep_drop_tmp(me); + keep_settle(me, KEEP_FAILED); + } + status = keep_on(me); + } + else + { + KLOG_D(TAG, "a replacement's answer nobody is waiting for"); + } + break; + } if ( (uint8_t)STORAGE_PATH_STAT != r->op ) { /* The answer to the unlink a failed copy asked for: it @@ -4662,10 +4774,6 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) KLOG_D(TAG, "a path answer that is not a stat"); break; } - if ( keep_owed_answer(me, e) ) - { - break; /* a STAT given up on, not this victim's */ - } if ( !me->keep_stat ) { KLOG_D(TAG, "an answer about a path nobody is waiting for"); @@ -4695,7 +4803,7 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) if ( r->exists ) { /* Another file of the name: an older version, or not the - * app at all. The copy truncates it. */ + * app at all. The copy replaces it once it is whole. */ KLOG_N(TAG, "'%s' is %u bytes and the image is %u; the " "app is copied over it", me->keep_path, (unsigned)r->size, (unsigned)me->keep_len); @@ -4770,8 +4878,10 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) break; } me->keep_writing = true; - storage_write_all_request(me->keep_path, me->keep_buf, - me->keep_len, &me->super); + char tmp[KODE_APP_FILE_MAX]; + + card_tmp_path(tmp, sizeof(tmp), me->keep_name); + storage_write_all_request(tmp, me->keep_buf, me->keep_len, &me->super); (void)QTimeEvt_rearm(&me->idle, APPMGR_KEEP_WRITE_TICKS(me->keep_len)); (void)QTimeEvt_rearm(&me->pacer, APPMGR_KEEP_BEAT_TICKS); @@ -4792,26 +4902,27 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) break; } (void)QTimeEvt_disarm(&me->pacer); - if ( (uint8_t)KODE_OK == w->status ) - { - KLOG_N(TAG, "'%s' kept as %s", me->keep_name, me->keep_path); - keep_settle(me, KEEP_SAVED); - } - else - { - /* The path was absent when asked about, so whatever is - * there now is this copy's stub (a close that failed leaves - * the file): remove it, or the next round would find it and - * call the app kept. Its answer is not waited for. */ - storage_path_request(&me->super, (uint8_t)STORAGE_PATH_UNLINK, - me->keep_path); - keep_settle(me, KEEP_FAILED); - } status = Q_HANDLED(); - if ( !keep_next(me) ) - { - status = keep_finish(me); + /* The answer says AO_Storage is done with the buffer. */ + khal_free(me->keep_buf); + me->keep_buf = NULL; + me->keep_writing = false; + if ( (uint8_t)KODE_OK != w->status ) + { + /* Whatever is under the temporary name is this copy's stub + * (a close that failed leaves the file): remove it. */ + keep_drop_tmp(me); + keep_settle(me, KEEP_FAILED); + status = keep_on(me); + break; } + /* The copy is whole under its temporary name. It becomes the + * app's copy when it is on the app's name, in one step in + * AO_Storage whatever is there: a refusal or a failure leaves the + * file that was there on the name. A reset in the middle leaves + * it as `.bak` and the copy under its temporary name, + * never a cut file on the name. */ + keep_move(me); break; } case FLASH_REQ_SIG: @@ -5076,13 +5187,13 @@ static bool card_answer_taken(AppMgr * const me, QEvt const * const e, { StoragePathRspEvt const * const r = Q_EVT_CAST(StoragePathRspEvt); - if ( (uint8_t)STORAGE_PATH_STAT != r->op ) + if ( keep_owed_answer(me, e) ) { - return false; + return false; /* a STAT or replacement given up on, not this question's */ } - if ( keep_owed_answer(me, e) || (!*stat) ) + if ( ((uint8_t)STORAGE_PATH_STAT != r->op) || (!*stat) ) { - return false; /* a STAT given up on, not this question's */ + return false; } *stat = false; (void)QTimeEvt_disarm(&me->idle); @@ -5780,8 +5891,8 @@ static const char *request_name(QSignal sig) * @brief The parent of every state with work in hand. * * Answers a question about the next install with `busy`, and settles the late - * AO_Storage answers owed to a STAT or a write given up on, for every child - * that does not take them itself. + * AO_Storage answers owed to a STAT, a replacement or a write given up on, for + * every child that does not take them itself. */ static QState AppMgr_busy(AppMgr * const me, QEvt const * const e) { diff --git a/components/kaos/core/include/kaos.h b/components/kaos/core/include/kaos.h index e914ddb..e49981c 100644 --- a/components/kaos/core/include/kaos.h +++ b/components/kaos/core/include/kaos.h @@ -3042,6 +3042,16 @@ typedef enum { * 32 KB chunk per step paced by a QTimeEvt, so * lower AOs keep running. Answered once at the * end; a failed copy leaves no destination. */ + STORAGE_PAIR_REPLACE, /**< Put @p src on @p dst, which may be taken: the + * file there goes to `.bak` first, is put + * back if a step fails and is unlinked when the + * copy is in place, all in one step with one + * answer. Same volume only. @p src stays where it + * was when the replacement fails. A `.bak` + * already there is removed. A reset between the + * steps leaves the file that was there as + * `.bak` and @p src where it was, never a + * cut file on @p dst. */ } storage_pair_op_t; /** @@ -3060,18 +3070,22 @@ typedef enum { void storage_path_request(QActive *who, uint8_t op, const char *path); /** - * @brief Ask AO_Storage to move or copy @p src to @p dst. + * @brief Ask AO_Storage to move or copy @p src to @p dst, or to replace @p dst by it. * * Answered once with StoragePathRspEvt; `rc` says how it went. On KODE_ERROR, * `exists` true means the source was checked afterwards and is missing. * MOVE waits for a transfer in flight like storage_path_request()'s deletes. * MOVE: KODE_DENIED malformed, no volume, two volumes, or a transfer that outlasted - * the wait; KODE_ERROR no mount or rename failed. COPY: KODE_DENIED malformed, no volume, same file, or a transfer in + * the wait; KODE_ERROR no mount or rename failed. REPLACE waits for a transfer in + * flight as MOVE does and is refused as it is, and with KODE_DENIED when a folder + * is on @p dst; KODE_ERROR when a step failed, the file on @p dst then as it was + * unless it would not go back, which the log says with both names. COPY: KODE_DENIED malformed, no volume, same file, or a transfer in * flight; KODE_ERROR otherwise. COPY answers at its end: size deadlines for it. * * @param who Where the response is posted. NULL is served and not answered for - * MOVE; refused for COPY, whose only sign of life is @p who. - * @param op storage_pair_op_t: STORAGE_PAIR_MOVE or STORAGE_PAIR_COPY. + * MOVE and REPLACE; refused for COPY, whose only sign of life is @p who. + * @param op storage_pair_op_t: STORAGE_PAIR_MOVE, _COPY or _REPLACE. The answer's + * `op` is the same. * @param src Absolute, normalised, virtual. Copied into the event. * @param dst Absolute, normalised, virtual. Copied into the event. */ diff --git a/components/kaos/storage/src/storage_ao.c b/components/kaos/storage/src/storage_ao.c index 2c5bfa4..3c4220b 100644 --- a/components/kaos/storage/src/storage_ao.c +++ b/components/kaos/storage/src/storage_ao.c @@ -3532,6 +3532,92 @@ static void answer_pair_move(QActive *who, uint8_t op, const char *src, post_path_rsp(who, op, rc, src_missing, false, 0U); } +/** + * @brief Answer a STORAGE_PAIR_REPLACE: put @p src on @p dst, which may be taken. + * + * FatFs f_rename() will not overwrite, so a taken name is moved aside to + * `.bak`, @p src is put on it and the backup unlinked, as + * answer_text_line_write() does for a rewrite; a step that fails puts the + * original back. One dispatch step, one answer. A reset between the two + * renames leaves the name free, the original as `.bak` and @p src where it + * was: the next replacement of the name puts its copy there and removes that + * backup. A `.bak` that is there is removed, and said. + */ +static void answer_pair_replace(QActive *who, uint8_t op, const char *src, + const char *dst) +{ + char bak[KODE_PATH_MAX]; + kdrv_stat_t st = { .exists = false, .is_dir = false, .size = 0U }; + kode_status_t rc; + + warn_if_card("replace", dst); + int const wrote = snprintf(bak, sizeof(bak), "%s.bak", dst); + if ( (0 > wrote) || ((size_t)wrote >= sizeof(bak)) ) + { + KLOG_W(TAG, "replace '%s': no room for a backup name", dst); + post_path_rsp(who, op, KODE_DENIED, false, false, 0U); + return; + } + if ( (kdrv_storage_stat(dst, &st) != KODE_OK) || !st.exists ) + { + /* The name is free (or the volume is not there, which the rename + * says): the one rename is the whole replacement. */ + rc = kdrv_storage_rename(src, dst); + if ( KODE_OK != rc ) + { + KLOG_W(TAG, "replace '%s' -> '%s' failed", src, dst); + } + else if ( kdrv_storage_unlink(bak) == KODE_OK ) + { + KLOG_N(TAG, "replace '%s': '%s' was there and is removed", dst, bak); + } + else + { + /* no backup in the way: the usual case */ + } + post_path_rsp(who, op, rc, false, false, 0U); + return; + } + if ( st.is_dir ) + { + KLOG_W(TAG, "replace '%s': a folder is on the name", dst); + post_path_rsp(who, op, KODE_DENIED, false, false, 0U); + return; + } + if ( kdrv_storage_unlink(bak) == KODE_OK ) + { + KLOG_N(TAG, "replace '%s': '%s' was there and is removed", dst, bak); + } + rc = kdrv_storage_rename(dst, bak); + if ( KODE_OK != rc ) + { + KLOG_W(TAG, "replace '%s': could not move the file there aside", dst); + post_path_rsp(who, op, KODE_ERROR, false, false, 0U); + return; + } + rc = kdrv_storage_rename(src, dst); + if ( KODE_OK != rc ) + { + if ( kdrv_storage_rename(bak, dst) == KODE_OK ) + { + KLOG_W(TAG, "replace '%s' -> '%s' failed; the file there is put back", + src, dst); + } + else + { + KLOG_E(TAG, "replace '%s' failed and the file there would not go " + "back: it stays as '%s', the copy as '%s'", dst, bak, src); + } + post_path_rsp(who, op, KODE_ERROR, false, false, 0U); + return; + } + if ( kdrv_storage_unlink(bak) != KODE_OK ) + { + KLOG_W(TAG, "replace '%s' done, backup '%s' left behind", dst, bak); + } + post_path_rsp(who, op, KODE_OK, false, false, 0U); +} + /* ---- cp ------------------------------------------------------------------- * The only thing in kodeOS that holds a reader and a writer at once. One * 32 KB chunk per dispatch step with a QTimeEvt between, so AO_Gui, AO_Radio @@ -4368,8 +4454,8 @@ static void handle_path_req(Storage * const me, const StoragePathEvt *q) /** * @brief Dispatch one StoragePairEvt by storage_pair_op_t. * - * MOVE is one rename() answered in this step; COPY holds two handles and - * answers many steps later. The default refuses, as in handle_path_req(). + * MOVE and REPLACE are renames answered in this step; COPY holds two handles + * and answers many steps later. The default refuses, as in handle_path_req(). */ static void handle_pair_req(Storage * const me, const StoragePairEvt *q) { @@ -4388,6 +4474,17 @@ static void handle_pair_req(Storage * const me, const StoragePairEvt *q) answer_pair_move(q->who, q->op, q->src, q->dst); break; } + case STORAGE_PAIR_REPLACE: { + if ( busy(me) ) + { + KLOG_W(TAG, "a transfer is already in flight; replace of '%s' " + "refused", q->dst); + post_path_rsp(q->who, q->op, KODE_DENIED, false, false, 0U); + break; + } + answer_pair_replace(q->who, q->op, q->src, q->dst); + break; + } case STORAGE_PAIR_COPY: { answer_pair_copy(me, q->who, q->op, q->src, q->dst); break; @@ -7624,8 +7721,10 @@ static bool turn_wanted(QEvt const * const e) break; } case STORAGE_PAIR_REQ_SIG: { - wanted = ((storage_pair_op_t)Q_EVT_CAST(StoragePairEvt)->op - == STORAGE_PAIR_MOVE); + storage_pair_op_t const op = + (storage_pair_op_t)Q_EVT_CAST(StoragePairEvt)->op; + + wanted = (STORAGE_PAIR_MOVE == op) || (STORAGE_PAIR_REPLACE == op); break; } default: { diff --git a/tests/host/cases/test_appmgr_ao.c b/tests/host/cases/test_appmgr_ao.c index 10b13f9..91372e3 100644 --- a/tests/host/cases/test_appmgr_ao.c +++ b/tests/host/cases/test_appmgr_ao.c @@ -2580,12 +2580,13 @@ typedef struct { unsigned open; /**< STORAGE_OPEN_REQ_SIG */ unsigned path; /**< STORAGE_PATH_REQ_SIG */ unsigned write; /**< STORAGE_WRITE_REQ_SIG */ + unsigned pair; /**< STORAGE_PAIR_REQ_SIG */ } storage_seen_t; /** @brief Count what AO_Storage's queue holds, and empty it. */ static storage_seen_t storage_tally(void) { - storage_seen_t seen = { 0U, 0U, 0U }; + storage_seen_t seen = { 0U, 0U, 0U, 0U }; const QEvt *e = NULL; while ( xQueueReceive(AO_Storage->eQueue, &e, 0U) == pdTRUE ) @@ -2602,6 +2603,10 @@ static storage_seen_t storage_tally(void) { ++seen.write; } + if ( (enum_t)e->sig == STORAGE_PAIR_REQ_SIG ) + { + ++seen.pair; + } QF_gc(e); } return seen; @@ -2688,6 +2693,46 @@ static void answer_write(uint8_t st, uint32_t len) QASM_DISPATCH(AO_AppMgr, &w.super, 0U); } +/** @brief Take AO_Storage's next path request and check what it asks. */ +static void path_req_expect(uint8_t op, const char *path, const char *what) +{ + const QEvt * const e = storage_take(STORAGE_PATH_REQ_SIG); + + TEST_ASSERT_NOT_NULL_MESSAGE(e, what); + TEST_ASSERT_EQUAL_UINT8_MESSAGE(op, ((StoragePathEvt const *)e)->op, what); + TEST_ASSERT_EQUAL_STRING_MESSAGE(path, ((StoragePathEvt const *)e)->path, what); + QF_gc(e); +} + +/** @brief Take AO_Storage's next replacement request and check what it asks. */ +static void pair_req_expect(const char *src, const char *dst, const char *what) +{ + const QEvt * const e = storage_take(STORAGE_PAIR_REQ_SIG); + + TEST_ASSERT_NOT_NULL_MESSAGE(e, what); + TEST_ASSERT_EQUAL_UINT8_MESSAGE((uint8_t)STORAGE_PAIR_REPLACE, + ((StoragePairEvt const *)e)->op, what); + TEST_ASSERT_EQUAL_STRING_MESSAGE(src, ((StoragePairEvt const *)e)->src, what); + TEST_ASSERT_EQUAL_STRING_MESSAGE(dst, ((StoragePairEvt const *)e)->dst, what); + QF_gc(e); +} + +/** @brief Answer AppMgr's replacement of its name by a finished copy. */ +static void answer_replace(uint8_t rc) +{ + answer_path((uint8_t)STORAGE_PAIR_REPLACE, rc, false, false, 0U); +} + +/** @brief Answer a copy's write as done, and the replacement of its name that follows. */ +static void answer_write_landed(uint32_t len) +{ + answer_write((uint8_t)KODE_OK, len); + const QEvt * const mv = storage_take(STORAGE_PAIR_REQ_SIG); + TEST_ASSERT_NOT_NULL_MESSAGE(mv, "the finished copy was never put on its name"); + QF_gc(mv); + answer_replace((uint8_t)KODE_OK); +} + /** @brief Answer the open of the install's source file. */ static void answer_open_with(uint32_t size, uint8_t medium, uint8_t st) { @@ -3126,13 +3171,24 @@ TEST_CASE("a victim missing from the card is written there before it goes", answer_stat(false, (uint8_t)KODE_OK); const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); /* the read, 32 KB a tick */ TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the copy never reached AO_Storage"); - TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin", ((StorageWriteEvt const *)wr)->name); + TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin.tmp", ((StorageWriteEvt const *)wr)->name); TEST_ASSERT_EQUAL_UINT32(old_bytes(), ((StorageWriteEvt const *)wr)->len); QF_gc(wr); TEST_ASSERT_TRUE(old_installed()); TEST_ASSERT_TRUE(klog_has("keeping 'old' on the card before it is evicted")); + /* The write landed under its temporary name: the copy is not its app's + * until it is put on the app's name, and nothing may start before. */ answer_write((uint8_t)KODE_OK, old_bytes()); + (void)qf_support_pump(AO_AppMgr, 8U); + pair_req_expect("/sdcard/old.bin.tmp", "/sdcard/old.bin", + "the finished copy was not put on its name"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, storage_tally().open, + "the install started again with the copy still under its temporary name"); + TEST_ASSERT_FALSE_MESSAGE(klog_has("'old' kept as"), + "the copy was called kept before it was on its name"); + + answer_replace((uint8_t)KODE_OK); /* The restart is the same request, posted to itself. */ (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, @@ -3143,6 +3199,50 @@ TEST_CASE("a victim missing from the card is written there before it goes", keep_end(); } +/* A replacement the card refuses or fails leaves the file on the name where + * it was: the copy is no copy, its temporary is unlinked, and an install from + * elsewhere goes on without it. */ +static void keep_replacement_fails(uint8_t rc) +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + (void)begin_install((uint8_t)KDRV_MEDIUM_FS, "new"); + storage_expect(STORAGE_PATH_REQ_SIG, "the victim's path was never asked about"); + answer_copy_of(old_bytes() / 2U); /* an older file is in the way */ + const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); + TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the copy never reached AO_Storage"); + QF_gc(wr); + + answer_write((uint8_t)KODE_OK, old_bytes()); + pair_req_expect("/sdcard/old.bin.tmp", "/sdcard/old.bin", + "the finished copy was not put on its name"); + answer_replace(rc); + path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/old.bin.tmp", + "the temporary that could not be put on its name was left on the card"); + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); + TEST_ASSERT_FALSE_MESSAGE(klog_has("'old' kept as"), + "a copy that never reached its name was called kept"); + TEST_ASSERT_TRUE(klog_has("could not be put on its name")); + TEST_ASSERT_TRUE(old_installed()); + + let_the_open_expire(); + keep_end(); +} + +TEST_CASE("a replacement the card refuses is a failed copy, and its temporary is unlinked", + "[appmgr_ao][keep]") +{ + keep_replacement_fails((uint8_t)KODE_DENIED); +} + +TEST_CASE("a replacement that fails is a failed copy, and its temporary is unlinked", + "[appmgr_ao][keep]") +{ + keep_replacement_fails((uint8_t)KODE_ERROR); +} + TEST_CASE("a victim already on the card is not copied again", "[appmgr_ao][keep]") { @@ -3186,13 +3286,23 @@ TEST_CASE("a file of the victim's name at another length is copied over, not tak "the file of another length went unsaid"); const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the app was not copied over the file of another length"); - TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin", ((StorageWriteEvt const *)wr)->name); + TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin.tmp", ((StorageWriteEvt const *)wr)->name); TEST_ASSERT_EQUAL_UINT32(old_bytes(), ((StorageWriteEvt const *)wr)->len); QF_gc(wr); TEST_ASSERT_TRUE_MESSAGE(old_installed(), "nothing may be evicted before its copy lands"); + /* FAT renames onto no name that is taken: AO_Storage takes the older + * file out of the way in the one request, so AppMgr asks for nothing + * else, and only once the whole copy is in the temporary. */ answer_write((uint8_t)KODE_OK, old_bytes()); (void)qf_support_pump(AO_AppMgr, 8U); + storage_seen_t const seen = storage_tally(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(1U, seen.pair, + "the finished copy was not put on its name in one request"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.path, + "the older file was dealt with outside the one request"); + answer_replace((uint8_t)KODE_OK); + (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the copy"); TEST_ASSERT_TRUE(klog_has("'old' kept as /sdcard/old.bin")); @@ -3218,7 +3328,7 @@ TEST_CASE("the file of another length is said with its bytes and the image's", const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the app was not copied"); QF_gc(wr); - answer_write((uint8_t)KODE_OK, old_bytes()); + answer_write_landed(old_bytes()); (void)qf_support_pump(AO_AppMgr, 8U); let_the_open_expire(); keep_end(); @@ -3245,6 +3355,8 @@ TEST_CASE("a failed copy cancels an install read from the card, and evicts nothi TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the copy never reached AO_Storage"); QF_gc(wr); answer_write((uint8_t)KODE_ERROR, 0U); + path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/old.bin.tmp", + "the stub of the failed write was left on the card"); run_ticks(4U); for ( const QEvt *e = ao_spy_take(); NULL != e; e = ao_spy_take() ) @@ -4997,7 +5109,7 @@ TEST_CASE("a request while copies run is refused", "[appmgr_ao][keep]") const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL(wr); QF_gc(wr); - answer_write((uint8_t)KODE_OK, old_bytes()); + answer_write_landed(old_bytes()); (void)qf_support_pump(AO_AppMgr, 8U); TEST_ASSERT_EQUAL_UINT_MESSAGE(1U, storage_tally().open, "the restart was not the one install it should have been"); @@ -5085,7 +5197,7 @@ TEST_CASE("the bar does not go quiet while the card writes", "[appmgr_ao][keep]" TEST_ASSERT_TRUE_MESSAGE(2U <= beats, "the bar and the LED pulse went quiet for three seconds"); - answer_write((uint8_t)KODE_OK, old_bytes()); + answer_write_landed(old_bytes()); (void)qf_support_pump(AO_AppMgr, 8U); let_the_open_expire(); keep_end(); @@ -5204,10 +5316,10 @@ TEST_CASE("two victims: one already kept, one copied, one bar", const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the second victim was not copied"); - TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin", ((StorageWriteEvt const *)wr)->name); + TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin.tmp", ((StorageWriteEvt const *)wr)->name); TEST_ASSERT_EQUAL_UINT32(3U * mb, ((StorageWriteEvt const *)wr)->len); QF_gc(wr); - answer_write((uint8_t)KODE_OK, 3U * mb); + answer_write_landed(3U * mb); (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); TEST_ASSERT_TRUE(seq_installed(1U)); @@ -5420,7 +5532,7 @@ TEST_CASE("a late STAT answer is not taken for the next victim's", const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the second victim was not copied"); QF_gc(wr); - answer_write((uint8_t)KODE_OK, 3U * mb); + answer_write_landed(3U * mb); (void)qf_support_pump(AO_AppMgr, 8U); TEST_ASSERT_TRUE(klog_has("'second' kept as /sdcard/second.bin")); @@ -5613,7 +5725,7 @@ TEST_CASE("adoption over a full region keeps the victim first", TEST_ASSERT_TRUE_MESSAGE(old_installed(), "evicted before its copy landed"); TEST_ASSERT_FALSE(boot_run_request(&off, &size, &mode)); - answer_write((uint8_t)KODE_OK, old_bytes()); + answer_write_landed(old_bytes()); run_ticks(8U); /* the LIFO adopt, then the copy */ TEST_ASSERT_TRUE(klog_has("'old' kept as /sdcard/old.bin")); @@ -5656,7 +5768,7 @@ TEST_CASE("an adoption's round publishes no progress, heartbeats included", TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, spy_count(APP_FLASH_PROGRESS_SIG), "a heartbeat of the copy reached the glass"); - answer_write((uint8_t)KODE_OK, old_bytes()); + answer_write_landed(old_bytes()); run_ticks(8U); TEST_ASSERT_TRUE(klog_has("'old' kept as /sdcard/old.bin")); TEST_ASSERT_TRUE(boot_run_request(&off, &size, &mode)); @@ -5733,17 +5845,6 @@ static void three_apps(void) plant_app(dir.e[2].offset, "tail"); } -/** @brief Take AO_Storage's next path request and check what it asks. */ -static void path_req_expect(uint8_t op, const char *path, const char *what) -{ - const QEvt * const e = storage_take(STORAGE_PATH_REQ_SIG); - - TEST_ASSERT_NOT_NULL_MESSAGE(e, what); - TEST_ASSERT_EQUAL_UINT8_MESSAGE(op, ((StoragePathEvt const *)e)->op, what); - TEST_ASSERT_EQUAL_STRING_MESSAGE(path, ((StoragePathEvt const *)e)->path, what); - QF_gc(e); -} - /* The mailbox plans with the container's decoded length, the one the decoder * plans with on its first chunk. Planned with the container's own few bytes, * the walk named the first victim only, the round copied that one, and the @@ -5773,7 +5874,7 @@ TEST_CASE("a deflated container is planned for its image, so every victim is kep const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the first victim was not copied"); QF_gc(wr); - answer_write((uint8_t)KODE_OK, 3U * mb); + answer_write_landed(3U * mb); /* The second victim is looked for with the first one's copy done and * nothing evicted yet. */ @@ -5784,10 +5885,10 @@ TEST_CASE("a deflated container is planned for its image, so every victim is kep answer_stat(false, (uint8_t)KODE_OK); wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the second victim was not copied"); - TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin", ((StorageWriteEvt const *)wr)->name); + TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin.tmp", ((StorageWriteEvt const *)wr)->name); QF_gc(wr); TEST_ASSERT_TRUE_MESSAGE(seq_installed(2U), "evicted before its copy landed"); - answer_write((uint8_t)KODE_OK, 3U * mb); + answer_write_landed(3U * mb); run_ticks(8U); /* the LIFO adopt: the second pass */ TEST_ASSERT_TRUE(klog_has("evicting 'first'")); @@ -5879,7 +5980,7 @@ TEST_CASE("a copy whose write failed is unlinked, so no later round calls it kep QF_gc(wr); answer_write((uint8_t)KODE_ERROR, 0U); - path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/old.bin", + path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/old.bin.tmp", "the stub of the failed write was left on the card"); (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); @@ -5891,6 +5992,51 @@ TEST_CASE("a copy whose write failed is unlinked, so no later round calls it kep keep_end(); } +/* The replacement is a question to the card like the STAT: a card that never + * answers it does not hold the install, and the answer that comes late is + * nobody's - least of all the next victim's STAT. */ +TEST_CASE("a replacement nobody answers fails the copy, and its late answer is not taken for the next victim's STAT", + "[appmgr_ao][keep]") +{ + uint32_t const mb = 0x100000U; + uint32_t const len = image_named("new"); + + Mockkdrv_slot_Init(); + keep_fresh(); + three_apps(); + ask_flash_dst("new.bin", (uint8_t)KFW_TARGET_APP); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); + answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/first.bin", + "the first victim was not looked for"); + answer_stat(false, (uint8_t)KODE_OK); + const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); + TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the first victim was not copied"); + QF_gc(wr); + answer_write((uint8_t)KODE_OK, 3U * mb); + pair_req_expect("/sdcard/first.bin.tmp", "/sdcard/first.bin", + "the finished copy was not put on its name"); + + run_ticks(APPMGR_OPEN_DEADLINE_TICKS + 50U); /* the replacement is never answered */ + path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/second.bin", + "the round did not go on to the second victim"); + TEST_ASSERT_FALSE_MESSAGE(klog_has("'first' kept as"), + "a replacement nobody answered was called a kept copy"); + + answer_replace((uint8_t)KODE_OK); /* the first's, late: the second's STAT is out */ + TEST_ASSERT_NULL_MESSAGE(storage_wait_for(STORAGE_WRITE_REQ_SIG, 150U), + "a late answer to a replacement was taken for the second victim's STAT"); + TEST_ASSERT_FALSE_MESSAGE(klog_has("'first' kept as"), + "a late answer to a replacement was taken for a copy that landed"); + + answer_copy_of(THREE_APP_LEN); /* the round ends properly */ + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); + let_the_open_expire(); + keep_end(); +} + /* The unlink is asked ahead of the next victim's STAT and answered ahead of * it, and its answer is a path answer like the STAT's: it must not be taken * for the STAT's, or the next victim would be copied on the strength of an @@ -5916,7 +6062,7 @@ TEST_CASE("an unlink's answer settles nothing while the next victim's STAT is ou QF_gc(wr); answer_write((uint8_t)KODE_ERROR, 0U); - path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/first.bin", + path_req_expect((uint8_t)STORAGE_PATH_UNLINK, "/sdcard/first.bin.tmp", "the stub of the failed write was left on the card"); path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/second.bin", "the round did not go on to the second victim"); @@ -5997,7 +6143,7 @@ TEST_CASE("a second plan that names a new victim evicts it uncopied, with a warn const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the first victim was not copied"); QF_gc(wr); - answer_write((uint8_t)KODE_OK, 3U * mb); + answer_write_landed(3U * mb); (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); @@ -6036,10 +6182,10 @@ TEST_CASE("a slot victim without a namesake is copied, then evicted after the wr answer_stat(false, (uint8_t)KODE_OK); const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 100U); TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the slot's victim was not copied"); - TEST_ASSERT_EQUAL_STRING("/sdcard/a00.bin", ((StorageWriteEvt const *)wr)->name); + TEST_ASSERT_EQUAL_STRING("/sdcard/a00.bin.tmp", ((StorageWriteEvt const *)wr)->name); QF_gc(wr); TEST_ASSERT_TRUE_MESSAGE(seq_installed(1U), "evicted before its copy landed"); - answer_write((uint8_t)KODE_OK, 4096U); + answer_write_landed(4096U); (void)qf_support_pump(AO_AppMgr, 8U); storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); TEST_ASSERT_TRUE_MESSAGE(seq_installed(1U), "evicted before the image was written"); @@ -6119,7 +6265,7 @@ TEST_CASE("a heartbeat queued behind the last answer reaches the adoption safely TEST_ASSERT_NOT_NULL_MESSAGE(wr, "the copy never reached AO_Storage"); QF_gc(wr); - answer_write((uint8_t)KODE_OK, old_bytes()); /* the adopt goes in front */ + answer_write_landed(old_bytes()); /* the adopt goes in front */ QACTIVE_POST(AO_AppMgr, &beat, NULL); /* the heartbeat, behind it */ run_ticks(8U); diff --git a/tests/host/cases/test_storage_ao.c b/tests/host/cases/test_storage_ao.c index 3524492..ea0616d 100644 --- a/tests/host/cases/test_storage_ao.c +++ b/tests/host/cases/test_storage_ao.c @@ -733,6 +733,186 @@ TEST_CASE("mv that meets a transfer waits for it and runs when it ends", TEST_ASSERT_FALSE(fake_media_file_exists(fake_media_fs_dir(), "src.bin")); } +/* ---- A replacement: a file put on a name that may be taken ------------------ + * FatFs renames onto no name that is there. AO_Storage moves the file on the + * name to `.bak`, puts the new one on it and unlinks the backup, and + * puts the original back when a step fails: one request, one answer, and the + * file on the name is the old one or the new one at every answer. The host's + * rename overwrites, as POSIX does, so what is checked is the order of the + * steps and what stays where when one fails. */ + +/** @brief Ask for the replacement and take its answer's `rc`; the answer's op must be the request's. */ +static int replace_answer(const char *src, const char *dst) +{ + storage_pair_request(ao_spy(), (uint8_t)STORAGE_PAIR_REPLACE, src, dst); + (void)qf_support_pump(AO_Storage, 8U); + const QEvt * const e = ao_spy_take(); + TEST_ASSERT_NOT_NULL_MESSAGE(e, "the replacement went unanswered"); + TEST_ASSERT_EQUAL_INT(STORAGE_PATH_RSP_SIG, (int)e->sig); + TEST_ASSERT_EQUAL_UINT8((uint8_t)STORAGE_PAIR_REPLACE, + ((const StoragePathRspEvt *)e)->op); + int const rc = (int)((const StoragePathRspEvt *)e)->rc; + ao_spy_drain(); + return rc; +} + +TEST_CASE("a replacement of a free name is the one rename", "[storage_ao][replace]") +{ + storage_fresh(); + make_file("new.tmp", 9U); + + TEST_ASSERT_EQUAL_INT(KODE_OK, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("app.bin")); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("new.tmp")); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("app.bin.bak")); +} + +TEST_CASE("a replacement of a taken name puts the copy there and leaves no backup", + "[storage_ao][replace]") +{ + storage_fresh(); + make_file("app.bin", 5U); + make_file("new.tmp", 9U); + + TEST_ASSERT_EQUAL_INT(KODE_OK, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT_MESSAGE(9, (int)fs_size("app.bin"), "the old file is still on the name"); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("new.tmp")); + TEST_ASSERT_EQUAL_INT_MESSAGE(-1, (int)fs_size("app.bin.bak"), "the backup was left behind"); +} + +/** @brief Whether a captured log line holds @p words. */ +static bool replace_said(const char *words) +{ + bool found = false; + + for ( unsigned i = 0U; (!found) && (fake_klog_count() > i); ++i ) + { + const char * const line = fake_klog_line(i); + found = (NULL != line) && (strstr(line, words) != NULL); + } + return found; +} + +TEST_CASE("a backup already on the card is removed by a replacement, and said", + "[storage_ao][replace]") +{ + storage_fresh(); + make_file("app.bin", 5U); + make_file("app.bin.bak", 3U); + make_file("new.tmp", 9U); + + TEST_ASSERT_EQUAL_INT(KODE_OK, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("app.bin")); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("app.bin.bak")); + TEST_ASSERT_TRUE_MESSAGE(replace_said("app.bin.bak' was there and is removed"), + "a backup that was on the card went without a word"); +} + +/* What a reset between the two renames leaves: the name free, the file that + * was there as its backup and the copy under its own name. The next + * replacement of the name puts its copy there and takes the backup away. */ +TEST_CASE("a replacement after one a reset cut puts its copy on the name and removes the backup", + "[storage_ao][replace]") +{ + storage_fresh(); + make_file("app.bin.bak", 5U); + make_file("new.tmp", 9U); + + TEST_ASSERT_EQUAL_INT(KODE_OK, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("app.bin")); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("new.tmp")); + TEST_ASSERT_EQUAL_INT_MESSAGE(-1, (int)fs_size("app.bin.bak"), + "the backup of the replacement that was cut stayed an orphan"); + TEST_ASSERT_TRUE(replace_said("app.bin.bak' was there and is removed")); +} + +TEST_CASE("a replacement of a name a folder is on is refused, and the folder stays", + "[storage_ao][replace]") +{ + struct stat st; + char path[512]; + + storage_fresh(); + fake_media_put_fs_dir("app.bin"); + make_file("new.tmp", 9U); + + TEST_ASSERT_EQUAL_INT(KODE_DENIED, replace_answer("/storage/new.tmp", "/storage/app.bin")); + (void)snprintf(path, sizeof(path), "%s/app.bin", fake_media_fs_dir()); + TEST_ASSERT_EQUAL_INT(0, stat(path, &st)); + TEST_ASSERT_TRUE_MESSAGE(S_ISDIR(st.st_mode), "the folder was moved off its name"); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("new.tmp")); + TEST_ASSERT_EQUAL_INT(-1, (int)fs_size("app.bin.bak")); +} + +TEST_CASE("a replacement whose copy is missing leaves the file on the name as it was", + "[storage_ao][replace]") +{ + storage_fresh(); + make_file("app.bin", 5U); + + TEST_ASSERT_EQUAL_INT(KODE_ERROR, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT_MESSAGE(5, (int)fs_size("app.bin"), + "the file on the name was lost with the copy"); + TEST_ASSERT_EQUAL_INT_MESSAGE(-1, (int)fs_size("app.bin.bak"), + "the file was left under its backup name"); +} + +TEST_CASE("a replacement that cannot move the file aside leaves both files where they were", + "[storage_ao][replace]") +{ + storage_fresh(); + make_file("app.bin", 5U); + make_file("new.tmp", 9U); + fake_media_put_fs_dir("app.bin.bak"); /* a name no file can be moved onto */ + + TEST_ASSERT_EQUAL_INT(KODE_ERROR, replace_answer("/storage/new.tmp", "/storage/app.bin")); + TEST_ASSERT_EQUAL_INT_MESSAGE(5, (int)fs_size("app.bin"), + "the file on the name changed with its first step failing"); + TEST_ASSERT_EQUAL_INT_MESSAGE(9, (int)fs_size("new.tmp"), + "the copy was lost with the first step failing"); +} + +TEST_CASE("a replacement that meets a transfer waits for it and runs when it ends", + "[storage_ao][replace][turn]") +{ + storage_fresh(); + hold_reader(); + make_file("app.bin", 5U); + make_file("new.tmp", 9U); + + storage_pair_request(ao_spy(), (uint8_t)STORAGE_PAIR_REPLACE, + "/storage/new.tmp", "/storage/app.bin"); + (void)qf_support_pump(AO_Storage, 8U); + TEST_ASSERT_NULL(ao_spy_take()); + TEST_ASSERT_EQUAL_INT_MESSAGE(5, (int)fs_size("app.bin"), + "the replacement ran over the transfer"); + + release_reader(); + const QEvt * const e = ao_spy_take(); + TEST_ASSERT_NOT_NULL_MESSAGE(e, "the replacement never ran"); + TEST_ASSERT_EQUAL_INT(KODE_OK, (int)((const StoragePathRspEvt *)e)->rc); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("app.bin")); +} + +TEST_CASE("a replacement is refused when the transfer outlasts its wait, and both files stay", + "[storage_ao][replace][turn]") +{ + storage_fresh(); + hold_reader(); + make_file("app.bin", 5U); + make_file("new.tmp", 9U); + + storage_pair_request(ao_spy(), (uint8_t)STORAGE_PAIR_REPLACE, + "/storage/new.tmp", "/storage/app.bin"); + wait_out_turn(); + const QEvt * const e = ao_spy_take(); + TEST_ASSERT_NOT_NULL_MESSAGE(e, "the replacement went unanswered"); + TEST_ASSERT_EQUAL_UINT8((uint8_t)STORAGE_PAIR_REPLACE, ((const StoragePathRspEvt *)e)->op); + TEST_ASSERT_EQUAL_INT(KODE_DENIED, (int)((const StoragePathRspEvt *)e)->rc); + TEST_ASSERT_EQUAL_INT(5, (int)fs_size("app.bin")); + TEST_ASSERT_EQUAL_INT(9, (int)fs_size("new.tmp")); +} + TEST_CASE("a text write that meets a transfer waits for it and lands when it ends", "[storage_ao][turn]") { From 426c29034ffb2064070fbb988d0a1d8bda956710 Mon Sep 17 00:00:00 2001 From: jbenedictocenteno Date: Fri, 13 Nov 2026 14:17:47 +0100 Subject: [PATCH 2/4] fix(kaos): Take the CRC of an install's victims in pieces Since the undo table, AO_AppMgr took the CRC of every victim of an install in one step: about 70 ms per MB, with every Active Object below it held back. The keep round now takes each victim's CRC 512 KB a step, one step a pacer tick, as the recovery at start-up does, before it asks the card about that victim. The CRCs wait in a PSRAM block of the round, as the undo table does, not in the Active Object's own memory. The app an install replaces is evicted like a victim when there is no room for both versions, and its CRC goes into the same table. It is in the round for that CRC alone: no question to the card, no copy. The second pass finds the CRCs taken and writes the table to NVS before it evicts or writes anything. A victim the round did not reach, a terminal install's or one for which PSRAM ran dry, is read in one step as before. Closes #118 --- components/kaos/appmgr/src/appmgr_ao.c | 218 ++++++++++++++++++++++--- tests/host/cases/test_appmgr_ao.c | 180 ++++++++++++++++++-- 2 files changed, 360 insertions(+), 38 deletions(-) diff --git a/components/kaos/appmgr/src/appmgr_ao.c b/components/kaos/appmgr/src/appmgr_ao.c index 5fd2732..7d53274 100644 --- a/components/kaos/appmgr/src/appmgr_ao.c +++ b/components/kaos/appmgr/src/appmgr_ao.c @@ -225,7 +225,8 @@ typedef enum { KEEP_UNTRIED = 0, KEEP_SAVED, /**< Written to the card now. */ KEEP_THERE, /**< Its file was already on the card. */ - KEEP_FAILED /**< No card, no memory, no name, or the write failed. */ + KEEP_FAILED, /**< No card, no memory, no name, or the write failed. */ + KEEP_CRC_ONLY /**< The app the install replaces: no copy, its CRC alone. */ } keep_res_t; /** @brief Which way an app an install evicted went (undo_table_t.fate). */ @@ -249,6 +250,14 @@ typedef struct { uint8_t fate[KAPPS_MAX_ENTRIES]; /**< UNDO_*: what became of each; not in the blob. */ } undo_table_t; +/** @brief The CRC-32 of each victim's extent, taken by the keep round (PSRAM, like the undo table). */ +typedef struct { + uint32_t crc[KAPPS_MAX_ENTRIES]; /**< Of the extent, in the order of keep_seq[]. */ + bool ok[KAPPS_MAX_ENTRIES]; /**< crc[] holds that victim's. */ + uint32_t done; /**< Bytes of the extent in hand already folded in. */ + uint32_t run; /**< The running CRC of that extent. */ +} keep_crcs_t; + /* A record is a directory entry and a name with nothing between, and a table * has a slot for every entry of the directory: the layout in NVS is this one. */ _Static_assert(sizeof(appmgr_undo_hdr_t) == APPMGR_UNDO_HDR_LEN, @@ -399,6 +408,8 @@ typedef struct { uint8_t *keep_buf; /**< PSRAM: one victim's extent. */ uint32_t keep_len; /**< That extent's length. */ uint32_t keep_off; /**< Its region offset. */ + keep_crcs_t *keep_crcs; /**< PSRAM: each victim's CRC, when taken; NULL when none. */ + bool keep_hashing; /**< The victim in hand's CRC is being taken, in keep_crcs. */ uint32_t keep_fill; /**< Bytes of it read so far. */ char keep_name[KDRV_SLOT_PROJECT_MAX]; /**< The victim's project name. */ char keep_path[KODE_APP_FILE_MAX]; /**< /sdcard/.bin */ @@ -948,6 +959,11 @@ static void keep_clear(AppMgr * const me) khal_free(me->keep_buf); me->keep_buf = NULL; } + if ( NULL != me->keep_crcs ) + { + khal_free(me->keep_crcs); + me->keep_crcs = NULL; + } victims_clear(me); /* an install that ends owes no folder to anyone */ undo_clear(me); /* nor is anything left to give back */ me->keep_round = false; @@ -955,6 +971,7 @@ static void keep_clear(AppMgr * const me) me->keep_forced = false; me->keep_writing = false; me->keep_stat = false; + me->keep_hashing = false; me->keep_moving = false; me->keep_count = 0U; me->keep_idx = 0U; @@ -1082,7 +1099,8 @@ static void app_list_page(AppListReqEvt const * const req) * Records every victim not yet in the round. Answers true when there is at * least one and the install can restart; the caller then abandons the * transfer before anything is committed. The app the install replaces is no - * victim: its new version is the install. + * victim to copy, its new version is the install: it is in the round for its + * CRC alone (KEEP_CRC_ONLY). * * @return true to defer the install. */ @@ -1095,12 +1113,27 @@ static bool keep_defer(AppMgr * const me, const kapps_dir_t *d, { return false; /* one round per install; the console cannot pause */ } + if ( NULL == me->keep_crcs ) + { + /* A dry PSRAM costs the pieces, never the install: undo_note() reads + * a victim the round has no CRC for in one step. */ + /* cppcheck-suppress misra-c2012-11.5 ; a table from a void * allocator */ + me->keep_crcs = (keep_crcs_t *)khal_realloc_psram(NULL, sizeof(*me->keep_crcs)); + if ( NULL != me->keep_crcs ) + { + (void)memset(me->keep_crcs, 0, sizeof(*me->keep_crcs)); + } + } for ( uint32_t n = 0U; n <= plan->evict_count; ++n ) { uint8_t const idx = plan_victim(plan, n); - if ( (KAPPS_NO_VICTIM != idx) && (plan->replaced != idx) - && (keep_find(me, d->e[idx].seq) < 0) - && (KAPPS_MAX_ENTRIES > me->keep_count) ) + if ( (KAPPS_NO_VICTIM == idx) + || (keep_find(me, d->e[idx].seq) >= 0) + || (KAPPS_MAX_ENTRIES <= me->keep_count) ) + { + /* no victim here, one already in the round, or no room to note it */ + } + else if ( plan->replaced != idx ) { me->keep_seq[me->keep_count] = d->e[idx].seq; me->keep_res[me->keep_count] = (uint8_t)KEEP_UNTRIED; @@ -1108,10 +1141,54 @@ static bool keep_defer(AppMgr * const me, const kapps_dir_t *d, ++me->keep_count; ++fresh; } + else if ( NULL != me->keep_crcs ) + { + /* The app replaced is evicted like the others and its CRC goes + * into the same table: the round takes it, and nothing else. */ + me->keep_seq[me->keep_count] = d->e[idx].seq; + me->keep_res[me->keep_count] = (uint8_t)KEEP_CRC_ONLY; + ++me->keep_count; + ++fresh; + } + else + { + /* the app replaced, and no room for its CRC: undo_note() reads it */ + } } return 0U < fresh; } +/** + * @brief Say what the round about to start is for. + * + * The app the install replaces is in the round for its CRC alone and is no + * app to keep. + * + * @param me The AO, with the round's victims noted. + * @param before What the round comes before, in the log's words. + */ +static void keep_say(const AppMgr * const me, const char *before) +{ + uint32_t copies = 0U; + + for ( uint32_t i = 0U; i < me->keep_count; ++i ) + { + if ( (uint8_t)KEEP_CRC_ONLY != me->keep_res[i] ) + { + ++copies; + } + } + if ( 0U < copies ) + { + KLOG_I(TAG, "%lu app(s) to keep on the card before %s", + (unsigned long)copies, before); + } + else + { + KLOG_I(TAG, "the CRC of the app being replaced is taken before %s", before); + } +} + /** * @brief Remember an evicted app, by name, so the install's tidy step deletes * its folder. @@ -1298,16 +1375,27 @@ static void undo_note(AppMgr * const me, const kapps_dir_t *d, uint8_t idx) unnamed_label(r->project, sizeof(r->project), d->e[idx].offset); } /* The digest the next boot checks the image against: of the bytes as they - * lie now, before the install's first erase can reach them. Read in one - * step, for every victim of the plan: a few tens of ms per MB of images. */ - uint32_t crc = KHAL_CRC32_INIT; + * lie now, before the install's first erase can reach them. The keep round + * took it in pieces, the app replaced included; a victim it did not reach + * (the terminal's, which cannot pause) is read here in one step, a few tens + * of ms per MB. */ + uint32_t crc = KHAL_CRC32_INIT; + int const k = keep_find(me, d->e[idx].seq); - if ( kdrv_apps_extent_crc(d->e[idx].offset, d->e[idx].size, &crc) != KODE_OK ) + if ( (0 <= k) && (NULL != me->keep_crcs) && me->keep_crcs->ok[k] ) + { + crc = me->keep_crcs->crc[k]; + } + else if ( kdrv_apps_extent_crc(d->e[idx].offset, d->e[idx].size, &crc) != KODE_OK ) { KLOG_W(TAG, "cannot read '%s' to record it; a reset in the middle of " "the install costs it", r->project); crc = 0U; } + else + { + /* read in one step: nothing took it before */ + } r->crc = crc; me->undo->fate[me->undo_count] = (uint8_t)UNDO_STAYS; ++me->undo_count; @@ -2786,8 +2874,7 @@ static kode_status_t appmgr_place(void *ctx, uint32_t img_len, const char *name) if ( keep_defer(me, &dir, &plan) ) { me->keep_pending = true; - KLOG_I(TAG, "%lu app(s) to keep on the card before this install", - (unsigned long)me->keep_count); + keep_say(me, "this install"); return KODE_ERROR; /* the decoder stops before the sink opens */ } /* Evictions, loudly and before any flash is touched; the copy is @@ -3345,6 +3432,7 @@ void AppMgr_ctor(void) me->os_target = false; me->sink = NULL; me->chunk = NULL; + me->keep_crcs = NULL; me->keep_stale_stat = 0U; me->keep_stale_move = 0U; me->keep_stale_write = 0U; @@ -4109,9 +4197,7 @@ static QState AppMgr_idle(AppMgr * const me, QEvt const * const e) * mailbox is a partition, so there is no medium to close. */ if ( keep_defer(me, &dir, &plan) ) { - KLOG_I(TAG, "%lu app(s) to keep on the card before " - "adopting the mailbox", - (unsigned long)me->keep_count); + keep_say(me, "adopting the mailbox"); keep_pause(me); status = Q_TRAN(&AppMgr_keeping); break; @@ -4539,9 +4625,22 @@ static uint32_t appdir_size_at(const kapps_dir_t *d, uint32_t offset) return size; } +/** + * @brief Start on the CRC of the victim in hand: a piece a step, from the pacer. + * @param me The AO, with its block for the round's CRCs (keep_crcs) had. + */ +static void keep_hash_begin(AppMgr * const me) +{ + me->keep_hashing = true; + me->keep_crcs->done = 0U; + me->keep_crcs->run = KHAL_CRC32_INIT; + (void)QTimeEvt_rearm(&me->pacer, APPMGR_CHUNK_GAP_TICKS); +} + /** * @brief Start on the next untried victim. - * @return true when one was started (a STAT is out), false when none is left. + * @return true when one was started (its CRC is being taken, or a STAT is + * out), false when none is left. */ static bool keep_next(AppMgr * const me) { @@ -4556,7 +4655,17 @@ static bool keep_next(AppMgr * const me) me->keep_len = ent->size; me->keep_off = ent->offset; } - if ( (NULL == ent) || (kdrv_apps_desc(ent, &vd, NULL) != KODE_OK) ) + if ( (uint8_t)KEEP_CRC_ONLY == me->keep_res[me->keep_idx] ) + { + /* The app replaced: no name, no path, no question to the card. */ + if ( NULL != ent ) + { + keep_hash_begin(me); + return true; + } + ++me->keep_idx; + } + else if ( (NULL == ent) || (kdrv_apps_desc(ent, &vd, NULL) != KODE_OK) ) { KLOG_W(TAG, "an app with no readable name cannot be kept on the card"); keep_settle(me, KEEP_FAILED); @@ -4566,8 +4675,15 @@ static bool keep_next(AppMgr * const me) (void)snprintf(me->keep_name, sizeof(me->keep_name), "%s", vd.project); card_copy_path(me->keep_path, sizeof(me->keep_path), vd.project); - card_stat(me, me->keep_path); - me->keep_stat = true; + if ( NULL != me->keep_crcs ) + { + keep_hash_begin(me); /* its CRC first */ + } + else + { + card_stat(me, me->keep_path); /* no room for the CRCs: the note reads it */ + me->keep_stat = true; + } return true; } } @@ -4618,6 +4734,56 @@ static void keep_drop_tmp(AppMgr * const me) storage_path_request(&me->super, (uint8_t)STORAGE_PATH_UNLINK, tmp); } +/** + * @brief Fold the next piece of the victim in hand's extent into its CRC. + * + * At most APPMGR_UNDO_STEP_BYTES a step, the next one a pacer tick later, as + * the recovery at start-up takes the same CRC. After the last piece the card + * is asked about the victim, unless it is the app the install replaces, which + * ends there. An extent that cannot be read leaves the victim with no CRC: + * undo_note() says so when it notes it. + * + * @param me The AO, with keep_hashing set. + * @return true when the victim in hand is done with and the round goes on. + */ +static bool keep_hash_step(AppMgr * const me) +{ + uint32_t n = me->keep_len - me->keep_crcs->done; + + if ( APPMGR_UNDO_STEP_BYTES < n ) + { + n = APPMGR_UNDO_STEP_BYTES; + } + if ( kdrv_apps_extent_crc(me->keep_off + me->keep_crcs->done, n, + &me->keep_crcs->run) != KODE_OK ) + { + me->keep_crcs->done = me->keep_len; /* no CRC: the note reads it again */ + } + else + { + me->keep_crcs->done += n; + if ( me->keep_crcs->done >= me->keep_len ) + { + me->keep_crcs->crc[me->keep_idx] = me->keep_crcs->run; + me->keep_crcs->ok[me->keep_idx] = true; + } + } + if ( me->keep_crcs->done < me->keep_len ) + { + (void)QTimeEvt_rearm(&me->pacer, APPMGR_CHUNK_GAP_TICKS); + return false; + } + me->keep_hashing = false; + if ( (uint8_t)KEEP_CRC_ONLY == me->keep_res[me->keep_idx] ) + { + ++me->keep_idx; /* the app replaced: its CRC is all the round takes */ + return true; + } + card_stat(me, me->keep_path); + me->keep_stat = true; + return false; +} + /** @brief Every victim tried: cancel (rule 5) or restart. */ static QState keep_finish(AppMgr * const me) { @@ -4659,10 +4825,12 @@ static QState keep_on(AppMgr * const me) /** * @brief Copy each victim to the card, one at a time, then restart. * - * Per victim: STAT its path, and when its copy is not there, read its extent - * in APPMGR_KEEP_CHUNK steps, hand it to AO_Storage to write under a temporary + * Per victim: take the CRC of its extent in APPMGR_UNDO_STEP_BYTES steps, STAT + * its path, and when its copy is not there, read its extent in + * APPMGR_KEEP_CHUNK steps, hand it to AO_Storage to write under a temporary * name, and ask AO_Storage to put that file on the app's name. A failure is - * KEEP_FAILED, and the round goes on. + * KEEP_FAILED, and the round goes on. The app the install replaces is in the + * round for its CRC alone. */ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) { @@ -4836,6 +5004,14 @@ static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e) } case APP_FLASH_CHUNK_SIG: { status = Q_HANDLED(); + if ( me->keep_hashing ) + { + if ( keep_hash_step(me) ) + { + status = keep_on(me); + } + break; + } if ( me->keep_writing ) { /* The heartbeat while AO_Storage writes: the same percent diff --git a/tests/host/cases/test_appmgr_ao.c b/tests/host/cases/test_appmgr_ao.c index 91372e3..2999e6d 100644 --- a/tests/host/cases/test_appmgr_ao.c +++ b/tests/host/cases/test_appmgr_ao.c @@ -1093,6 +1093,7 @@ static void rm_rsp(const char *project, kode_status_t st); static void wipe_rsp(const char *project, kode_status_t st, uint8_t fs_files, uint8_t sd_files, uint8_t left); static void answer_stat(bool exists, uint8_t rc); +static const QEvt *storage_take_patient(enum_t sig); static void answer_copy_of(uint32_t bytes); /* Fix 2: adoption plans and evicts BEFORE the copy, and the copy is what the @@ -1155,7 +1156,7 @@ TEST_CASE("a mailbox image that fails verification does not evict an app for roo * directory entry may name nothing the round can read, in which case it * restarts at once - pump that too. */ (void)qf_support_pump(AO_AppMgr, 8U); - const QEvt *s = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *s = storage_take_patient(STORAGE_PATH_REQ_SIG); if ( NULL != s ) { QF_gc(s); @@ -2468,6 +2469,9 @@ TEST_CASE("an install's end that finds the pool dry still arrives, unnamed, " * copy runs in a fraction of a second. */ +/** @brief The bytes of a victim's extent one step folds into its CRC, as appmgr_ao.c has it. */ +#define KEEP_HASH_STEP_BYTES (512U * 1024U) + /** @brief The QP ticks between a heartbeat and the next, as appmgr_ao.c has it. */ #define APPMGR_KEEP_BEAT_TICKS (1000U / 10U) @@ -2566,14 +2570,6 @@ static const QEvt *storage_take(enum_t sig) return NULL; } -/** @brief Take an event of @p sig out of AO_Storage's queue, or fail. */ -static void storage_expect(enum_t sig, const char *what) -{ - const QEvt * const e = storage_take(sig); - - TEST_ASSERT_NOT_NULL_MESSAGE(e, what); - QF_gc(e); -} /** @brief What AO_Storage's queue held, by kind; see storage_tally(). */ typedef struct { @@ -2633,6 +2629,37 @@ static const QEvt *storage_wait_for(enum_t sig, unsigned max_ticks) return NULL; } +/** + * @brief The ticks a victim's CRC may take before AO_Storage is asked about it: + * more than the pieces of the largest extent. + */ +#define KEEP_HASH_WAIT_TICKS 200U + +/** + * @brief Take AO_Storage's next event of @p sig, ticking for it when it is not + * queued yet: a victim is looked for once its CRC is taken. + * @return The event (the caller QF_gc()s it), or NULL when none came. + */ +static const QEvt *storage_take_patient(enum_t sig) +{ + const QEvt * const e = storage_take(sig); + + if ( NULL != e ) + { + return e; + } + return storage_wait_for(sig, KEEP_HASH_WAIT_TICKS); +} + +/** @brief Take an event of @p sig out of AO_Storage's queue, ticking for it, or fail. */ +static void storage_expect(enum_t sig, const char *what) +{ + const QEvt * const e = storage_take_patient(sig); + + TEST_ASSERT_NOT_NULL_MESSAGE(e, what); + QF_gc(e); +} + /** @brief Did klog capture a line holding @p needle this case? */ static bool klog_has(const char *needle) { @@ -2696,7 +2723,7 @@ static void answer_write(uint8_t st, uint32_t len) /** @brief Take AO_Storage's next path request and check what it asks. */ static void path_req_expect(uint8_t op, const char *path, const char *what) { - const QEvt * const e = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt * const e = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(e, what); TEST_ASSERT_EQUAL_UINT8_MESSAGE(op, ((StoragePathEvt const *)e)->op, what); @@ -3161,7 +3188,7 @@ TEST_CASE("a victim missing from the card is written there before it goes", (void)begin_install((uint8_t)KDRV_MEDIUM_FS, "new"); /* the plan defers */ TEST_ASSERT_TRUE_MESSAGE(old_installed(), "nothing may be evicted yet"); - const QEvt *stat = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the victim's path was never asked about"); TEST_ASSERT_EQUAL_UINT8((uint8_t)STORAGE_PATH_STAT, ((StoragePathEvt const *)stat)->op); @@ -3199,6 +3226,37 @@ TEST_CASE("a victim missing from the card is written there before it goes", keep_end(); } +/* Since #115 the CRC of every victim goes into the table the install writes + * before it evicts. Taken in one step it kept every Active Object below + * AO_AppMgr from running for about 70 ms per MB: it is taken in pieces, one a + * tick, as the recovery at start-up takes the same CRCs. */ +TEST_CASE("a victim's CRC is taken in pieces before it is looked for on the card", + "[appmgr_ao][keep]") +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + uint32_t const pieces = (old_bytes() + KEEP_HASH_STEP_BYTES - 1U) + / KEEP_HASH_STEP_BYTES; + (void)begin_install((uint8_t)KDRV_MEDIUM_FS, "new"); /* the plan defers */ + + TEST_ASSERT_NULL_MESSAGE(storage_take(STORAGE_PATH_REQ_SIG), + "the victim was looked for with its CRC not taken yet"); + run_ticks(pieces - 2U); + TEST_ASSERT_NULL_MESSAGE(storage_take(STORAGE_PATH_REQ_SIG), + "the CRC was taken in bigger pieces than the recovery takes it in"); + const QEvt *stat = storage_wait_for(STORAGE_PATH_REQ_SIG, 4U); + TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the victim was never looked for after its CRC"); + QF_gc(stat); + TEST_ASSERT_TRUE(old_installed()); + + answer_copy_of(old_bytes()); + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); + let_the_open_expire(); + keep_end(); +} + /* A replacement the card refuses or fails leaves the file on the name where * it was: the copy is no copy, its temporary is unlinked, and an install from * elsewhere goes on without it. */ @@ -4314,6 +4372,94 @@ TEST_CASE("an install that evicts records its victims and their CRCs in NVS befo keep_end(); } +/* The terminal's install never enters the keep round, so the case above + * compares the table's CRCs with nothing the round took. An install from a + * file with the region full does: "old" fills it, 27 pieces of 512 KB, and the + * table the second pass writes must carry the CRC of its extent as it lay. */ +TEST_CASE("the table of an install from a file carries the CRCs the keep round took in pieces", + "[appmgr_ao][reset][keep]") +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + uint32_t const want = extent_crc(KAPPS_DATA_OFF, old_bytes()); + uint32_t const len = begin_install((uint8_t)KDRV_MEDIUM_FS, "new"); + + storage_expect(STORAGE_PATH_REQ_SIG, "the victim's path was never asked about"); + answer_copy_of(old_bytes()); /* already on the card */ + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again"); + TEST_ASSERT_FALSE_MESSAGE(table_in_nvs(), + "the table was written before the second pass evicted anything"); + + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); /* the plan: the table, then the eviction */ + uint8_t blob[APPMGR_UNDO_HDR_LEN + (APPMGR_UNDO_MAX * APPMGR_UNDO_REC_LEN)]; + size_t blob_len = sizeof(blob); + TEST_ASSERT_EQUAL_INT(KODE_OK, + khal_nvs_get_blob(APPMGR_UNDO_NS, APPMGR_UNDO_KEY, blob, &blob_len)); + TEST_ASSERT_EQUAL_UINT(APPMGR_UNDO_HDR_LEN + APPMGR_UNDO_REC_LEN, blob_len); + appmgr_undo_rec_t rec; + (void)memcpy(&rec, &blob[APPMGR_UNDO_HDR_LEN], sizeof(rec)); + TEST_ASSERT_EQUAL_STRING("old", rec.project); + TEST_ASSERT_EQUAL_HEX32_MESSAGE(want, rec.crc, + "the CRC the keep round took in pieces is not its extent's"); + + answer_eof(); /* ends the write: nothing armed */ + run_ticks(4U); + keep_end(); +} + +/* The app an install replaces is no victim to copy, but it is evicted like one + * when there is no room for both versions, and its CRC goes into the same + * table. Updating a big app from a file is the common case: its CRC is taken + * by the round in pieces too, with no question to the card and no copy. */ +TEST_CASE("the CRC of the app an install replaces is taken in pieces, with no copy and no question to the card", + "[appmgr_ao][reset][keep]") +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + uint32_t const want = extent_crc(KAPPS_DATA_OFF, old_bytes()); + uint32_t const pieces = (old_bytes() + KEEP_HASH_STEP_BYTES - 1U) + / KEEP_HASH_STEP_BYTES; + uint32_t const len = begin_install((uint8_t)KDRV_MEDIUM_FS, "old"); /* over itself */ + + TEST_ASSERT_FALSE_MESSAGE(klog_has("to keep on the card"), + "the app the install replaces was counted as an app to keep"); + TEST_ASSERT_TRUE(klog_has("the CRC of the app being replaced is taken before this install")); + TEST_ASSERT_FALSE_MESSAGE(table_in_nvs(), "the table was written before the CRC was taken"); + run_ticks(pieces - 2U); + storage_seen_t const seen = storage_tally(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.open, + "the CRC of the app replaced was taken in bigger pieces than the recovery takes it in"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.path, + "the card was asked about the app the install replaces"); + const QEvt *again = storage_wait_for(STORAGE_OPEN_REQ_SIG, 6U); + TEST_ASSERT_NOT_NULL_MESSAGE(again, "the install did not start again after the CRC"); + QF_gc(again); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, storage_tally().write, + "the app the install replaces was copied to the card"); + TEST_ASSERT_TRUE(old_installed()); + + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); /* the plan: the table, then the old copy goes */ + TEST_ASSERT_TRUE(klog_has("makes room for its new version")); + uint8_t blob[APPMGR_UNDO_HDR_LEN + (APPMGR_UNDO_MAX * APPMGR_UNDO_REC_LEN)]; + size_t blob_len = sizeof(blob); + TEST_ASSERT_EQUAL_INT(KODE_OK, + khal_nvs_get_blob(APPMGR_UNDO_NS, APPMGR_UNDO_KEY, blob, &blob_len)); + appmgr_undo_rec_t rec; + (void)memcpy(&rec, &blob[APPMGR_UNDO_HDR_LEN], sizeof(rec)); + TEST_ASSERT_EQUAL_STRING("old", rec.project); + TEST_ASSERT_EQUAL_HEX32_MESSAGE(want, rec.crc, + "the CRC the round took of the app replaced is not its extent's"); + + answer_eof(); /* ends the write: nothing armed */ + run_ticks(4U); + keep_end(); +} + TEST_CASE("a landed install takes its table out of NVS, a failed one after its restore", "[appmgr_ao][reset]") { @@ -5293,12 +5439,12 @@ TEST_CASE("two victims: one already kept, one copied, one bar", answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); - const QEvt *stat = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL(stat); TEST_ASSERT_EQUAL_STRING("/sdcard/first.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); answer_copy_of(3U * mb); - stat = storage_take(STORAGE_PATH_REQ_SIG); + stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the second victim was not looked for"); TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); @@ -5459,7 +5605,7 @@ TEST_CASE("a late write answer settles nothing", "[appmgr_ao][keep]") /* Never answered: past the deadline the second victim's STAT is out. */ run_ticks(KEEP_WRITE_DEADLINE_TICKS(3U * mb) + 50U); - const QEvt *stat = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the round did not go on to the second victim"); TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); @@ -5518,7 +5664,7 @@ TEST_CASE("a late STAT answer is not taken for the next victim's", /* Never answered in time: past the deadline the second STAT is out. */ run_ticks(APPMGR_OPEN_DEADLINE_TICKS + 50U); - const QEvt *stat = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the round did not go on to the second victim"); TEST_ASSERT_EQUAL_STRING("/sdcard/second.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); @@ -5711,7 +5857,7 @@ TEST_CASE("adoption over a full region keeps the victim first", QASM_DISPATCH(AO_AppMgr, &adopt, 0U); TEST_ASSERT_TRUE_MESSAGE(old_installed(), "nothing may be evicted yet"); - const QEvt *stat = storage_take(STORAGE_PATH_REQ_SIG); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the victim's path was never asked about"); TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); @@ -5934,7 +6080,7 @@ TEST_CASE("a plan that changes under an adoption is deferred from inside it", QASM_DISPATCH(AO_AppMgr, &adopt, 0U); /* room to spare: no victim */ TEST_ASSERT_FALSE(klog_has("before adopting the mailbox")); put_old(old_bytes()); /* and now "old" is in the way */ - const QEvt *stat = storage_wait_for(STORAGE_PATH_REQ_SIG, 10U); + const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL_MESSAGE(stat, "the decoder's victim was never asked about"); TEST_ASSERT_EQUAL_STRING("/sdcard/old.bin", ((StoragePathEvt const *)stat)->path); QF_gc(stat); From c61f1d88d149418a6ac89f515c051de6b802bb23 Mon Sep 17 00:00:00 2001 From: jbenedictocenteno Date: Fri, 13 Nov 2026 14:58:32 +0100 Subject: [PATCH 3/4] feat(kdrv): Check an ESP image as its bytes arrive An image that comes from a file can only be judged once it has been written, by esp_image_verify() over the region. Something that must not give up an installed app for a file that may be cut needs the verdict first. kdrv_image_check takes the file's bytes in any pieces and follows the header chain against the file's length, refusing a segment that leaves it as soon as its header is read. When the image carries a SHA-256 it folds one over the bytes before it and compares at the end. Nothing is read from flash or written. --- components/kdrv/CMakeLists.txt | 1 + .../kdrv/apps/include/kdrv_image_check.h | 63 ++++ components/kdrv/apps/src/kdrv_image_check.c | 328 +++++++++++++++++ tests/host/cases/test_kdrv_image_check.c | 340 ++++++++++++++++++ tests/host/main/CMakeLists.txt | 1 + 5 files changed, 733 insertions(+) create mode 100644 components/kdrv/apps/include/kdrv_image_check.h create mode 100644 components/kdrv/apps/src/kdrv_image_check.c create mode 100644 tests/host/cases/test_kdrv_image_check.c diff --git a/components/kdrv/CMakeLists.txt b/components/kdrv/CMakeLists.txt index 0055551..7516685 100644 --- a/components/kdrv/CMakeLists.txt +++ b/components/kdrv/CMakeLists.txt @@ -100,6 +100,7 @@ set(srcs "apps/src/kdrv_slot_crash.c" "apps/src/kdrv_apps.c" "apps/src/kdrv_kfw.c" + "apps/src/kdrv_image_check.c" "apps/src/kapp_desc.c" "apps/src/kapp_cat.c" "apps/src/kapp_data.c" diff --git a/components/kdrv/apps/include/kdrv_image_check.h b/components/kdrv/apps/include/kdrv_image_check.h new file mode 100644 index 0000000..1f7b702 --- /dev/null +++ b/components/kdrv/apps/include/kdrv_image_check.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: GPL-3.0-or-later */ +/** + * @file kdrv_image_check.h + * @brief An ESP image judged as its bytes arrive: the header chain against the file's length, then the SHA-256 it carries. + */ +#ifndef KDRV_IMAGE_CHECK_H +#define KDRV_IMAGE_CHECK_H + +#include +#include + +#include "kode_defs.h" + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * @brief Open a check for an image in a file of @p file_len bytes. + * + * One check at a time. The bytes are fed in order, from the image's first; + * nothing is read from flash and nothing is written. Whoever is about to give + * up an installed app for an image that arrives from a file feeds it through + * here first. + * + * @param file_len The file's length: no segment may declare bytes past it. + * @return KODE_OK; KODE_DENIED when a check is already open. + */ +kode_status_t kdrv_image_check_begin(uint32_t file_len); + +/** + * @brief Feed the next bytes of the file. + * + * Bytes past the image's end (the SHA-256 included) are taken and ignored. + * + * @param data The bytes. + * @param len How many. + * @return KODE_OK while the file may still be a whole image; KODE_ERROR once + * it cannot be: a bad magic or segment count, a segment that leaves + * the file, or a SHA-256 that does not match. The check is then spent + * and kdrv_image_check_end() answers KODE_ERROR; KODE_DENIED with no + * check open. + */ +kode_status_t kdrv_image_check_feed(const void *data, size_t len); + +/** + * @brief Close the check and give its verdict. + * + * @return KODE_OK when the chain closed inside the bytes fed and the SHA-256 + * it carries, if it carries one, matched; KODE_ERROR otherwise, + * including a file that ends inside its image; KODE_DENIED with no + * check open. + */ +kode_status_t kdrv_image_check_end(void); + +/** @brief Abandon the check. Always safe. */ +void kdrv_image_check_abort(void); + +#ifdef __cplusplus +} +#endif + +#endif /* KDRV_IMAGE_CHECK_H */ diff --git a/components/kdrv/apps/src/kdrv_image_check.c b/components/kdrv/apps/src/kdrv_image_check.c new file mode 100644 index 0000000..f392c23 --- /dev/null +++ b/components/kdrv/apps/src/kdrv_image_check.c @@ -0,0 +1,328 @@ +/* SPDX-License-Identifier: GPL-3.0-or-later */ +/** + * @file kdrv_image_check.c + * @brief An ESP image judged as its bytes arrive: the header chain against the file's length, then the SHA-256 it carries. + */ +#include "kdrv_image_check.h" + +#include +#include + +#include "esp_image_format.h" +#include "psa/crypto.h" + +#include "kode_log.h" + +#define TAG "kdrv.imgchk" /**< This file's log tag. */ + +#define CHK_HDR_LEN ((uint32_t)sizeof(esp_image_header_t)) /**< Bytes of the image header. */ +#define CHK_SEG_LEN ((uint32_t)sizeof(esp_image_segment_header_t)) /**< Bytes of a segment header. */ +#define CHK_PAD_MASK 15U /**< The checksum byte pads the image out to 16 bytes, less one. */ +#define CHK_HASH_LEN 32U /**< Bytes of the SHA-256 an image may carry after its checksum. */ + +/** @brief Where the check is in the image. */ +typedef enum { + CHK_IDLE = 0, /**< No check open. */ + CHK_HEADER, /**< Collecting the image header. */ + CHK_SEG_HDR, /**< Collecting a segment header. */ + CHK_SEG_DATA, /**< Inside a segment's data. */ + CHK_PAD, /**< In the padding that ends with the checksum byte. */ + CHK_HASH, /**< Collecting the SHA-256 the image carries. */ + CHK_DONE, /**< The image is whole; what follows is not its. */ + CHK_BAD /**< It cannot be a whole image. */ +} chk_state_t; + +/** @brief The check in hand: one at a time, so one set of statics. */ +typedef struct { + chk_state_t st; /**< Where it is. */ + uint32_t limit; /**< The file's length. */ + uint32_t taken; /**< Bytes of the image taken so far. */ + uint32_t left; /**< Bytes the part in hand still needs. */ + uint8_t segs; /**< Segments still to come, the one in hand not counted. */ + bool carries; /**< The image carries a SHA-256. */ + bool hashing; /**< sha is open. */ + uint8_t fill; /**< Bytes of head[] in hand. */ + uint8_t head[sizeof(esp_image_header_t)]; /**< The header being collected. */ + uint8_t want[CHK_HASH_LEN]; /**< The SHA-256 the image carries. */ + uint32_t want_fill; /**< Bytes of want[] in hand. */ + psa_hash_operation_t sha; /**< Over every byte before the hash. */ +} chk_t; + +static chk_t s_chk; /**< The check in hand. */ + +/** @brief Spend the check: it cannot be a whole image. */ +static void chk_bad(const char *why) +{ + KLOG_E(TAG, "the file is not a whole image: %s", why); + if ( s_chk.hashing ) + { + (void)psa_hash_abort(&s_chk.sha); + s_chk.hashing = false; + } + s_chk.st = CHK_BAD; +} + +/** @brief Whether @p more more bytes of the image would still lie inside the file. */ +static bool chk_room(uint32_t more) +{ + return (s_chk.taken <= s_chk.limit) && (more <= (s_chk.limit - s_chk.taken)); +} + +/** @brief The image header is in: read it and start on the first segment. */ +static void chk_header_done(void) +{ + esp_image_header_t header; + + (void)memcpy(&header, s_chk.head, sizeof(header)); + if ( (ESP_IMAGE_HEADER_MAGIC != header.magic) + || (0U == header.segment_count) + || (ESP_IMAGE_MAX_SEGMENTS < header.segment_count) ) + { + chk_bad("bad magic or segment count"); + return; + } + s_chk.carries = (0U != header.hash_appended); + if ( s_chk.carries ) + { + (void)psa_crypto_init(); + s_chk.sha = (psa_hash_operation_t)PSA_HASH_OPERATION_INIT; + if ( (psa_hash_setup(&s_chk.sha, PSA_ALG_SHA_256) != PSA_SUCCESS) + || (psa_hash_update(&s_chk.sha, s_chk.head, sizeof(header)) != PSA_SUCCESS) ) + { + chk_bad("the hash would not start"); + return; + } + s_chk.hashing = true; + } + s_chk.segs = header.segment_count; + s_chk.fill = 0U; + s_chk.left = CHK_SEG_LEN; + s_chk.st = CHK_SEG_HDR; +} + +/** @brief A segment header is in: the segment's data is next, if it fits the file. */ +static void chk_seg_hdr_done(void) +{ + esp_image_segment_header_t seg; + + (void)memcpy(&seg, s_chk.head, sizeof(seg)); + if ( s_chk.hashing + && (psa_hash_update(&s_chk.sha, s_chk.head, sizeof(seg)) != PSA_SUCCESS) ) + { + chk_bad("the hash would not take a segment header"); + return; + } + if ( !chk_room(seg.data_len) ) + { + chk_bad("a segment leaves the file"); + return; + } + --s_chk.segs; + s_chk.fill = 0U; + s_chk.left = seg.data_len; + s_chk.st = CHK_SEG_DATA; +} + +/** @brief The part in hand is whole: go on to the next. */ +static void chk_advance(void) +{ + switch ( s_chk.st ) + { + case CHK_HEADER: + { + chk_header_done(); + break; + } + case CHK_SEG_HDR: + { + chk_seg_hdr_done(); + break; + } + case CHK_SEG_DATA: + { + if ( 0U < s_chk.segs ) + { + s_chk.fill = 0U; + s_chk.left = CHK_SEG_LEN; + s_chk.st = CHK_SEG_HDR; + } + else + { + s_chk.left = ((s_chk.taken | CHK_PAD_MASK) + 1U) - s_chk.taken; + s_chk.st = CHK_PAD; + if ( !chk_room(s_chk.left) ) + { + chk_bad("the checksum leaves the file"); + } + } + break; + } + case CHK_PAD: + { + if ( s_chk.carries ) + { + s_chk.left = CHK_HASH_LEN; + s_chk.want_fill = 0U; + s_chk.st = CHK_HASH; + if ( !chk_room(s_chk.left) ) + { + chk_bad("the hash leaves the file"); + } + } + else + { + s_chk.st = CHK_DONE; + } + break; + } + case CHK_HASH: + { + uint8_t got[CHK_HASH_LEN]; + size_t got_len = 0U; + + if ( (psa_hash_finish(&s_chk.sha, got, sizeof(got), &got_len) != PSA_SUCCESS) + || (sizeof(got) != got_len) ) + { + s_chk.hashing = false; + chk_bad("the hash would not finish"); + break; + } + s_chk.hashing = false; + if ( memcmp(got, s_chk.want, sizeof(got)) != 0 ) + { + chk_bad("its SHA-256 does not match"); + } + else + { + s_chk.st = CHK_DONE; + } + break; + } + default: + { + break; + } + } +} + +/** + * @brief Take @p n bytes of the part in hand. + * @param p The bytes; @p n of them lie in the part. + * @param n How many. + */ +static void chk_take(const uint8_t *p, uint32_t n) +{ + switch ( s_chk.st ) + { + case CHK_HEADER: + case CHK_SEG_HDR: + { + (void)memcpy(&s_chk.head[s_chk.fill], p, n); + s_chk.fill = (uint8_t)(s_chk.fill + n); + break; + } + case CHK_SEG_DATA: + case CHK_PAD: + { + if ( s_chk.hashing && (psa_hash_update(&s_chk.sha, p, n) != PSA_SUCCESS) ) + { + chk_bad("the hash would not take the data"); + } + break; + } + case CHK_HASH: + { + (void)memcpy(&s_chk.want[s_chk.want_fill], p, n); + s_chk.want_fill += n; + break; + } + default: + { + break; + } + } + s_chk.taken += n; + s_chk.left -= n; +} + +kode_status_t kdrv_image_check_begin(uint32_t file_len) +{ + if ( CHK_IDLE != s_chk.st ) + { + return KODE_DENIED; + } + (void)memset(&s_chk, 0, sizeof(s_chk)); + s_chk.limit = file_len; + s_chk.left = CHK_HDR_LEN; + s_chk.st = CHK_HEADER; + return KODE_OK; +} + +kode_status_t kdrv_image_check_feed(const void *data, size_t len) +{ + /* cppcheck-suppress misra-c2012-11.5 ; the bytes of a buffer a caller lent as a void * */ + const uint8_t *p = (const uint8_t *)data; + size_t n = len; + + if ( (CHK_IDLE == s_chk.st) || (NULL == data) ) + { + return KODE_DENIED; + } + while ( (CHK_BAD != s_chk.st) && (CHK_DONE != s_chk.st) ) + { + if ( 0U == s_chk.left ) + { + chk_advance(); + } + else if ( 0U < n ) + { + uint32_t take = s_chk.left; + + if ( (size_t)take > n ) + { + take = (uint32_t)n; + } + chk_take(p, take); + p = &p[take]; + n -= take; + } + else + { + break; + } + } + if ( CHK_BAD == s_chk.st ) + { + return KODE_ERROR; + } + return KODE_OK; +} + +kode_status_t kdrv_image_check_end(void) +{ + kode_status_t rc = KODE_OK; + + if ( CHK_IDLE == s_chk.st ) + { + return KODE_DENIED; + } + if ( CHK_DONE != s_chk.st ) + { + if ( CHK_BAD != s_chk.st ) + { + chk_bad("the file ends inside its image"); + } + rc = KODE_ERROR; + } + kdrv_image_check_abort(); + return rc; +} + +void kdrv_image_check_abort(void) +{ + if ( s_chk.hashing ) + { + (void)psa_hash_abort(&s_chk.sha); + } + (void)memset(&s_chk, 0, sizeof(s_chk)); +} diff --git a/tests/host/cases/test_kdrv_image_check.c b/tests/host/cases/test_kdrv_image_check.c new file mode 100644 index 0000000..ecb9eac --- /dev/null +++ b/tests/host/cases/test_kdrv_image_check.c @@ -0,0 +1,340 @@ +/* SPDX-License-Identifier: GPL-3.0-or-later */ +/** + * @file test_kdrv_image_check.c + * @brief An image judged as its bytes arrive: the chain against the file, then the SHA-256 it carries. + */ +#include + +#include "esp_image_format.h" +#include "unity.h" + +#include "fakes.h" +#include "kdrv_image_check.h" +#include "kdrv_kfw.h" + +/** @brief The most bytes a case's image takes: header, sixteen segments, pad, hash. */ +#define IMG_MAX 512U + +/** @brief Bytes of the first segment's data. */ +#define SEG0_LEN 20U + +/** @brief Bytes of the second segment's data. */ +#define SEG1_LEN 7U + +/** + * @brief Build an image of two segments in @p img. + * + * Header, a segment of SEG0_LEN bytes, a segment of SEG1_LEN bytes, the + * checksum byte padding the image to 16 bytes, and with @p hash its SHA-256. + * + * @param img At least IMG_MAX bytes. + * @param hash Whether the image carries its hash. + * @return The image's length. + */ +static uint32_t image_make(uint8_t *img, bool hash) +{ + esp_image_header_t header; + esp_image_segment_header_t seg; + uint32_t off = 0U; + + (void)memset(img, 0xA5, IMG_MAX); + (void)memset(&header, 0, sizeof(header)); + header.magic = ESP_IMAGE_HEADER_MAGIC; + header.segment_count = 2U; + header.hash_appended = hash ? 1U : 0U; + (void)memcpy(&img[off], &header, sizeof(header)); + off += (uint32_t)sizeof(header); + (void)memset(&seg, 0, sizeof(seg)); + seg.data_len = SEG0_LEN; + (void)memcpy(&img[off], &seg, sizeof(seg)); + off += (uint32_t)sizeof(seg) + SEG0_LEN; + seg.data_len = SEG1_LEN; + (void)memcpy(&img[off], &seg, sizeof(seg)); + off += (uint32_t)sizeof(seg) + SEG1_LEN; + off = (off | 15U) + 1U; /* the checksum byte pads it out */ + if ( hash ) + { + kfw_sha256(img, off, &img[off]); + off += 32U; + } + return off; +} + +/** @brief Feed @p len bytes in pieces of @p step and say whether every piece was taken. */ +static bool feed_in(const uint8_t *img, uint32_t len, uint32_t step) +{ + for ( uint32_t at = 0U; at < len; at += step ) + { + uint32_t n = len - at; + if ( step < n ) + { + n = step; + } + if ( kdrv_image_check_feed(&img[at], n) != KODE_OK ) + { + return false; + } + } + return true; +} + +TEST_CASE("an image whose chain closes and whose hash matches passes", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, len)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +TEST_CASE("the same image fed a byte at a time passes", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, 1U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +TEST_CASE("an image with no hash passes on its chain alone", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, false); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, 5U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +TEST_CASE("a flipped byte in a segment fails on the hash", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + img[sizeof(esp_image_header_t) + sizeof(esp_image_segment_header_t) + 3U] ^= 0x01U; + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_FALSE_MESSAGE(feed_in(img, len, len), + "the hash that does not match was taken"); + TEST_ASSERT_EQUAL_INT(KODE_ERROR, kdrv_image_check_end()); +} + +TEST_CASE("a file cut inside a segment fails at the end", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, 40U, 40U)); + TEST_ASSERT_EQUAL_INT_MESSAGE(KODE_ERROR, kdrv_image_check_end(), + "a file that ends inside its image was called whole"); +} + +TEST_CASE("a file cut before its hash fails at the end", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len - 1U, len - 1U)); + TEST_ASSERT_EQUAL_INT(KODE_ERROR, kdrv_image_check_end()); +} + +TEST_CASE("a segment that leaves the file is refused as soon as its header is read", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + /* The file is shorter than the first segment says it is. */ + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len - 70U)); + TEST_ASSERT_FALSE_MESSAGE(feed_in(img, len, len), + "a segment that runs past the file's length was waited for"); + kdrv_image_check_abort(); +} + +TEST_CASE("a bad magic is refused at the first byte", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + img[0] = 0x00U; + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_FALSE(feed_in(img, len, len)); + TEST_ASSERT_EQUAL_INT(KODE_ERROR, kdrv_image_check_end()); +} + +TEST_CASE("bytes after the image are taken and ignored", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len + 16U)); + TEST_ASSERT_TRUE(feed_in(img, len + 16U, 16U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +TEST_CASE("one check at a time, and a spent one can be followed by another", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_make(img, false); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_EQUAL_INT(KODE_DENIED, kdrv_image_check_begin(len)); + kdrv_image_check_abort(); + TEST_ASSERT_EQUAL_INT(KODE_DENIED, kdrv_image_check_feed(img, 1U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, len)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +/** + * @brief Build an image of @p segs segments of @p data bytes each in @p img. + * + * @param img At least IMG_MAX bytes. + * @param segs How many segments the header says there are. + * @param data Bytes of each segment's data. + * @param hash Whether the image carries its hash. + * @return The image's length. + */ +static uint32_t image_segs(uint8_t *img, uint8_t segs, uint32_t data, bool hash) +{ + esp_image_header_t header; + esp_image_segment_header_t seg; + uint32_t off = 0U; + + (void)memset(img, 0xA5, IMG_MAX); + (void)memset(&header, 0, sizeof(header)); + header.magic = ESP_IMAGE_HEADER_MAGIC; + header.segment_count = segs; + header.hash_appended = hash ? 1U : 0U; + (void)memcpy(&img[off], &header, sizeof(header)); + off += (uint32_t)sizeof(header); + for ( uint8_t i = 0U; segs > i; ++i ) + { + (void)memset(&seg, 0, sizeof(seg)); + seg.data_len = data; + (void)memcpy(&img[off], &seg, sizeof(seg)); + off += (uint32_t)sizeof(seg) + data; + } + off = (off | 15U) + 1U; + if ( hash ) + { + kfw_sha256(img, off, &img[off]); + off += 32U; + } + return off; +} + +/* The checksum byte pads the image out to 16 bytes and is itself part of the + * padding: an image whose segments end on a boundary takes a whole block more. + * The walk of the region reads it so, and so does this. */ +TEST_CASE("an image that ends its segments on a 16-byte boundary takes a whole block of padding", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_segs(img, 1U, 16U, true); /* 24 + 8 + 16 = 48, then 16 more */ + + TEST_ASSERT_EQUAL_UINT32(48U + 16U + 32U, len); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, 7U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); + + uint32_t const bare = image_segs(img, 1U, 16U, false); /* the same, no hash: 64 bytes */ + + TEST_ASSERT_EQUAL_UINT32(64U, bare); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(bare)); + TEST_ASSERT_TRUE(feed_in(img, bare - 1U, bare - 1U)); /* the block's last byte is missing */ + TEST_ASSERT_EQUAL_INT_MESSAGE(KODE_ERROR, kdrv_image_check_end(), + "an image without its whole checksum block was called whole"); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(bare)); + TEST_ASSERT_TRUE(feed_in(img, 48U, 48U)); /* and with none of it */ + TEST_ASSERT_EQUAL_INT(KODE_ERROR, kdrv_image_check_end()); +} + +TEST_CASE("sixteen segments pass and a seventeenth is refused at the header", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_segs(img, 16U, 4U, true); + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, 13U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); + + (void)image_segs(img, 17U, 4U, false); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(IMG_MAX)); + TEST_ASSERT_FALSE_MESSAGE(feed_in(img, sizeof(esp_image_header_t), sizeof(esp_image_header_t)), + "a header of seventeen segments was taken"); + kdrv_image_check_abort(); +} + +/* What is and is not caught: an image with no hash is judged by its chain + * alone, so a flipped byte in a segment's data goes through. The checksum + * byte is not read either; the verify that follows the write is what holds + * those. */ +TEST_CASE("an image with no hash passes with a flipped byte in its data: only the chain is checked", + "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + uint32_t const len = image_segs(img, 2U, SEG0_LEN, false); + + img[sizeof(esp_image_header_t) + sizeof(esp_image_segment_header_t) + 3U] ^= 0x01U; + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(len)); + TEST_ASSERT_TRUE(feed_in(img, len, len)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} + +/* A length that would wrap the running offset is judged by the room left, not + * by a sum: the segment is refused when its header is read. */ +TEST_CASE("a segment of 0xFFFFFFFF bytes is refused at its header", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + esp_image_segment_header_t seg; + + (void)image_segs(img, 2U, SEG0_LEN, false); + (void)memset(&seg, 0, sizeof(seg)); + seg.data_len = UINT32_MAX; + (void)memcpy(&img[sizeof(esp_image_header_t)], &seg, sizeof(seg)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(UINT32_MAX)); + TEST_ASSERT_FALSE_MESSAGE( + feed_in(img, sizeof(esp_image_header_t) + sizeof(seg), 4U), + "a segment longer than any file was waited for"); + TEST_ASSERT_EQUAL_INT(KODE_ERROR, kdrv_image_check_end()); +} + +TEST_CASE("a segment of no bytes between two others passes", "[kdrv_image_check]") +{ + uint8_t img[IMG_MAX]; + esp_image_header_t header; + esp_image_segment_header_t seg; + uint32_t off = 0U; + + (void)memset(img, 0xA5, sizeof(img)); + (void)memset(&header, 0, sizeof(header)); + header.magic = ESP_IMAGE_HEADER_MAGIC; + header.segment_count = 3U; + header.hash_appended = 1U; + (void)memcpy(&img[off], &header, sizeof(header)); + off += (uint32_t)sizeof(header); + for ( uint8_t i = 0U; 3U > i; ++i ) + { + (void)memset(&seg, 0, sizeof(seg)); + if ( 1U != i ) + { + seg.data_len = SEG1_LEN; + } + (void)memcpy(&img[off], &seg, sizeof(seg)); + off += (uint32_t)sizeof(seg) + seg.data_len; + } + off = (off | 15U) + 1U; + kfw_sha256(img, off, &img[off]); + off += 32U; + + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_begin(off)); + TEST_ASSERT_TRUE(feed_in(img, off, 3U)); + TEST_ASSERT_EQUAL_INT(KODE_OK, kdrv_image_check_end()); +} diff --git a/tests/host/main/CMakeLists.txt b/tests/host/main/CMakeLists.txt index dc04ab4..6393604 100644 --- a/tests/host/main/CMakeLists.txt +++ b/tests/host/main/CMakeLists.txt @@ -183,6 +183,7 @@ set(SOURCES_UNDER_TEST "${SDK_DIR}/components/klog/src/klog.c" "${SDK_DIR}/components/kdrv/apps/src/kdrv_apps.c" "${SDK_DIR}/components/kdrv/apps/src/kdrv_kfw.c" + "${SDK_DIR}/components/kdrv/apps/src/kdrv_image_check.c" "${SDK_DIR}/components/kdrv/mirror/src/kmirror_proto.c" "${SDK_DIR}/components/kdrv/ir/src/knec.c" "${SDK_DIR}/components/kdrv/audio/src/kes_fields.c" From 30f3c65d1c3f10433865b90f7703a97991ebefee Mon Sep 17 00:00:00 2001 From: jbenedictocenteno Date: Fri, 13 Nov 2026 14:58:46 +0100 Subject: [PATCH 4/4] fix(kaos): Read an install's file through before it gives anything up An install from a file overwrote what was installed before the file was known to be whole. A file cut short of its image, of an app already installed, cost the app and ended in "did not verify". When the plan has to give something up (it evicts, or drops the old copy of the app it replaces), AO_AppMgr stops at it and reads the file through once in AppMgr_verifying: the header chain against the file's length and the SHA-256 the image carries. A file that does not hold together ends the install with nothing given up. One that does starts the install again, as the keep round does. The install's own second read, served before the read-through's open has answered, is ignored. A URL and a terminal install cannot be read twice, and the mailbox is verified in place. A .kfw container is not covered: its decoder places the image, and so evicts, at its header and compares the digest only at the end, so a cut container still costs what it gives up. An image with no appended hash is judged by its segment chain alone. Refs #103 --- components/kaos/appmgr/src/appmgr_ao.c | 338 +++++++++++++++++- tests/host/cases/test_appmgr_ao.c | 465 +++++++++++++++++++++++++ 2 files changed, 789 insertions(+), 14 deletions(-) diff --git a/components/kaos/appmgr/src/appmgr_ao.c b/components/kaos/appmgr/src/appmgr_ao.c index 7d53274..0fb421b 100644 --- a/components/kaos/appmgr/src/appmgr_ao.c +++ b/components/kaos/appmgr/src/appmgr_ao.c @@ -15,6 +15,7 @@ #include "kapp_cat.h" #include "kdrv_apps.h" #include "kdrv_kfw.h" +#include "kdrv_image_check.h" #include "kapp_data.h" #if CONFIG_KODEOS_BLE_CONSOLE #include "kdrv_ble.h" /* the console's second port: its backlog, before a launch */ @@ -71,6 +72,8 @@ * region: the same 32 KB the writes use, so a step stays short (rule 5). */ #define APPMGR_KEEP_CHUNK (32U * 1024U) #define APPMGR_KEEP_TMP_SUFFIX ".tmp" /**< What a copy's name carries while it is written. */ +#define APPMGR_BYTE_BITS 8U /**< Bits in a byte: a word read from a chunk shifts by it. */ +#define APPMGR_LOAN_BITS 8U /**< Bits of to_return: one per buffer a medium can lend. */ /* Re-publish the percent this often while the card writes a copy. AO_Power * takes the LED's orange pulse back after three seconds without a report, and * a multi-MB write has none of its own. */ @@ -370,6 +373,23 @@ typedef struct { * host's byte count is the wire's only * boundary) but no longer written. */ + /* ---- Reading a file through before an install gives anything up ------ + * An install from a file that has to give up something installed (it + * evicts, or drops the old copy of the app it replaces) stops at its plan + * and the file is read through once, by AppMgr_verifying: its chain + * against its length and the SHA-256 it carries. A file that does not hold + * together ends the install with nothing given up. A file that does starts + * the install again from the top with verify_done set, as the keep round + * does. A URL and a terminal install cannot be read twice; the mailbox is + * verified in place. A .kfw container is not read through: its decoder + * places the image, and so evicts, at its header, and compares the digest + * only at the end, so a cut container still costs what it gives up. */ + bool verify_pending; /**< appmgr_place() deferred: read the file through first. */ + bool verify_done; /**< The file was read through, or needs no reading. */ + bool verify_restart; /**< The next FLASH_REQ_SIG goes on with this install. */ + bool verify_opened; /**< The read-through's own open has answered. */ + bool verify_first; /**< The next chunk is the file's first. */ + /* ---- Keeping victims on the card before an eviction ---------------- * One round per install: the first plan that names victims abandons the * transfer, AppMgr_keeping copies them, and the install starts again @@ -560,6 +580,7 @@ static QState AppMgr_opening(AppMgr * const me, QEvt const * const e); static QState AppMgr_flashing(AppMgr * const me, QEvt const * const e); static QState AppMgr_adopting(AppMgr * const me, QEvt const * const e); static QState AppMgr_keeping(AppMgr * const me, QEvt const * const e); +static QState AppMgr_verifying(AppMgr * const me, QEvt const * const e); static QState AppMgr_busy(AppMgr * const me, QEvt const * const e); static QState AppMgr_asking(AppMgr * const me, QEvt const * const e); static QState AppMgr_finding(AppMgr * const me, QEvt const * const e); @@ -971,6 +992,9 @@ static void keep_clear(AppMgr * const me) me->keep_forced = false; me->keep_writing = false; me->keep_stat = false; + me->verify_pending = false; + me->verify_done = false; + me->verify_restart = false; me->keep_hashing = false; me->keep_moving = false; me->keep_count = 0U; @@ -1093,6 +1117,25 @@ static void app_list_page(AppListReqEvt const * const req) app_list_end(req->who, i < d->count); } +/** + * @brief Decide whether the file must be read through before this plan is acted on. + * + * Only an install that gives something up needs it, and only from a file: a + * URL and a terminal install cannot be read twice, and the mailbox is verified + * in place. + * + * @param me The AO. + * @param plan The plan the decoder's header made. + * @return true to defer the install. + */ +static bool verify_defer(const AppMgr * const me, const kapps_plan_t *plan) +{ + bool const file = ((uint8_t)APPMGR_ORIGIN_FILE == me->origin) + || ((uint8_t)APPMGR_ORIGIN_CARD == me->origin); + + return file && !me->verify_done && !me->bypass && (0U < plan->evict_count); +} + /** * @brief Decide whether this plan's victims must be copied first. * @@ -2871,6 +2914,13 @@ static kode_status_t appmgr_place(void *ctx, uint32_t img_len, const char *name) "everything", (unsigned long)img_len); return KODE_ERROR; } + if ( verify_defer(me, &plan) ) + { + me->verify_pending = true; + KLOG_I(TAG, "the file is to be read through before this install gives " + "anything up"); + return KODE_ERROR; /* the decoder stops before the sink opens */ + } if ( keep_defer(me, &dir, &plan) ) { me->keep_pending = true; @@ -2973,6 +3023,29 @@ static void medium_return(AppMgr * const me, uint8_t idx) storage_next_request(idx); } +/** + * @brief Hand back every buffer the medium lent since the last pacer tick. + * + * The pacer's gap is what lets the glass and the log run between two chunks; + * one tick returns however many buffers are waiting. + * + * @param me The AO, in a state that reads the medium. + */ +static void loans_return(AppMgr * const me) +{ + uint8_t const mask = me->to_return; + + me->pacing = false; + me->to_return = 0U; + for ( uint8_t i = 0U; APPMGR_LOAN_BITS > i; ++i ) + { + if ( (mask & (uint8_t)(1U << i)) != 0U ) + { + medium_return(me, i); + } + } +} + /** * @brief Open the transfer: the sink itself for a park, otherwise a decode session. * @@ -3280,7 +3353,7 @@ static bool consume_chunk(AppMgr * const me, const StorageDataEvt *d) kode_status_t const wst = xfer_feed(me, buf, d->len); if ( KODE_OK != wst ) { - if ( me->keep_pending ) + if ( me->keep_pending || me->verify_pending ) { return false; /* deferred, not failed: the caller moves on */ } @@ -3820,11 +3893,12 @@ static QState AppMgr_idle(AppMgr * const me, QEvt const * const e) FlashReqEvt const * const req = Q_EVT_CAST(FlashReqEvt); xfer_reset(me); - if ( !me->keep_round ) + if ( !me->keep_round && !me->verify_restart ) { keep_clear(me); me->keep_req = *req; /* what a restart would send again */ } + me->verify_restart = false; me->req_who = req->who; /* owed a verdict, settled below or in AppMgr_opening */ me->origin = (uint8_t)APPMGR_ORIGIN_FILE; if ( (uint8_t)KFW_SRC_HTTP == req->src_kind ) @@ -4425,6 +4499,27 @@ static void keep_pause(AppMgr * const me) me->keep_idx = 0U; } +static void install_again(AppMgr * const me); + +/** + * @brief Stop the install at its plan and open its file again, to read it through. + * + * Nothing was committed: the transfer is abandoned, the file closed and opened + * anew for AppMgr_verifying. + * + * @param me The AO, with verify_pending set. + * @return The state to go on in. + */ +static QState verify_start(AppMgr * const me) +{ + keep_pause(me); + me->verify_pending = false; + me->verify_opened = false; + me->verify_first = true; + storage_open_request(me->file, &me->super); + return Q_TRAN(&AppMgr_verifying); +} + /** @brief Start the same install again from the top; the round is done. */ static void keep_restart(AppMgr * const me) { @@ -4437,6 +4532,20 @@ static void keep_restart(AppMgr * const me) QACTIVE_POST_LIFO(&me->super, &adopt); return; } + install_again(me); +} + +/** + * @brief Start the same install again from the top, ahead of any request + * already queued. + * + * LIFO: a request already queued must not run between what was done for the + * install and the install it was done for. A failure to post ends the install. + * + * @param me The AO, whose keep_req is the request. + */ +static void install_again(AppMgr * const me) +{ FlashReqEvt * const r = Q_NEW_X(FlashReqEvt, 0U, FLASH_REQ_SIG); if ( NULL == r ) { @@ -4448,8 +4557,6 @@ static void keep_restart(AppMgr * const me) r->launch = me->keep_req.launch; r->who = NULL; /* the verdict was given with the first open */ (void)snprintf(r->src, sizeof(r->src), "%s", me->keep_req.src); - /* LIFO: a request already queued must not run between the copies and - * the install they were made for. */ QACTIVE_POST_LIFO(&me->super, &r->super); } @@ -6588,6 +6695,11 @@ static QState AppMgr_flashing(AppMgr * const me, QEvt const * const e) uint8_t const idx = d->idx; if ( !consume_chunk(me, d) ) { + if ( me->verify_pending ) + { + status = verify_start(me); + break; + } if ( me->keep_pending ) { keep_pause(me); @@ -6612,16 +6724,7 @@ static QState AppMgr_flashing(AppMgr * const me, QEvt const * const e) break; } case APP_FLASH_CHUNK_SIG: { - me->pacing = false; - uint8_t const mask = me->to_return; - me->to_return = 0U; - for ( uint8_t i = 0U; 8U > i; ++i ) - { - if ( (mask & (uint8_t)(1U << i)) != 0U ) - { - medium_return(me, i); - } - } + loans_return(me); status = Q_HANDLED(); break; } @@ -6656,6 +6759,213 @@ static QState AppMgr_flashing(AppMgr * const me, QEvt const * const e) return status; } +/** + * @brief Whether a file's first bytes are a `.kfw` container's. + * @param buf The first chunk. + * @param len Its length. + * @return true for the container's magic. + */ +static bool chunk_is_container(const uint8_t *buf, uint16_t len) +{ + uint32_t magic = 0U; + + if ( sizeof(magic) > len ) + { + return false; + } + for ( uint32_t i = 0U; sizeof(magic) > i; ++i ) + { + magic |= (uint32_t)buf[i] << (APPMGR_BYTE_BITS * i); /* little-endian, as the container's */ + } + return KFW_MAGIC == magic; +} + +/** + * @brief End the read-through: the file does not hold together, nothing was given up. + * @param me The AO. + * @param why The log's phrase. + * @return AppMgr_idle. + */ +static QState verify_fail(AppMgr * const me, const char *why) +{ + kdrv_image_check_abort(); + flash_fail(me, why, KFAIL_WRITE); + return Q_TRAN(&AppMgr_idle); +} + +/** + * @brief The file reads through whole, or needs no reading: go on with the install. + * @param me The AO. + * @return AppMgr_idle. + */ +static QState verify_resume(AppMgr * const me) +{ + me->verify_done = true; + me->verify_restart = true; + medium_close(me); + install_again(me); /* a failed post calls flash_fail() itself */ + return Q_TRAN(&AppMgr_idle); +} + +/** + * @brief Read the file through once, then start the install again. + * + * The chunks are the install's own, one buffer at a time, paced like + * AppMgr_flashing's; they are fed to kdrv_image_check and written nowhere. + * A .kfw container is not read through: its first bytes tell it apart and it + * is left to its decoder, which places the image (and so evicts) at its header + * and compares the digest only at the end. + */ +static QState AppMgr_verifying(AppMgr * const me, QEvt const * const e) +{ + QState status; + switch ( e->sig ) + { + case Q_ENTRY_SIG: { + QTimeEvt_armX(&me->idle, APPMGR_OPEN_TICKS, 0U); + (void)QTimeEvt_rearm(&me->beat, APPMGR_WAIT_BEAT_TICKS); + status = Q_HANDLED(); + break; + } + case Q_EXIT_SIG: { + (void)QTimeEvt_disarm(&me->idle); + (void)QTimeEvt_disarm(&me->beat); + (void)QTimeEvt_disarm(&me->pacer); + me->pacing = false; + me->to_return = 0U; + kdrv_image_check_abort(); + status = Q_HANDLED(); + break; + } + case APP_BEAT_SIG: { + /* The read makes no report of its own: say the same percent again. */ + beat_say(me); + status = Q_HANDLED(); + break; + } + case APP_CANCEL_REQ_SIG: { + status = verify_fail(me, "the person went back"); + break; + } + case APP_RECV_IDLE_SIG: { + status = verify_fail(me, "the medium stopped sending"); + break; + } + case STORAGE_OPEN_RSP_SIG: { + StorageOpenRspEvt const * const rsp = Q_EVT_CAST(StorageOpenRspEvt); + + if ( ((uint8_t)KODE_OK != rsp->status) + || (kdrv_image_check_begin(rsp->size) != KODE_OK) ) + { + status = verify_fail(me, "the file could not be read through; " + "nothing was given up"); + break; + } + me->verify_opened = true; + (void)QTimeEvt_rearm(&me->idle, APPMGR_CHUNK_TICKS); + medium_read(me); + medium_read(me); + status = Q_HANDLED(); + break; + } + case STORAGE_DATA_SIG: { + StorageDataEvt const * const d = Q_EVT_CAST(StorageDataEvt); + + if ( !me->verify_opened ) + { + /* The install's own second read, asked before it was stopped + * at its plan: AO_Storage serves it while the first reader is + * still open, ahead of the close and the open this state + * asked for, so its chunk (or its end of file) lands here + * before the open's answer. It is nobody's: AO_Storage's close + * takes every buffer back, and a NEXT would only start a read + * on the reader that is opened next. */ + KLOG_D(TAG, "a chunk of the first reader, before the file was opened again"); + status = Q_HANDLED(); + break; + } + if ( (uint8_t)KODE_OK != d->status ) + { + status = verify_fail(me, "read from storage failed"); + break; + } + if ( d->eof ) + { + if ( kdrv_image_check_end() != KODE_OK ) + { + status = verify_fail(me, "the file is cut or damaged; " + "nothing was given up"); + break; + } + KLOG_I(TAG, "'%s' reads through whole", me->file); + status = verify_resume(me); + break; + } + const uint8_t * const buf = medium_buf(me, d->idx); + if ( NULL == buf ) + { + status = verify_fail(me, "storage handed back a buffer that " + "does not exist"); + break; + } + if ( me->verify_first ) + { + me->verify_first = false; + if ( chunk_is_container(buf, d->len) ) + { + KLOG_I(TAG, "'%s' is a container: it is not read through", me->file); + kdrv_image_check_abort(); + status = verify_resume(me); + break; + } + } + if ( kdrv_image_check_feed(buf, d->len) != KODE_OK ) + { + status = verify_fail(me, "the file is cut or damaged; " + "nothing was given up"); + break; + } + (void)QTimeEvt_rearm(&me->idle, APPMGR_CHUNK_TICKS); + /* The buffer goes back after the pacer's gap, as AppMgr_flashing's + * does: the glass and the log run between two chunks. */ + me->to_return |= (uint8_t)(1U << d->idx); + if ( !me->pacing ) + { + me->pacing = true; + QTimeEvt_armX(&me->pacer, APPMGR_CHUNK_GAP_TICKS, 0U); + } + status = Q_HANDLED(); + break; + } + case APP_FLASH_CHUNK_SIG: { + loans_return(me); + status = Q_HANDLED(); + break; + } + case FLASH_REQ_SIG: + case APP_RUN_REQ_SIG: + case APP_ERASE_REQ_SIG: + case APP_LIST_REQ_SIG: + case APP_RECV_REQ_SIG: { + KLOG_W(TAG, "busy reading '%s' through; request ignored", me->file); + busy_deny(e); + status = Q_HANDLED(); + break; + } + case APP_CAT_REQ_SIG: { + KLOG_W(TAG, "busy reading '%s' through; app_cat request denied", me->file); + app_cat_deny(me); + status = Q_HANDLED(); + break; + } + default: { + status = Q_SUPER(&AppMgr_busy); + break; + } + } + return status; +} + /** One chunk per APP_FLASH_CHUNK_SIG, paced like the file path. Sibling of * AppMgr_flashing, but its source is kdrv_apps_pending_read - it never * talks to AO_Storage. */ diff --git a/tests/host/cases/test_appmgr_ao.c b/tests/host/cases/test_appmgr_ao.c index 2999e6d..d04e95a 100644 --- a/tests/host/cases/test_appmgr_ao.c +++ b/tests/host/cases/test_appmgr_ao.c @@ -1094,6 +1094,7 @@ static void wipe_rsp(const char *project, kode_status_t st, uint8_t fs_files, uint8_t sd_files, uint8_t left); static void answer_stat(bool exists, uint8_t rc); static const QEvt *storage_take_patient(enum_t sig); +static unsigned klog_count(const char *needle); static void answer_copy_of(uint32_t bytes); /* Fix 2: adoption plans and evicts BEFORE the copy, and the copy is what the @@ -2530,9 +2531,13 @@ static void storage_buffers(void) kbsp_release(KBSP_RES_FATFS, AO_Storage); } +/** @brief How many read-throughs of an install's file this case has answered; see read_through_asked(). */ +static unsigned s_rt_handled; + /** @brief The start of every keep case: a fresh AppMgr and a fresh Storage. */ static void keep_fresh(void) { + s_rt_handled = 0U; keep_put_dir_back(); recv_fresh(); storage_buffers(); @@ -2841,6 +2846,70 @@ static void answer_eof(void) * @param project The project name the image carries. * @return The image's length. */ +/** @brief What AO_AppMgr logs when an install is stopped at its plan to have its file read through. */ +#define READ_THROUGH_SAID "to be read through before this install gives anything up" + +/** + * @brief Whether an install was stopped at its plan to have its file read + * through, and the case has not answered for it yet. + * + * An install that gives something up reads its file through once, in + * AppMgr_verifying, before it goes on; the case answers that round after the + * first pass's data. + */ +static bool read_through_asked(void) +{ + unsigned const n = klog_count(READ_THROUGH_SAID); + + if ( n < s_rt_handled ) + { + s_rt_handled = 0U; /* the log started over: a new case */ + } + if ( n == s_rt_handled ) + { + return false; + } + s_rt_handled = n; + return true; +} + +/** + * @brief Answer the read-through of a file: it is opened again and read to its end. + * + * Then the install starts again from the top, and the case answers the open + * and the first bytes of that pass as it answered them of the first. + * + * @param src The file's bytes. + * @param whole How many of them the read-through is given. + * @param first How many the restarted pass is given. + * @param open_size The size the medium says the file has. + * @param medium The medium. + */ +static void read_through(const uint8_t *src, uint32_t whole, uint32_t first, + uint32_t open_size, uint8_t medium) +{ + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + answer_open(open_size, medium); + answer_data_bytes(src, whole); + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the file was read through"); + answer_open(open_size, medium); + answer_data_bytes(src, first); +} + +/** @brief As read_through() for a container: its first bytes end the read, which its decoder checks. */ +static void read_through_container(const uint8_t *src, uint32_t len, uint8_t medium) +{ + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + answer_open(len, medium); + answer_data_bytes(src, len); + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the container was seen"); + answer_open(len, medium); + answer_data_bytes(src, len); +} + static uint32_t begin_install(uint8_t medium, const char *project) { uint32_t const len = image_named(project); @@ -2849,6 +2918,10 @@ static uint32_t begin_install(uint8_t medium, const char *project) storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(len, medium); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, len, medium); + } return len; } @@ -3157,6 +3230,8 @@ TEST_CASE("a write whose medium goes quiet is abandoned, not waited on forever", ao_spy_subscribe(APP_FLASH_FAIL_SIG); (void)begin_install((uint8_t)KDRV_MEDIUM_FS, "new"); /* then silence */ + TEST_ASSERT_FALSE_MESSAGE(klog_has(READ_THROUGH_SAID), + "an install that gives nothing up read its file through first"); run_ticks(APPMGR_OPEN_DEADLINE_TICKS - 50U); TEST_ASSERT_NULL_MESSAGE(ao_spy_take(), "gave up before the deadline"); @@ -3176,6 +3251,319 @@ TEST_CASE("a write whose medium goes quiet is abandoned, not waited on forever", keep_end(); } +/* An install from a file that has to give up something installed reads the + * file through first: its chain against its length and the SHA-256 it carries. + * A file that is cut costs nothing that was installed. */ +TEST_CASE("an install whose file is cut is refused before it gives anything up", + "[appmgr_ao][verify]") +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + ao_spy_subscribe(APP_FLASH_FAIL_SIG); + uint32_t const len = image_named("new"); + + ask_flash_dst("new.bin", (uint8_t)KFW_TARGET_APP); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); /* the plan: "old" is in the way */ + TEST_ASSERT_TRUE_MESSAGE(klog_has(READ_THROUGH_SAID), + "the install did not stop at its plan to read the file through"); + TEST_ASSERT_TRUE(old_installed()); + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len - 16U); /* the file stops inside its image */ + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + + unsigned fails = 0U; + for ( const QEvt *ev = ao_spy_take(); NULL != ev; ev = ao_spy_take() ) + { + if ( (int)ev->sig == APP_FLASH_FAIL_SIG ) + { + ++fails; + } + } + QActive_unsubscribe(ao_spy(), APP_FLASH_FAIL_SIG); + ao_spy_drain(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(1U, fails, "a cut file was not refused"); + TEST_ASSERT_TRUE(klog_has("nothing was given up")); + TEST_ASSERT_TRUE_MESSAGE(old_installed(), "a cut file cost an installed app"); + storage_seen_t const seen = storage_tally(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.path, + "the card was asked about an app that was never going to be evicted"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.write, "a copy was written for a cut file"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.open, "a cut file started the install again"); + keep_end(); +} + +/* The case seen on a board: a file cut short of its image, of an app that was + * already installed and with no room for both. The old copy made room, the + * write took its time, and the last check failed with the app gone. */ +TEST_CASE("a cut file of an app already installed leaves the installed version alone", + "[appmgr_ao][verify]") +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + uint32_t const len = image_named("old"); + + ask_flash_dst("old.bin", (uint8_t)KFW_TARGET_APP); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + TEST_ASSERT_TRUE_MESSAGE(klog_has(READ_THROUGH_SAID), + "an install that drops the old copy of its app did not read its file through"); + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len - 16U); + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + + TEST_ASSERT_TRUE_MESSAGE(old_installed(), "the old copy was given up for a file that is cut"); + TEST_ASSERT_FALSE_MESSAGE(klog_has("makes room for its new version"), + "the old copy made room before the file was known to be whole"); + ao_spy_drain(); + keep_end(); +} + +/** @brief An image with its SHA-256 appended, good or one bit off; returns the file's length. */ +static uint32_t image_named_hashed(const char *project, bool good) +{ + uint32_t const len = image_named(project); + esp_image_header_t header; + + TEST_ASSERT_TRUE(sizeof(s_named_img) >= (len + 32U)); + (void)memcpy(&header, s_named_img, sizeof(header)); + header.hash_appended = 1U; + (void)memcpy(s_named_img, &header, sizeof(header)); + kfw_sha256(s_named_img, len, &s_named_img[len]); + if ( !good ) + { + s_named_img[len] ^= 0x01U; + } + return len + 32U; +} + +/** + * @brief Deliver a read of the install's first reader that AO_Storage served + * after the install was stopped at its plan. + * + * AppMgr_opening primes two reads; the plan stops the install on the first + * chunk, and the close and the open of the read-through queue behind the + * second read in AO_Storage's FIFO. That read is served while the first reader + * is open, so its chunk (or its end of file) reaches AppMgr before the open's + * answer. + */ +static void stale_data(uint8_t idx, bool eof) +{ + StorageDataEvt d; + + (void)memset(&d, 0, sizeof(d)); + d.super = (QEvt)QEVT_INITIALIZER(STORAGE_DATA_SIG); + d.idx = idx; + d.len = 16U; + d.eof = eof; + d.status = (uint8_t)KODE_OK; + if ( eof ) + { + d.len = 0U; + } + QASM_DISPATCH(AO_AppMgr, &d.super, 0U); +} + +/** + * @brief Bring an install on a full region to its read-through: stopped at its + * plan, the file asked for again. + * @return The image's length. + */ +static uint32_t dry_pass_begins(uint32_t file_len) +{ + Mockkdrv_slot_Init(); + keep_fresh(); + region_full_of_old(); + ao_spy_subscribe(APP_FLASH_FAIL_SIG); + ask_flash_dst("new.bin", (uint8_t)KFW_TARGET_APP); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); + answer_open(file_len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(file_len); + TEST_ASSERT_TRUE_MESSAGE(klog_has(READ_THROUGH_SAID), + "the install did not stop at its plan to read the file through"); + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + return file_len; +} + +/** @brief The end of a case whose read-through refused the file: one failure, nothing given up. */ +static void dry_pass_refused(const char *why) +{ + unsigned fails = 0U; + + for ( const QEvt *e = ao_spy_take(); NULL != e; e = ao_spy_take() ) + { + if ( (int)e->sig == APP_FLASH_FAIL_SIG ) + { + ++fails; + } + } + QActive_unsubscribe(ao_spy(), APP_FLASH_FAIL_SIG); + ao_spy_drain(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(1U, fails, "the read-through's end was not one failure"); + TEST_ASSERT_TRUE(klog_has(why)); + TEST_ASSERT_TRUE_MESSAGE(old_installed(), "an installed app was given up for a file refused"); + storage_seen_t const seen = storage_tally(); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.path, "the card was asked about an app that was never to be evicted"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.write, "a copy was written for a file refused"); + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, seen.open, "a file refused started the install again"); + keep_end(); +} + +/* On the board the install's own second read is served ahead of the close and + * the open of the read-through, so its chunk and its end of file land in + * AppMgr_verifying before the open's answer. They are nobody's. */ +TEST_CASE("reads of the first reader that land before the read-through's open are ignored", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + stale_data(1U, false); /* the second primed read */ + stale_data(1U, true); /* and the end of a file of one chunk */ + TEST_ASSERT_FALSE_MESSAGE(klog_has("nothing was given up"), + "a read of the first reader ended the read-through"); + + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + TEST_ASSERT_TRUE(klog_has("reads through whole")); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the read-through"); + TEST_ASSERT_TRUE(old_installed()); + + QActive_unsubscribe(ao_spy(), APP_FLASH_FAIL_SIG); + ao_spy_drain(); + let_the_open_expire(); + keep_end(); +} + +/* The same the other way round: when the read-through ends, the reads it had + * primed are served ahead of the close and the open of the pass that follows, + * so a chunk or an end of file of the read-through's reader lands in + * AppMgr_opening before the open's answer. They are nobody's there either. */ +TEST_CASE("reads of the read-through's reader that land before the next pass's open are ignored", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + TEST_ASSERT_TRUE(klog_has("reads through whole")); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the read-through"); + + stale_data(1U, false); /* the read-through's second read */ + stale_data(1U, true); /* and its second end of file */ + TEST_ASSERT_EQUAL_UINT_MESSAGE(0U, spy_count(APP_FLASH_FAIL_SIG), + "a read of the read-through's reader ended the install"); + + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); /* the pass goes on to its plan: the round */ + storage_expect(STORAGE_PATH_REQ_SIG, "the install did not go on to its victim after the stale reads"); + TEST_ASSERT_TRUE(old_installed()); + + QActive_unsubscribe(ao_spy(), APP_FLASH_FAIL_SIG); + ao_spy_drain(); + answer_copy_of(old_bytes()); + (void)qf_support_pump(AO_AppMgr, 8U); + let_the_open_expire(); + keep_end(); +} + +TEST_CASE("the person going back during the read-through ends it with nothing given up", + "[appmgr_ao][verify]") +{ + static const QEvt cancel = QEVT_INITIALIZER(APP_CANCEL_REQ_SIG); + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + QASM_DISPATCH(AO_AppMgr, &cancel, 0U); + dry_pass_refused("the person went back"); +} + +TEST_CASE("a read error during the read-through ends it with nothing given up", + "[appmgr_ao][verify]") +{ + StorageDataEvt d; + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + (void)memset(&d, 0, sizeof(d)); + d.super = (QEvt)QEVT_INITIALIZER(STORAGE_DATA_SIG); + d.status = (uint8_t)KODE_ERROR; + QASM_DISPATCH(AO_AppMgr, &d.super, 0U); + dry_pass_refused("read from storage failed"); +} + +TEST_CASE("a file that is never opened again ends the read-through at its deadline", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + run_ticks(APPMGR_OPEN_DEADLINE_TICKS + 50U); + dry_pass_refused("the medium stopped sending"); +} + +TEST_CASE("a file that stops arriving during the read-through ends it at its deadline", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named("new"); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len / 2U); /* and then nothing */ + run_ticks(APPMGR_OPEN_DEADLINE_TICKS + 50U); + dry_pass_refused("the medium stopped sending"); +} + +/* The hash the image carries is judged through AO_AppMgr: a bit off in it + * ends the read-through at the chunk that completes it, and the right one + * lets the install go on. */ +TEST_CASE("an image whose SHA-256 does not match is refused with nothing given up", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named_hashed("new", false); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + dry_pass_refused("nothing was given up"); +} + +TEST_CASE("an image whose SHA-256 matches is read through and the install goes on", + "[appmgr_ao][verify]") +{ + uint32_t const len = image_named_hashed("new", true); + + (void)dry_pass_begins(len); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data(len); + answer_eof(); + (void)qf_support_pump(AO_AppMgr, 8U); + TEST_ASSERT_TRUE(klog_has("reads through whole")); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the read-through"); + TEST_ASSERT_TRUE(old_installed()); + + QActive_unsubscribe(ao_spy(), APP_FLASH_FAIL_SIG); + ao_spy_drain(); + let_the_open_expire(); + keep_end(); +} + /* The whole round on the happy path: the plan wants "old" gone, it is not on * the card, so it is read out of the region and written there, and only then * does the install start again and evict it. */ @@ -3846,6 +4234,11 @@ TEST_CASE("an install from a medium that fails part-way gives back the apps it d storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(UNDO_IMG_LEN, (uint8_t)KDRV_MEDIUM_FS); answer_data_bytes(s_undo_img, sizeof(s_named_img)); + if ( read_through_asked() ) + { + read_through(s_undo_img, UNDO_IMG_LEN, sizeof(s_named_img), UNDO_IMG_LEN, + (uint8_t)KDRV_MEDIUM_FS); + } for ( unsigned victim = 0U; 3U > victim; ++victim ) { storage_expect(STORAGE_PATH_REQ_SIG, "a victim's path was never asked about"); @@ -5285,6 +5678,10 @@ TEST_CASE("a restarted install that fails clears the round", "[appmgr_ao][keep]" storage_expect(STORAGE_OPEN_REQ_SIG, "the next install never asked for its file"); answer_open(len, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, len, (uint8_t)KDRV_MEDIUM_FS); + } TEST_ASSERT_TRUE_MESSAGE(old_installed(), "the next install evicted at once: the last one's round was still held"); storage_expect(STORAGE_PATH_REQ_SIG, @@ -5438,6 +5835,10 @@ TEST_CASE("two victims: one already kept, one copied, one bar", storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } const QEvt *stat = storage_take_patient(STORAGE_PATH_REQ_SIG); TEST_ASSERT_NOT_NULL(stat); @@ -5597,6 +5998,10 @@ TEST_CASE("a late write answer settles nothing", "[appmgr_ao][keep]") storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } storage_expect(STORAGE_PATH_REQ_SIG, "the first victim was not looked for"); answer_stat(false, (uint8_t)KODE_OK); const QEvt *wr = storage_wait_for(STORAGE_WRITE_REQ_SIG, 1000U); @@ -5660,6 +6065,10 @@ TEST_CASE("a late STAT answer is not taken for the next victim's", storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } storage_expect(STORAGE_PATH_REQ_SIG, "the first victim was not looked for"); /* Never answered in time: past the deadline the second STAT is out. */ @@ -6154,6 +6563,10 @@ TEST_CASE("a replacement nobody answers fails the copy, and its late answer is n storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/first.bin", "the first victim was not looked for"); answer_stat(false, (uint8_t)KODE_OK); @@ -6200,6 +6613,14 @@ TEST_CASE("an unlink's answer settles nothing while the next victim's STAT is ou storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(5U * mb, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, 5U * mb, (uint8_t)KDRV_MEDIUM_FS); + } path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/first.bin", "the first victim was not looked for"); answer_stat(false, (uint8_t)KODE_OK); @@ -6283,6 +6704,10 @@ TEST_CASE("a second plan that names a new victim evicts it uncopied, with a warn storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(len, (uint8_t)KDRV_MEDIUM_FS); /* small: "first" is enough */ answer_data(len); + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, len, (uint8_t)KDRV_MEDIUM_FS); + } path_req_expect((uint8_t)STORAGE_PATH_STAT, "/sdcard/first.bin", "the first victim was not looked for"); answer_stat(false, (uint8_t)KODE_OK); @@ -7067,6 +7492,10 @@ TEST_CASE("the late answer to a question's lost STAT is settled where it lands, answer_copy_of(old_bytes()); answer_open(len, (uint8_t)KDRV_MEDIUM_FS); answer_data(len); /* "old" must go */ + if ( read_through_asked() ) + { + read_through(s_named_img, len, len, len, (uint8_t)KDRV_MEDIUM_FS); + } /* Keeping asks the card about "old" and its own answer is what comes * next: it counts. Eaten as the late one, the round would sit out its @@ -8402,6 +8831,8 @@ TEST_CASE("an evicted app kept on the card keeps its folder", storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); answer_open(len, (uint8_t)KDRV_MEDIUM_FS); answer_data_bytes(s_cont, len); + TEST_ASSERT_TRUE_MESSAGE(read_through_asked(), "the install was not stopped at its plan"); + read_through_container(s_cont, len, (uint8_t)KDRV_MEDIUM_FS); storage_expect(STORAGE_PATH_REQ_SIG, "the victim's path was never asked about"); answer_copy_of(old_bytes()); /* already on the card */ (void)qf_support_pump(AO_AppMgr, 8U); @@ -8422,6 +8853,40 @@ TEST_CASE("an evicted app kept on the card keeps its folder", tidy_end_case(); } +/* A container is not read through, and the second read primed for it is + * served ahead of the close and the open of the pass that follows: its chunk + * lands in AppMgr_opening before the open's answer, and is nobody's. */ +TEST_CASE("a container's second read that lands before the next pass's open is ignored", + "[appmgr_ao][verify]") +{ + tidy_begin_case(); + region_full_of_old(); + uint32_t const len = container_named("fresh", true, false); + + ask_flash_dst("new.bin", (uint8_t)KFW_TARGET_APP); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install never asked for its file"); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data_bytes(s_cont, len); + TEST_ASSERT_TRUE_MESSAGE(read_through_asked(), "the install was not stopped at its plan"); + storage_expect(STORAGE_OPEN_REQ_SIG, "the file was not opened again to be read through"); + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data_bytes(s_cont, len); + TEST_ASSERT_TRUE(klog_has("is a container: it is not read through")); + (void)qf_support_pump(AO_AppMgr, 8U); + storage_expect(STORAGE_OPEN_REQ_SIG, "the install did not start again after the container was seen"); + + stale_data(1U, false); /* the second read primed for the container */ + answer_open(len, (uint8_t)KDRV_MEDIUM_FS); + answer_data_bytes(s_cont, len); + storage_expect(STORAGE_PATH_REQ_SIG, "the install did not go on to its victim after the stale read"); + TEST_ASSERT_TRUE(old_installed()); + + answer_copy_of(old_bytes()); + (void)qf_support_pump(AO_AppMgr, 8U); + let_the_open_expire(); + tidy_end_case(); +} + TEST_CASE("an evicted app with the installed app's name keeps the new folder", "[appmgr_ao][tidy]") {