From 81b88922f7af41b519e7f250beddf16d3032cf43 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Thu, 16 Jul 2026 13:21:47 +0530 Subject: [PATCH 1/5] system/nxpkg: network sync, install hardening, and CLI completion. Fill in most of what the initial nxpkg slice deferred: network sync and artifact acquisition over plain HTTP (verified via SHA-256), an install rewrite that supports network sources and reclaims state left by an interrupted previous install, and wiring update/remove/rollback/ available into the CLI. Harden the package path against untrusted input along the way: bounded memory-safety helpers and explicit size limits throughout, storage read/write hardened against partially-written files, path-traversal rejection in manifest name/version before they reach the filesystem, and a fix for a lost-update race on the shared installed-packages database (two concurrent installs of different packages could otherwise silently clobber each other's recorded state). Add an optional manifest icon field for the nxstore GUI frontend to consume, raise PKG_INDEX_MAX now that the in-memory index is heap-allocated, and document the repository layout and local server setup in system/nxpkg/README.txt. Also fixes a real build break: pkg_runtime_compat() unconditionally referenced CONFIG_ARCH_BOARD, a Kconfig string symbol with no default clause under ARCH_BOARD_CUSTOM, so it is left entirely undefined rather than defined-but-empty on a custom board - falls back to CONFIG_ARCH_BOARD_CUSTOM_NAME instead. Routes pkg_error()/pkg_info() through syslog rather than stdio, since neither is visible to a supervisor with no attached console. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 --- system/nxpkg/CMakeLists.txt | 1 + system/nxpkg/Kconfig | 9 + system/nxpkg/Makefile | 1 + system/nxpkg/README.txt | 175 +++++++++ system/nxpkg/pkg.h | 154 +++++++- system/nxpkg/pkg_compat.c | 9 + system/nxpkg/pkg_install.c | 726 ++++++++++++++++++++++++++++++++---- system/nxpkg/pkg_log.c | 24 +- system/nxpkg/pkg_main.c | 107 ++++-- system/nxpkg/pkg_manifest.c | 71 ++++ system/nxpkg/pkg_metadata.c | 237 ++++++++++-- system/nxpkg/pkg_repo.c | 612 ++++++++++++++++++++++++++++++ system/nxpkg/pkg_store.c | 316 ++++++++++++++-- 13 files changed, 2259 insertions(+), 183 deletions(-) create mode 100644 system/nxpkg/README.txt create mode 100644 system/nxpkg/pkg_repo.c diff --git a/system/nxpkg/CMakeLists.txt b/system/nxpkg/CMakeLists.txt index 8ac3e358532..66a57e1ef3a 100644 --- a/system/nxpkg/CMakeLists.txt +++ b/system/nxpkg/CMakeLists.txt @@ -38,6 +38,7 @@ if(CONFIG_SYSTEM_NXPKG) pkg_log.c pkg_manifest.c pkg_metadata.c + pkg_repo.c pkg_store.c pkg_txn.c) endif() diff --git a/system/nxpkg/Kconfig b/system/nxpkg/Kconfig index afc7fa32670..196e7bdfd67 100644 --- a/system/nxpkg/Kconfig +++ b/system/nxpkg/Kconfig @@ -29,4 +29,13 @@ config SYSTEM_NXPKG_STACKSIZE int "'nxpkg' stack size" default 16384 +config SYSTEM_NXPKG_ROOT + string "'nxpkg' storage root" + default "/tmp/nxpkg" + ---help--- + Base directory used by nxpkg for its local index, installed + metadata, temporary downloads, and package payload storage. + Boards with external storage can point this to a persistent + location such as /mnt/sdcard/nxpkg. + endif diff --git a/system/nxpkg/Makefile b/system/nxpkg/Makefile index 757f9fc896e..da3e935cf63 100644 --- a/system/nxpkg/Makefile +++ b/system/nxpkg/Makefile @@ -29,6 +29,7 @@ MODULE = $(CONFIG_SYSTEM_NXPKG) CSRCS = pkg_compat.c pkg_hash.c pkg_install.c pkg_log.c pkg_manifest.c CSRCS += pkg_metadata.c pkg_store.c pkg_txn.c +CSRCS += pkg_repo.c MAINSRC = pkg_main.c include $(APPDIR)/Application.mk diff --git a/system/nxpkg/README.txt b/system/nxpkg/README.txt new file mode 100644 index 00000000000..c3a48ee80d9 --- /dev/null +++ b/system/nxpkg/README.txt @@ -0,0 +1,175 @@ +nxpkg - a package manager for NuttX application images +======================================================== + + nxpkg installs, updates, uninstalls, and rolls back standalone loadable + ELF applications (MODULE=m in an app's Makefile - see games/NXDoom or + examples/calculator for real ports) from a package repository served + over HTTP, onto local storage (an SD card on this board's config). + system/nxstore is an LVGL touchscreen front-end for nxpkg; this + document covers the repository/server side, which nxstore and the + nxpkg CLI both consume identically. + + +Repository layout +================== + + A repository is nothing more than a directory of static files: + + / + index.json - the package catalog + artifacts////// + icons////.bin - optional, see below + + index.json is a single JSON object with one "packages" array. Each + entry is one installable version of one package: + + { + "packages": [ + { + "name": "calc", + "version": "6", + "arch": "xtensa", + "compat": "esp32s3-touch-lcd-7", + "artifact": "artifacts/xtensa/esp32s3/esp32s3-touch-lcd-7/calc/6/calc", + "sha256": "<64 hex chars>", + "type": "elf", + "description": "Simple 4-function calculator", + "category": "Utilities", + "icon": "icons/xtensa/esp32s3/esp32s3-touch-lcd-7/calc.bin" + } + ] + } + + "artifact" and "icon" (both optional except "artifact") are resolved + relative to the repository's own base URL (the URL index.json itself + was fetched from, with the filename stripped) unless they're already + a full http(s):// URL - see pkg_resolve_artifact_source()/ + pkg_resolve_icon_source() in pkg_repo.c. That means the whole + directory tree above can be moved, mirrored, or served from a + completely different host without editing a single path in + index.json, as long as the *relative* structure between index.json + and artifacts/icons/ stays the same. + + "arch"/"compat" must match this board's CONFIG_ARCH ("xtensa") and + CONFIG_ARCH_BOARD ("esp32s3-touch-lcd-7") exactly - see + pkg_compat_check() in pkg_compat.c - so one repository can host + packages for several different boards side by side; each board only + ever installs the entries that match its own identity. + + +What actually serves this - any static file host works +======================================================== + + pkg_repo_fetch_url() (pkg_repo.c) does a plain HTTP GET with no auth, + no custom headers, and no server-side logic required - it just needs + a 2xx response. This means literally any static file server works: + nginx or Apache with a DocumentRoot pointed at the repo directory, a + one-line Python http.server, GitHub Pages, S3/R2/Cloudflare Pages, or + a NAS's built-in web server. There is nothing nxpkg-specific to + install on the server side. + + For local development/testing (what this project actually used while + building and testing every package in this series on real hardware): + + cd /path/to/repo-root + python3 -m http.server 8000 + + That's the entire "server setup." Confirm it's reachable from your + own machine first: + + curl -sI http://:8000/index.json + + sha256 verification (pkg_hash_file_sha256(), checked against the + manifest's "sha256" field before an artifact is ever activated) + already protects payload integrity in transit even over plain HTTP. + Only confidentiality/availability would need HTTPS on top of this if + that matters for a given deployment - nothing in the protocol assumes + HTTP specifically, an https:// URL works exactly the same way. + + +Populating a repository: tools/export_pkg_repo.py +================================================== + + Building the JSON by hand is error-prone (sha256 digests, exact + relative paths); tools/export_pkg_repo.py does it for you from a + built binary: + + python3 tools/export_pkg_repo.py \ + --arch xtensa --chip esp32s3 --compat esp32s3-touch-lcd-7 \ + --package "calc:6:elf:/path/to/apps/bin/calc:Simple 4-function calculator:Utilities" \ + /path/to/repo-root + + --package can be repeated to export several packages/versions in one + call. The colon-separated spec is: + + :::[::[:]] + + is the built artifact on your machine (e.g. apps/bin/calc + after a normal `make`). is optional: any image Pillow can open + (PNG, JPEG, ...) - the tool resizes it to a fixed 48x48 and encodes it + into the small raw RGB565 format system/nxstore's icon rendering + expects (see nxstore_load_icon() in nxstore_main.c for the exact + on-wire layout). Re-running the script is idempotent: it loads + whatever index.json already exists in the target directory, merges in + the packages you just passed (matched on name+version+arch+compat+ + type), and rewrites the file - existing unrelated entries are left + alone. + + This board's FAT-formatted SD card only supports short (8.3) file/ + directory names with no long-name extension - any path component + (package name, artifact leaf filename) over 8 characters before its + extension fails to install with errno 22, not a clear error message. + Keep names short (this is why this series' packages are named "calc", + "brick", "cgol" rather than "calculator", "brickmatch", "conway") - + this is a board/filesystem constraint, not an export_pkg_repo.py or + nxpkg one, and would not apply to a board with a different storage + filesystem. + + +Pointing the board at your repository +====================================== + + Two ways to get an index synced onto the board's local storage, + useful in different situations: + + 1. One-shot from NSH, useful for iterating on a package during + development (this is what building and testing every package in + this series actually looked like): + + nxpkg sync http://:8000/index.json + nxpkg install + nxpkg update # re-check for/install a newer version + nxpkg list # show what's installed, current vs previous + nxpkg rollback # swap back to the previous version + nxpkg remove + + 2. Automatically at boot, via system/nxstore: the repo URL nxstore + syncs from at startup is currently a single hardcoded string in + board_nxstore_autostart() (boards/xtensa/esp32s3/ + esp32s3-touch-lcd-7/src/esp32s3_bringup.c) - change that one line + to point at a different host, or a public one, and every boot + re-syncs the catalog automatically (falling back to whatever was + last synced to local storage if the network/server is unreachable + at boot - nxstore never blocks the app list on a failed sync). + + Development-network note: if your development machine's IP address + changes (switching Wi-Fi networks, a new DHCP lease, a hotspot), + re-run `nxpkg sync` with the new address - the board and the machine + serving the repository just need to be able to reach each other over + IP; nothing else about the setup changes. + + +Concurrency note +================= + + nxpkg protects a single package's own install/update/rollback against + running twice at once (a per-package lock file), and separately + protects the shared installed-packages database against being + corrupted by two *different* packages' installs racing each other + (see pkg_install_acquire_installed_lock() in pkg_install.c) - so it is + safe to have, for example, system/nxstore's own background install + worker and a directly-invoked `nxpkg install` running at the same + time for different packages. Two operations on the *same* package + name at the same time will have the second one fail with -EBUSY + (surfaced as "package has an install/update in progress") rather than + either one silently clobbering the other's work. diff --git a/system/nxpkg/pkg.h b/system/nxpkg/pkg.h index 39a4bf0c21d..ff515962ab9 100644 --- a/system/nxpkg/pkg.h +++ b/system/nxpkg/pkg.h @@ -27,30 +27,150 @@ * Included Files ****************************************************************************/ +#include + #include #include #include #include +#include + +#include /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ -#define PKG_REPO_DIR "/etc/nxpkg" -#define PKG_REPO_INDEX "/etc/nxpkg/index.json" -#define PKG_REPO_INSTALLED "/var/lib/nxpkg/installed.json" -#define PKG_STORE_DIR "/var/lib/nxpkg/pkgs" -#define PKG_TMP_DIR "/var/cache/nxpkg" -#define PKG_TMP_PKG_DIR "/var/cache/nxpkg/pkg" +#define PKG_ROOT_DIR CONFIG_SYSTEM_NXPKG_ROOT +#define PKG_REPO_DIR PKG_ROOT_DIR +#define PKG_REPO_INDEX PKG_ROOT_DIR "/index.jsn" +#define PKG_REPO_SOURCE PKG_ROOT_DIR "/repo.url" +#define PKG_REPO_INSTALLED PKG_ROOT_DIR "/instpkg.jsn" +#define PKG_STORE_DIR PKG_ROOT_DIR "/pkgs" +#define PKG_TMP_DIR PKG_ROOT_DIR "/tmp" +#define PKG_TMP_PKG_DIR PKG_ROOT_DIR "/tmp/pkg" #define PKG_NAME_MAX 63 #define PKG_VERSION_MAX 31 #define PKG_ARCH_MAX 31 #define PKG_COMPAT_MAX 63 +#define PKG_DESCRIPTION_MAX 127 +#define PKG_CATEGORY_MAX 31 #define PKG_HASH_HEX_LEN 64 -#define PKG_INDEX_MAX 32 +/* Each manifest slot is ~1.7KB (dominated by PKG_LAUNCH_ARGS_MAX slots). + * This used to be 6 to fit inside a static internal-DRAM array, but the + * catalog outgrew that once it held every real GSoC package (calc, the + * four games, NXDoom, and the original examples) - system/nxstore/ + * nxstore_main.c now heap-allocates its struct pkg_index_s g_index via + * pkg_zalloc(), which draws from the PSRAM-backed user heap on this board + * (CONFIG_ESP32S3_SPIRAM_USER_HEAP) instead of internal DRAM, so this can + * grow without the same pressure. Still not unbounded: keep it sized for + * "a real catalog", not arbitrary attacker-controlled growth. + */ + +#define PKG_INDEX_MAX 16 #define PKG_INSTALLED_MAX 16 #define PKG_INSTALLED_VERSIONS_MAX 8 +#define PKG_LAUNCH_ARGS_MAX 8 +#define PKG_LAUNCH_ARG_MAX 127 + +/* Caps against a malicious/compromised HTTP server: without these, an + * oversized response can exhaust SD-card space (downloads) or force an + * unbounded single heap allocation sized directly off attacker-controlled + * content (pkg_store_read_text). Text/metadata files (index.jsn, + * instpkg.jsn) are always small; artifact downloads cover the largest + * real payloads seen in practice (a multi-MB WAD, a ~1MB game ELF) with + * generous headroom. + */ + +#define PKG_TEXT_MAX_SIZE (256 * 1024) +#define PKG_DOWNLOAD_MAX_SIZE (32 * 1024 * 1024) + +/* nxpkg is a one-shot CLI, not a daemon, so a lock file older than this + * cannot belong to a still-running install under normal use (even a full + * multi-MB artifact over a slow link finishes well within this window) - + * it can only be left over from a process that was killed or a device + * that lost power mid-install. Reclaiming it is what makes install/ + * update/rollback usable again after the crash/power-loss scenarios this + * target is prone to, instead of failing with EBUSY forever. + */ + +#define PKG_LOCK_STALE_SECONDS (600) + +static inline void *pkg_malloc(size_t size) +{ + void *ptr = malloc(size); + + if (ptr == NULL) + { + ptr = kmm_malloc(size); + } + + return ptr; +} + +static inline void *pkg_zalloc(size_t size) +{ + void *ptr = calloc(1, size); + + if (ptr == NULL) + { + ptr = kmm_zalloc(size); + } + + return ptr; +} + +static inline void *pkg_realloc(void *ptr, size_t size) +{ + if (ptr == NULL) + { + return pkg_malloc(size); + } + + if (size == 0) + { + if (kmm_heapmember(ptr)) + { + kmm_free(ptr); + } + else + { + free(ptr); + } + + return NULL; + } + + if (kmm_heapmember(ptr)) + { + return kmm_realloc(ptr, size); + } + + return realloc(ptr, size); +} + +static inline void pkg_free(void *ptr) +{ + if (ptr == NULL) + { + return; + } + + if (kmm_heapmember(ptr)) + { + kmm_free(ptr); + } + else + { + free(ptr); + } +} + +static inline FAR char *pkg_path_alloc(void) +{ + return pkg_malloc(PATH_MAX); +} /**************************************************************************** * Public Types @@ -83,7 +203,12 @@ struct pkg_manifest_s char compat[PKG_COMPAT_MAX + 1]; char artifact[PATH_MAX]; char sha256[PKG_HASH_HEX_LEN + 1]; + char launch_args[PKG_LAUNCH_ARGS_MAX][PKG_LAUNCH_ARG_MAX + 1]; + char description[PKG_DESCRIPTION_MAX + 1]; + char category[PKG_CATEGORY_MAX + 1]; + char icon[PATH_MAX]; enum pkg_payload_type_e type; + size_t launch_argc; }; struct pkg_index_s @@ -124,6 +249,7 @@ int pkg_store_ensure_package_root(FAR const char *name); int pkg_store_ensure_version_dir(FAR const char *name, FAR const char *version); int pkg_store_format_index_path(FAR char *buffer, size_t size); +int pkg_store_format_repo_source_path(FAR char *buffer, size_t size); int pkg_store_format_installed_path(FAR char *buffer, size_t size); int pkg_store_format_package_root(FAR char *buffer, size_t size, FAR const char *name); @@ -152,6 +278,8 @@ int pkg_store_read_text(FAR const char *path, FAR char **buffer); int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text); int pkg_store_copy_file(FAR const char *src, FAR const char *dest); int pkg_store_remove_file(FAR const char *path); +int pkg_store_remove_version_dir(FAR const char *name, + FAR const char *version); const char *pkg_runtime_arch(void); const char *pkg_runtime_compat(void); @@ -160,6 +288,8 @@ int pkg_compat_check(FAR const struct pkg_manifest_s *manifest); int pkg_hash_file_sha256(FAR const char *path, FAR char digest[PKG_HASH_HEX_LEN + 1]); +int pkg_metadata_load_index_path(FAR const char *path, + FAR struct pkg_index_s *index); int pkg_metadata_load_index(FAR struct pkg_index_s *index); FAR const struct pkg_manifest_s * pkg_metadata_find_latest(FAR const struct pkg_index_s *index, @@ -178,7 +308,17 @@ const char *pkg_txn_state_str(enum pkg_txn_state_e state); int pkg_txn_write_state(FAR const char *name, enum pkg_txn_state_e state); int pkg_txn_clear_state(FAR const char *name); +bool pkg_source_is_url(FAR const char *source); +int pkg_resolve_artifact_source(FAR char *buffer, size_t size, + FAR const struct pkg_manifest_s *manifest); +int pkg_resolve_icon_source(FAR char *buffer, size_t size, + FAR const struct pkg_manifest_s *manifest); +int pkg_acquire_source(FAR const char *source, FAR const char *dest); +int pkg_sync(FAR const char *source); int pkg_install(FAR const char *name); +int pkg_uninstall(FAR const char *name); +int pkg_rollback(FAR const char *name); +int pkg_available(FAR FILE *stream); int pkg_list(FAR FILE *stream); void pkg_error(FAR const char *fmt, ...); diff --git a/system/nxpkg/pkg_compat.c b/system/nxpkg/pkg_compat.c index 301fb07aacf..b701a1d8a09 100644 --- a/system/nxpkg/pkg_compat.c +++ b/system/nxpkg/pkg_compat.c @@ -40,7 +40,16 @@ const char *pkg_runtime_arch(void) const char *pkg_runtime_compat(void) { +#ifdef CONFIG_ARCH_BOARD return CONFIG_ARCH_BOARD; +#elif defined(CONFIG_ARCH_BOARD_CUSTOM_NAME) + if (CONFIG_ARCH_BOARD_CUSTOM_NAME[0] != '\0') + { + return CONFIG_ARCH_BOARD_CUSTOM_NAME; + } +#endif + + return ""; } int pkg_compat_check(FAR const struct pkg_manifest_s *manifest) diff --git a/system/nxpkg/pkg_install.c b/system/nxpkg/pkg_install.c index c4098200191..3048342696e 100644 --- a/system/nxpkg/pkg_install.c +++ b/system/nxpkg/pkg_install.c @@ -27,8 +27,11 @@ #include #include #include +#include #include #include +#include +#include #include #include "pkg.h" @@ -37,30 +40,37 @@ * Private Functions ****************************************************************************/ -static int pkg_install_resolve_artifact(FAR char *buffer, size_t size, - FAR const struct pkg_manifest_s - *manifest) +/**************************************************************************** + * Name: pkg_install_reclaim_stale_lock + * + * Description: + * Remove "path" if it is old enough that it cannot belong to a + * still-running install (see PKG_LOCK_STALE_SECONDS). Best-effort: any + * stat()/unlink() failure just falls through to the normal -EBUSY + * result, since a lock we can't inspect should be treated as held. + * + ****************************************************************************/ + +static void pkg_install_reclaim_stale_lock(FAR const char *path) { - int ret; + struct stat st; + time_t now; - if (manifest->artifact[0] == '/') + if (stat(path, &st) < 0) { - ret = snprintf(buffer, size, "%s", manifest->artifact); - if (ret < 0) - { - return ret; - } - - return (size_t)ret >= size ? -ENAMETOOLONG : 0; + return; } - ret = snprintf(buffer, size, "%s/%s", PKG_REPO_DIR, manifest->artifact); - if (ret < 0) + now = time(NULL); + if (now < st.st_mtime || + (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) { - return ret; + return; } - return (size_t)ret >= size ? -ENAMETOOLONG : 0; + pkg_error("reclaiming stale lock '%s' (age %ld s)", + path, (long)(now - st.st_mtime)); + unlink(path); } static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, @@ -82,6 +92,12 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, } fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + if (fd < 0 && errno == EEXIST) + { + pkg_install_reclaim_stale_lock(path); + fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + } + if (fd < 0) { return errno == EEXIST ? -EBUSY : -errno; @@ -91,6 +107,82 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, return 0; } +/**************************************************************************** + * Name: pkg_install_acquire_installed_lock + * + * Description: + * The per-package lock above (pkg_install_acquire_lock()) only ever + * protects one package's own version directory/manifest - it says + * nothing about the single shared installed-packages database + * (instpkg.jsn) that every install/uninstall/rollback reads, modifies + * in memory, and writes back as a whole. Two of those operations for + * *different* packages (their own per-package locks don't conflict) + * can each load a snapshot of that shared file, add/change their own + * entry, and save - and whichever one saves last wins, silently + * discarding whatever the other one had just added. This is exactly + * what "a previously-installed package vanishes from `nxpkg list` + * with no error" looks like, without needing any corrupted file or + * non-atomic write at all: the write path is already atomic + * (pkg_store_write_text_atomic() writes to a temp file, fsyncs, then + * renames), so the *file* is always internally consistent - it just + * might be a consistent snapshot that's missing an entry a concurrent + * operation already believed it had durably saved. + * + * A separate, single global lock file (independent of any specific + * package's own lock) closes that window: acquire it right before + * pkg_metadata_load_installed() and hold it until immediately after + * pkg_metadata_save_installed(), so that whole read-modify-write + * sequence is atomic with respect to every other caller of this + * function, regardless of which package(s) they're touching. + * + * Blocking (bounded retry loop) rather than instant-EBUSY like the + * per-package lock: the critical section this protects is a handful + * of local SD-card JSON operations with no network I/O in it, so a + * real holder releases it within milliseconds - a caller should wait + * that out rather than fail an otherwise-healthy install/uninstall/ + * rollback just because another one's shared-db update was in + * flight at the same instant. + * + ****************************************************************************/ + +static int pkg_install_acquire_installed_lock(FAR char *path, size_t size) +{ + int fd; + int ret; + int tries; + + ret = snprintf(path, size, PKG_ROOT_DIR "/instpkg.lk"); + if (ret < 0) + { + return ret; + } + + if ((size_t)ret >= size) + { + return -ENAMETOOLONG; + } + + for (tries = 0; tries < 100; tries++) + { + fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + if (fd >= 0) + { + close(fd); + return 0; + } + + if (errno != EEXIST) + { + return -errno; + } + + pkg_install_reclaim_stale_lock(path); + usleep(20 * 1000); + } + + return -EBUSY; +} + static bool pkg_install_has_version( FAR const struct pkg_installed_entry_s *entry, FAR const char *version) @@ -108,6 +200,46 @@ static bool pkg_install_has_version( return false; } +static int pkg_install_prune_oldest_version( + FAR struct pkg_installed_entry_s *entry) +{ + size_t victim = entry->version_count; + size_t i; + + /* Versions are appended in install order, so the lowest index that + * isn't the active ("current") or rollback ("previous") version is the + * oldest one safe to drop. Without this, a package updated more than + * PKG_INSTALLED_VERSIONS_MAX times becomes permanently un-installable + * (pkg_install_add_version would just fail forever). + */ + + for (i = 0; i < entry->version_count; i++) + { + if (strcmp(entry->versions[i], entry->current) != 0 && + strcmp(entry->versions[i], entry->previous) != 0) + { + victim = i; + break; + } + } + + if (victim == entry->version_count) + { + return -E2BIG; + } + + pkg_store_remove_version_dir(entry->name, entry->versions[victim]); + + for (i = victim; i + 1 < entry->version_count; i++) + { + memcpy(entry->versions[i], entry->versions[i + 1], + sizeof(entry->versions[i])); + } + + entry->version_count--; + return 0; +} + static int pkg_install_add_version(FAR struct pkg_installed_entry_s *entry, FAR const char *version) { @@ -120,7 +252,11 @@ static int pkg_install_add_version(FAR struct pkg_installed_entry_s *entry, if (entry->version_count >= PKG_INSTALLED_VERSIONS_MAX) { - return -E2BIG; + ret = pkg_install_prune_oldest_version(entry); + if (ret < 0) + { + return ret; + } } ret = snprintf(entry->versions[entry->version_count], @@ -201,7 +337,7 @@ static int pkg_install_update_installed(FAR struct pkg_installed_db_s *db, static int pkg_install_write_pointers( FAR const struct pkg_installed_db_s *db, - FAR const struct pkg_manifest_s *manifest) + FAR const char *name) { FAR struct pkg_installed_entry_s *entry; char current[PATH_MAX]; @@ -209,33 +345,35 @@ static int pkg_install_write_pointers( int ret; entry = pkg_metadata_find_installed((FAR struct pkg_installed_db_s *)db, - manifest->name); + name); if (entry == NULL) { - return -ENOENT; + ret = -ENOENT; + goto out; } - ret = pkg_store_format_current_path(current, sizeof(current), - manifest->name); + ret = pkg_store_format_current_path(current, PATH_MAX, name); if (ret < 0) { - return ret; + goto out; } - ret = pkg_store_format_previous_path(previous, sizeof(previous), - manifest->name); + ret = pkg_store_format_previous_path(previous, PATH_MAX, name); if (ret < 0) { - return ret; + goto out; } ret = pkg_store_write_text_atomic(current, entry->current); if (ret < 0) { - return ret; + goto out; } - return pkg_store_write_text_atomic(previous, entry->previous); + ret = pkg_store_write_text_atomic(previous, entry->previous); + +out: + return ret; } /**************************************************************************** @@ -247,29 +385,65 @@ int pkg_install(FAR const char *name) FAR struct pkg_index_s *index; FAR struct pkg_installed_db_s *installed; FAR const struct pkg_manifest_s *manifest; - char source[PATH_MAX]; - char tmp[PATH_MAX] = ""; - char payload[PATH_MAX]; - char manifest_path[PATH_MAX]; - char lock[PATH_MAX] = ""; + FAR char *source; + FAR char *tmp; + FAR char *payload; + FAR char *manifest_path; + FAR char *lock; + FAR char *installed_lock; + FAR const char *artifact; char digest[PKG_HASH_HEX_LEN + 1]; + bool staged_to_tmp; + bool version_dir_created; + bool installed_lock_held; int ret; - index = malloc(sizeof(*index)); - installed = malloc(sizeof(*installed)); - if (index == NULL || installed == NULL) + index = pkg_zalloc(sizeof(*index)); + installed = pkg_zalloc(sizeof(*installed)); + source = pkg_path_alloc(); + tmp = pkg_path_alloc(); + payload = pkg_path_alloc(); + manifest_path = pkg_path_alloc(); + lock = pkg_path_alloc(); + installed_lock = pkg_path_alloc(); + if (index == NULL || installed == NULL || source == NULL || tmp == NULL || + payload == NULL || manifest_path == NULL || lock == NULL || + installed_lock == NULL) { - free(index); - free(installed); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("unable to allocate package metadata buffers"); return EXIT_FAILURE; } + source[0] = '\0'; + tmp[0] = '\0'; + payload[0] = '\0'; + manifest_path[0] = '\0'; + lock[0] = '\0'; + installed_lock[0] = '\0'; + installed_lock_held = false; + artifact = NULL; + staged_to_tmp = false; + version_dir_created = false; + ret = pkg_store_prepare_layout(); if (ret < 0) { - free(index); - free(installed); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("unable to prepare package layout: %d", ret); return EXIT_FAILURE; } @@ -277,8 +451,14 @@ int pkg_install(FAR const char *name) ret = pkg_metadata_load_index(index); if (ret < 0) { - free(index); - free(installed); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("unable to load local index metadata: %d", ret); return EXIT_FAILURE; } @@ -286,22 +466,44 @@ int pkg_install(FAR const char *name) manifest = pkg_metadata_find_latest(index, name); if (manifest == NULL) { - free(index); - free(installed); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("package '%s' not found in local index", name); return EXIT_FAILURE; } - ret = pkg_install_resolve_artifact(source, sizeof(source), manifest); + ret = pkg_resolve_artifact_source(source, PATH_MAX, manifest); if (ret < 0) { - pkg_error("artifact path for '%s' is too long", name); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); + pkg_error("unable to resolve artifact source for '%s': %d", name, ret); return EXIT_FAILURE; } - ret = pkg_install_acquire_lock(name, lock, sizeof(lock)); + ret = pkg_install_acquire_lock(name, lock, PATH_MAX); if (ret < 0) { + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("unable to acquire package lock for '%s': %d", name, ret); return EXIT_FAILURE; } @@ -309,123 +511,174 @@ int pkg_install(FAR const char *name) ret = pkg_txn_write_state(name, PKG_TXN_FETCHING); if (ret < 0) { + pkg_error("txn state fetching failed: %d", ret); goto errout; } - ret = pkg_store_format_download_path(tmp, sizeof(tmp), manifest->name, - manifest->version); - if (ret < 0) + if (pkg_source_is_url(source)) { - goto errout; - } + ret = pkg_store_format_download_path(tmp, PATH_MAX, manifest->name, + manifest->version); + if (ret < 0) + { + pkg_error("download path format failed: %d", ret); + goto errout; + } - ret = pkg_store_copy_file(source, tmp); - if (ret < 0) + ret = pkg_acquire_source(source, tmp); + if (ret < 0) + { + pkg_error("acquire source failed: %d", ret); + goto errout; + } + + artifact = tmp; + staged_to_tmp = true; + } + else { - goto errout; + artifact = source; } - ret = pkg_hash_file_sha256(tmp, digest); + ret = pkg_hash_file_sha256(artifact, digest); if (ret < 0) { + pkg_error("sha256 failed: %d", ret); goto errout; } if (strcasecmp(digest, manifest->sha256) != 0) { ret = -EILSEQ; + pkg_error("sha256 mismatch: %d", ret); goto errout; } ret = pkg_txn_write_state(name, PKG_TXN_VERIFIED); if (ret < 0) { + pkg_error("txn state verified failed: %d", ret); goto errout; } ret = pkg_store_ensure_version_dir(manifest->name, manifest->version); if (ret < 0) { + pkg_error("ensure version dir failed: %d", ret); goto errout; } - ret = pkg_store_format_payload_path(payload, sizeof(payload), + version_dir_created = true; + + ret = pkg_store_format_payload_path(payload, PATH_MAX, manifest->name, manifest->version, manifest->artifact); if (ret < 0) { + pkg_error("payload path format failed: %d", ret); goto errout; } - ret = pkg_store_copy_file(tmp, payload); + ret = pkg_store_copy_file(artifact, payload); if (ret < 0) { + pkg_error("copy payload failed: %d", ret); goto errout; } - ret = pkg_store_format_manifest_path(manifest_path, sizeof(manifest_path), + if (manifest->type == PKG_PAYLOAD_ELF && + chmod(payload, 0755) < 0 && errno != ENOSYS) + { + ret = -errno; + pkg_error("mark payload executable failed: %d", ret); + goto errout; + } + + ret = pkg_store_format_manifest_path(manifest_path, PATH_MAX, manifest->name, manifest->version); if (ret < 0) { + pkg_error("manifest path format failed: %d", ret); goto errout; } ret = pkg_metadata_write_manifest(manifest_path, manifest); if (ret < 0) { + pkg_error("write manifest failed: %d", ret); goto errout; } ret = pkg_txn_write_state(name, PKG_TXN_STAGED); if (ret < 0) { + pkg_error("txn state staged failed: %d", ret); goto errout; } ret = pkg_compat_check(manifest); if (ret < 0) { + pkg_error("compat check failed: %d", ret); goto errout; } ret = pkg_txn_write_state(name, PKG_TXN_COMPAT_OK); if (ret < 0) { + pkg_error("txn state compat_ok failed: %d", ret); + goto errout; + } + + ret = pkg_install_acquire_installed_lock(installed_lock, PATH_MAX); + if (ret < 0) + { + pkg_error("unable to acquire installed-db lock: %d", ret); goto errout; } + installed_lock_held = true; + ret = pkg_metadata_load_installed(installed); if (ret < 0) { + pkg_error("load installed metadata failed: %d", ret); goto errout; } ret = pkg_install_update_installed(installed, manifest); if (ret < 0) { + pkg_error("update installed metadata failed: %d", ret); goto errout; } - ret = pkg_install_write_pointers(installed, manifest); + ret = pkg_install_write_pointers(installed, manifest->name); if (ret < 0) { + pkg_error("write current/previous pointers failed: %d", ret); goto errout; } ret = pkg_metadata_save_installed(installed); if (ret < 0) { + pkg_error("save installed metadata failed: %d", ret); goto errout; } + pkg_store_remove_file(installed_lock); + installed_lock_held = false; + ret = pkg_txn_write_state(name, PKG_TXN_ACTIVATED); if (ret < 0) { + pkg_error("txn state activated failed: %d", ret); goto errout; } pkg_txn_write_state(name, PKG_TXN_CLEANUP); - if (tmp[0] != '\0') + if (staged_to_tmp && tmp[0] != '\0') { pkg_store_remove_file(tmp); } @@ -437,27 +690,67 @@ int pkg_install(FAR const char *name) } pkg_info("installed %s version %s", manifest->name, manifest->version); - free(index); - free(installed); + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); return EXIT_SUCCESS; errout: pkg_txn_write_state(name, PKG_TXN_FAILED); - if (tmp[0] != '\0') + if (staged_to_tmp && tmp[0] != '\0') { pkg_store_remove_file(tmp); } + /* Reclaim whatever was staged into the version directory (payload, + * manifest.jsn) before this failure - otherwise every failure past + * this point leaves a permanently orphaned, never-activated version + * directory with no way to reclaim it short of "remove". + */ + + if (version_dir_created) + { + pkg_store_remove_version_dir(manifest->name, manifest->version); + } + pkg_txn_clear_state(name); if (lock[0] != '\0') { pkg_store_remove_file(lock); } - free(index); - free(installed); + if (installed_lock_held) + { + pkg_store_remove_file(installed_lock); + } + + pkg_free(index); + pkg_free(installed); + pkg_free(source); + pkg_free(tmp); + pkg_free(payload); + pkg_free(manifest_path); + pkg_free(lock); + pkg_free(installed_lock); pkg_error("install failed for '%s': %d", name, ret); - return EXIT_FAILURE; + + /* Propagate the real negative errno (rather than the constant + * EXIT_FAILURE) here specifically, since every meaningful pipeline + * failure - network/download, sha256 mismatch (-EILSEQ), wrong + * arch/board (-ENOEXEC/-EXDEV from pkg_compat_check) - funnels through + * this handler. nxstore calls pkg_install() directly (not through a + * shell) and can use this to show a differentiated message instead of + * one generic "install failed" string; any nonzero value (this is + * always < 0) still satisfies the plain success/failure contract for + * callers that only check for zero. + */ + + return ret; } int pkg_list(FAR FILE *stream) @@ -465,7 +758,7 @@ int pkg_list(FAR FILE *stream) FAR struct pkg_installed_db_s *db; int ret; - db = malloc(sizeof(*db)); + db = pkg_zalloc(sizeof(*db)); if (db == NULL) { pkg_error("unable to allocate installed metadata buffer"); @@ -475,7 +768,7 @@ int pkg_list(FAR FILE *stream) ret = pkg_store_prepare_layout(); if (ret < 0) { - free(db); + pkg_free(db); pkg_error("unable to prepare package layout: %d", ret); return EXIT_FAILURE; } @@ -483,7 +776,7 @@ int pkg_list(FAR FILE *stream) ret = pkg_metadata_load_installed(db); if (ret < 0) { - free(db); + pkg_free(db); pkg_error("unable to load installed metadata: %d", ret); return EXIT_FAILURE; } @@ -491,11 +784,300 @@ int pkg_list(FAR FILE *stream) ret = pkg_metadata_print_installed(stream, db); if (ret < 0) { - free(db); + pkg_free(db); pkg_error("unable to print installed metadata: %d", ret); return EXIT_FAILURE; } - free(db); + pkg_free(db); + return EXIT_SUCCESS; +} + +/**************************************************************************** + * Name: pkg_uninstall + * + * Description: + * Remove every installed version of "name": their version directories + * (payload + manifest.jsn), the current/previous pointer files, any + * leftover txn.tx, the entry in the shared installed-packages database, + * and finally the now-empty package root directory. Refuses to run + * while an install/update for the same package is in flight (a live + * lock.lk), since removing the store out from under it would corrupt + * whatever it's mid-writing. + * + ****************************************************************************/ + +int pkg_uninstall(FAR const char *name) +{ + FAR struct pkg_installed_db_s *db; + FAR struct pkg_installed_entry_s *entry; + char path[PATH_MAX]; + char installed_lock[PATH_MAX]; + size_t index; + size_t i; + int ret; + + if (name == NULL || name[0] == '\0') + { + pkg_error("remove requires a package name"); + return EXIT_FAILURE; + } + + db = pkg_zalloc(sizeof(*db)); + if (db == NULL) + { + pkg_error("unable to allocate installed metadata buffer"); + return EXIT_FAILURE; + } + + ret = pkg_store_prepare_layout(); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to prepare package layout: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_install_acquire_installed_lock(installed_lock, + sizeof(installed_lock)); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to acquire installed-db lock: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_metadata_load_installed(db); + if (ret < 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("unable to load installed metadata: %d", ret); + return EXIT_FAILURE; + } + + entry = pkg_metadata_find_installed(db, name); + if (entry == NULL) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("package '%s' is not installed", name); + return EXIT_FAILURE; + } + + if (pkg_store_format_lock_path(path, sizeof(path), name) == 0 && + access(path, F_OK) == 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("package '%s' has an install/update in progress", name); + return EXIT_FAILURE; + } + + for (i = 0; i < entry->version_count; i++) + { + pkg_store_remove_version_dir(name, entry->versions[i]); + } + + if (pkg_store_format_txn_path(path, sizeof(path), name) == 0) + { + pkg_store_remove_file(path); + } + + /* Drop this entry from the in-memory db (shift the tail down over it) + * and persist before touching any more of the on-disk layout. + */ + + index = (size_t)(entry - db->entries); + for (i = index; i + 1 < db->count; i++) + { + db->entries[i] = db->entries[i + 1]; + } + + db->count--; + + ret = pkg_metadata_save_installed(db); + pkg_store_remove_file(installed_lock); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to save installed metadata: %d", ret); + return EXIT_FAILURE; + } + + if (pkg_store_format_current_path(path, sizeof(path), name) == 0) + { + pkg_store_remove_file(path); + } + + if (pkg_store_format_previous_path(path, sizeof(path), name) == 0) + { + pkg_store_remove_file(path); + } + + if (pkg_store_format_package_root(path, sizeof(path), name) == 0) + { + rmdir(path); + } + + pkg_info("removed %s", name); + pkg_free(db); + return EXIT_SUCCESS; +} + +/**************************************************************************** + * Name: pkg_rollback + * + * Description: + * Swap "name"'s current and previous installed versions. The swap (as + * opposed to just clearing "previous") lets a second rollback undo the + * first. Verifies the rollback target's version directory still + * exists on disk before committing any state change, and refuses to + * run while an install/update for the same package is in flight. + * + ****************************************************************************/ + +int pkg_rollback(FAR const char *name) +{ + FAR struct pkg_installed_db_s *db; + FAR struct pkg_installed_entry_s *entry; + char version_path[PATH_MAX]; + char lock_path[PATH_MAX]; + char installed_lock[PATH_MAX]; + char swap[PKG_VERSION_MAX + 1]; + struct stat st; + int ret; + + if (name == NULL || name[0] == '\0') + { + pkg_error("rollback requires a package name"); + return EXIT_FAILURE; + } + + db = pkg_zalloc(sizeof(*db)); + if (db == NULL) + { + pkg_error("unable to allocate installed metadata buffer"); + return EXIT_FAILURE; + } + + ret = pkg_store_prepare_layout(); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to prepare package layout: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_install_acquire_installed_lock(installed_lock, + sizeof(installed_lock)); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to acquire installed-db lock: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_metadata_load_installed(db); + if (ret < 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("unable to load installed metadata: %d", ret); + return EXIT_FAILURE; + } + + entry = pkg_metadata_find_installed(db, name); + if (entry == NULL) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("package '%s' is not installed", name); + return EXIT_FAILURE; + } + + if (entry->previous[0] == '\0') + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("package '%s' has no previous version to roll back to", + name); + return EXIT_FAILURE; + } + + if (pkg_store_format_lock_path(lock_path, sizeof(lock_path), name) == 0 && + access(lock_path, F_OK) == 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("package '%s' has an install/update in progress", name); + return EXIT_FAILURE; + } + + ret = pkg_store_format_version_path(version_path, sizeof(version_path), + name, entry->previous); + if (ret < 0 || stat(version_path, &st) < 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("rollback target version '%s' is missing on disk", + entry->previous); + return EXIT_FAILURE; + } + + ret = snprintf(swap, sizeof(swap), "%s", entry->current); + if (ret < 0 || (size_t)ret >= sizeof(swap)) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("current version string too long to swap"); + return EXIT_FAILURE; + } + + ret = snprintf(entry->current, sizeof(entry->current), "%s", + entry->previous); + if (ret < 0 || (size_t)ret >= sizeof(entry->current)) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("unable to update current version"); + return EXIT_FAILURE; + } + + ret = snprintf(entry->previous, sizeof(entry->previous), "%s", swap); + if (ret < 0 || (size_t)ret >= sizeof(entry->previous)) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("unable to update previous version"); + return EXIT_FAILURE; + } + + /* Persist the pointer-file swap and the db entry together, in that + * order, before releasing anything - a crash between these two writes + * still leaves "current" resolving to the (now rolled-back-to) version + * that's actually on disk, which is the side that must win. + */ + + ret = pkg_install_write_pointers(db, name); + if (ret < 0) + { + pkg_store_remove_file(installed_lock); + pkg_free(db); + pkg_error("unable to write current/previous pointers: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_metadata_save_installed(db); + pkg_store_remove_file(installed_lock); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to save installed metadata: %d", ret); + return EXIT_FAILURE; + } + + pkg_info("rolled back %s to version %s", name, entry->current); + pkg_free(db); return EXIT_SUCCESS; } diff --git a/system/nxpkg/pkg_log.c b/system/nxpkg/pkg_log.c index bed06b13ffb..658f6b64f68 100644 --- a/system/nxpkg/pkg_log.c +++ b/system/nxpkg/pkg_log.c @@ -26,6 +26,8 @@ #include #include +#include +#include #include "pkg.h" @@ -33,13 +35,19 @@ * Private Functions ****************************************************************************/ -static void pkg_vlog(FAR FILE *stream, FAR const char *level, - FAR const char *fmt, va_list ap) +static void pkg_vlog(FAR const char *level, FAR const char *fmt, va_list ap) { - fprintf(stream, "nxpkg: %s: ", level); - vfprintf(stream, fmt, ap); - fputc('\n', stream); - fflush(stream); + char message[256]; + int ret; + + ret = vsnprintf(message, sizeof(message), fmt, ap); + if (ret < 0) + { + return; + } + + syslog(strcmp(level, "error") == 0 ? LOG_ERR : LOG_INFO, + "nxpkg: %s: %s", level, message); } /**************************************************************************** @@ -51,7 +59,7 @@ void pkg_error(FAR const char *fmt, ...) va_list ap; va_start(ap, fmt); - pkg_vlog(stderr, "error", fmt, ap); + pkg_vlog("error", fmt, ap); va_end(ap); } @@ -60,6 +68,6 @@ void pkg_info(FAR const char *fmt, ...) va_list ap; va_start(ap, fmt); - pkg_vlog(stdout, "info", fmt, ap); + pkg_vlog("info", fmt, ap); va_end(ap); } diff --git a/system/nxpkg/pkg_main.c b/system/nxpkg/pkg_main.c index c049592c5b6..f20137f9ec5 100644 --- a/system/nxpkg/pkg_main.c +++ b/system/nxpkg/pkg_main.c @@ -29,8 +29,18 @@ #include #include +#include + #include "pkg.h" +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define PKG_USAGE \ + "Usage: %s " \ + "[args]\n" + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -38,12 +48,15 @@ int main(int argc, FAR char *argv[]) { FAR const char *cmd; + cJSON_Hooks hooks; + + hooks.malloc_fn = malloc; + hooks.free_fn = free; + cJSON_InitHooks(&hooks); if (argc < 2) { - fprintf(stderr, - "Usage: %s [args]\n", - argv[0]); + fprintf(stderr, PKG_USAGE, argv[0]); return EXIT_FAILURE; } @@ -52,9 +65,7 @@ int main(int argc, FAR char *argv[]) if (strcmp(cmd, "help") == 0 || strcmp(cmd, "--help") == 0 || strcmp(cmd, "-h") == 0) { - fprintf(stdout, - "Usage: %s [args]\n", - argv[0]); + fprintf(stdout, PKG_USAGE, argv[0]); return EXIT_SUCCESS; } @@ -63,19 +74,59 @@ int main(int argc, FAR char *argv[]) if (argc != 3) { pkg_error("install expects exactly one package name"); - fprintf(stderr, - "Usage: %s [args]\n", - argv[0]); + fprintf(stderr, PKG_USAGE, argv[0]); return EXIT_FAILURE; } - return pkg_install(argv[2]); + /* pkg_install() returns a real negative errno on the meaningful + * pipeline failures (nxstore uses that directly), not just + * EXIT_SUCCESS/EXIT_FAILURE - normalize to a plain 0/1 shell exit + * status here. + */ + + return pkg_install(argv[2]) == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } + /* "update" is a package name resolving to whatever version is latest + * in the local index - pkg_install() already handles the "already + * installed at a different version" transition transparently via + * pkg_install_update_installed(), so no separate code path is needed. + */ + if (strcmp(cmd, "update") == 0) { - pkg_error("'update' is not implemented yet in the current unit"); - return EXIT_FAILURE; + if (argc != 3) + { + pkg_error("update expects exactly one package name"); + fprintf(stderr, PKG_USAGE, argv[0]); + return EXIT_FAILURE; + } + + return pkg_install(argv[2]) == 0 ? EXIT_SUCCESS : EXIT_FAILURE; + } + + if (strcmp(cmd, "remove") == 0 || strcmp(cmd, "uninstall") == 0) + { + if (argc != 3) + { + pkg_error("remove expects exactly one package name"); + fprintf(stderr, PKG_USAGE, argv[0]); + return EXIT_FAILURE; + } + + return pkg_uninstall(argv[2]); + } + + if (strcmp(cmd, "rollback") == 0) + { + if (argc != 3) + { + pkg_error("rollback expects exactly one package name"); + fprintf(stderr, PKG_USAGE, argv[0]); + return EXIT_FAILURE; + } + + return pkg_rollback(argv[2]); } if (strcmp(cmd, "list") == 0) @@ -83,24 +134,38 @@ int main(int argc, FAR char *argv[]) if (argc != 2) { pkg_error("list does not take additional arguments"); - fprintf(stderr, - "Usage: %s [args]\n", - argv[0]); + fprintf(stderr, PKG_USAGE, argv[0]); return EXIT_FAILURE; } return pkg_list(stdout); } - if (strcmp(cmd, "rollback") == 0) + if (strcmp(cmd, "available") == 0) { - pkg_error("'rollback' is not implemented yet in the current unit"); - return EXIT_FAILURE; + if (argc != 2) + { + pkg_error("available does not take additional arguments"); + fprintf(stderr, PKG_USAGE, argv[0]); + return EXIT_FAILURE; + } + + return pkg_available(stdout); + } + + if (strcmp(cmd, "sync") == 0) + { + if (argc != 3) + { + pkg_error("sync expects exactly one index source"); + fprintf(stderr, PKG_USAGE, argv[0]); + return EXIT_FAILURE; + } + + return pkg_sync(argv[2]); } fprintf(stderr, "ERROR: Unknown subcommand '%s'\n", cmd); - fprintf(stderr, - "Usage: %s [args]\n", - argv[0]); + fprintf(stderr, PKG_USAGE, argv[0]); return EXIT_FAILURE; } diff --git a/system/nxpkg/pkg_manifest.c b/system/nxpkg/pkg_manifest.c index 3e7b56a2637..4c09803851e 100644 --- a/system/nxpkg/pkg_manifest.c +++ b/system/nxpkg/pkg_manifest.c @@ -74,6 +74,49 @@ static bool pkg_validate_hex(FAR const char *value) return true; } +/**************************************************************************** + * Name: pkg_validate_path_component + * + * Description: + * Reject any value that could escape the intended directory when spliced + * into a filesystem path (pkg_store.c's PKG_STORE_DIR "/%s/%s/..." + * formatters). This is required for "name" and "version" specifically, + * since both come straight from an untrusted, network-fetched + * index.json and are used unsanitized to build install paths - a + * version of "../../evil" would otherwise let a malicious index write + * or delete files outside the package store entirely. + * + ****************************************************************************/ + +static bool pkg_validate_path_component(FAR const char *value) +{ + FAR const char *p; + + if (pkg_validate_required(value) < 0) + { + return false; + } + + /* Reject a leading '.' outright: blocks ".", "..", and any + * "../"-prefixed traversal in one check. + */ + + if (value[0] == '.') + { + return false; + } + + for (p = value; *p != '\0'; p++) + { + if (*p == '/' || *p == '\\') + { + return false; + } + } + + return true; +} + /**************************************************************************** * Public Functions ****************************************************************************/ @@ -95,6 +138,8 @@ const char *pkg_manifest_type_str(enum pkg_payload_type_e type) int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) { + size_t i; + if (manifest == NULL) { return -EINVAL; @@ -110,6 +155,19 @@ int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) return -EINVAL; } + /* "name" and "version" get spliced unsanitized into on-disk paths + * (pkg_store.c) - they must not contain path separators or traversal + * sequences. "artifact" is validated separately in pkg_repo.c, where + * it's legitimately allowed to be a relative repo path (just not an + * absolute one or one that escapes the repo root). + */ + + if (!pkg_validate_path_component(manifest->name) || + !pkg_validate_path_component(manifest->version)) + { + return -EINVAL; + } + if (strlen(manifest->sha256) != PKG_HASH_HEX_LEN) { return -EINVAL; @@ -126,6 +184,19 @@ int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest) return -EINVAL; } + if (manifest->launch_argc > PKG_LAUNCH_ARGS_MAX) + { + return -EINVAL; + } + + for (i = 0; i < manifest->launch_argc; i++) + { + if (pkg_validate_required(manifest->launch_args[i]) < 0) + { + return -EINVAL; + } + } + return 0; } diff --git a/system/nxpkg/pkg_metadata.c b/system/nxpkg/pkg_metadata.c index aadb4692e58..38907f59fd0 100644 --- a/system/nxpkg/pkg_metadata.c +++ b/system/nxpkg/pkg_metadata.c @@ -100,6 +100,54 @@ static FAR cJSON *pkg_metadata_packages_array(FAR cJSON *root) return cJSON_GetObjectItemCaseSensitive(root, "packages"); } +static int pkg_metadata_parse_launch_args( + FAR cJSON *item, FAR struct pkg_manifest_s *manifest) +{ + FAR cJSON *field; + FAR cJSON *arg; + size_t argc = 0; + FAR const char *value; + int ret; + + field = cJSON_GetObjectItemCaseSensitive(item, "launch_args"); + if (field == NULL) + { + manifest->launch_argc = 0; + return 0; + } + + if (!cJSON_IsArray(field)) + { + return -EINVAL; + } + + cJSON_ArrayForEach(arg, field) + { + if (argc >= PKG_LAUNCH_ARGS_MAX) + { + return -E2BIG; + } + + value = cJSON_GetStringValue(arg); + if (value == NULL) + { + return -EINVAL; + } + + ret = pkg_copy_string(manifest->launch_args[argc], + sizeof(manifest->launch_args[argc]), value); + if (ret < 0) + { + return ret; + } + + argc++; + } + + manifest->launch_argc = argc; + return 0; +} + static int pkg_metadata_parse_manifest(FAR cJSON *item, FAR struct pkg_manifest_s *manifest) { @@ -170,6 +218,39 @@ static int pkg_metadata_parse_manifest(FAR cJSON *item, return -EINVAL; } + /* description/category/icon are optional and purely for UI display; + * missing fields just leave the manifest's copy empty. + */ + + field = cJSON_GetObjectItemCaseSensitive(item, "description"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->description, sizeof(manifest->description), + value); + } + + field = cJSON_GetObjectItemCaseSensitive(item, "category"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->category, sizeof(manifest->category), + value); + } + + field = cJSON_GetObjectItemCaseSensitive(item, "icon"); + value = cJSON_GetStringValue(field); + if (value != NULL) + { + pkg_copy_string(manifest->icon, sizeof(manifest->icon), value); + } + + ret = pkg_metadata_parse_launch_args(item, manifest); + if (ret < 0) + { + return ret; + } + return pkg_manifest_validate(manifest); } @@ -395,6 +476,8 @@ static FAR cJSON *pkg_metadata_manifest_to_json( FAR const struct pkg_manifest_s *manifest) { FAR cJSON *root; + FAR cJSON *launch_args; + size_t i; root = cJSON_CreateObject(); if (root == NULL) @@ -410,51 +493,55 @@ static FAR cJSON *pkg_metadata_manifest_to_json( cJSON_AddStringToObject(root, "sha256", manifest->sha256); cJSON_AddStringToObject(root, "type", pkg_manifest_type_str(manifest->type)); - return root; -} -/**************************************************************************** - * Public Functions - ****************************************************************************/ - -int pkg_metadata_load_index(FAR struct pkg_index_s *index) -{ - FAR cJSON *root; - FAR cJSON *packages; - FAR cJSON *item; - FAR char *text; - char path[PATH_MAX]; - size_t count = 0; - size_t textlen; - int ret; - - if (index == NULL) + if (manifest->description[0] != '\0') { - return -EINVAL; + cJSON_AddStringToObject(root, "description", manifest->description); } - memset(index, 0, sizeof(*index)); - - ret = pkg_store_format_index_path(path, sizeof(path)); - if (ret < 0) + if (manifest->category[0] != '\0') { - return ret; + cJSON_AddStringToObject(root, "category", manifest->category); } - pkg_info("loading index from %s", path); - - ret = pkg_store_read_text(path, &text); - if (ret < 0) + if (manifest->launch_argc > 0) { - return ret; + launch_args = cJSON_AddArrayToObject(root, "launch_args"); + if (launch_args == NULL) + { + cJSON_Delete(root); + return NULL; + } + + for (i = 0; i < manifest->launch_argc; i++) + { + FAR cJSON *arg; + + arg = cJSON_CreateString(manifest->launch_args[i]); + if (arg == NULL) + { + cJSON_Delete(root); + return NULL; + } + + cJSON_AddItemToArray(launch_args, arg); + } } - textlen = strlen(text); - pkg_info("index read complete (%zu bytes)", textlen); + return root; +} + +static int pkg_metadata_parse_index_text(FAR const char *text, + FAR struct pkg_index_s *index) +{ + FAR cJSON *root; + FAR cJSON *packages; + FAR cJSON *item; + size_t count = 0; + int ret; root = cJSON_Parse(text); pkg_info("cJSON_Parse returned %s", root != NULL ? "success" : "failure"); - free(text); if (root == NULL) { return -EINVAL; @@ -471,15 +558,27 @@ int pkg_metadata_load_index(FAR struct pkg_index_s *index) { if (count >= PKG_INDEX_MAX) { - cJSON_Delete(root); - return -E2BIG; + /* Keep what's already parsed rather than discarding the whole + * index: a catalog that's grown past PKG_INDEX_MAX shouldn't + * make every other package unavailable too. + */ + + pkg_error("index has more than %d packages, truncating", + PKG_INDEX_MAX); + break; } ret = pkg_metadata_parse_manifest(item, &index->manifests[count]); if (ret < 0) { - cJSON_Delete(root); - return ret; + /* Skip a malformed entry instead of discarding the entire + * index: one bad/malicious package definition shouldn't make + * every other, otherwise-valid package unavailable too. + */ + + pkg_error("skipping malformed package entry %zu: %d", count, + ret); + continue; } pkg_info("parsed manifest %s %s", @@ -493,6 +592,54 @@ int pkg_metadata_load_index(FAR struct pkg_index_s *index) return 0; } +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +int pkg_metadata_load_index_path(FAR const char *path, + FAR struct pkg_index_s *index) +{ + FAR char *text; + size_t textlen; + int ret; + + if (path == NULL || index == NULL) + { + return -EINVAL; + } + + memset(index, 0, sizeof(*index)); + + pkg_info("loading index from %s", path); + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + textlen = strlen(text); + pkg_info("index read complete (%zu bytes)", textlen); + + ret = pkg_metadata_parse_index_text(text, index); + pkg_free(text); + return ret; +} + +int pkg_metadata_load_index(FAR struct pkg_index_s *index) +{ + char path[PATH_MAX]; + int ret; + + ret = pkg_store_format_index_path(path, sizeof(path)); + if (ret < 0) + { + return ret; + } + + return pkg_metadata_load_index_path(path, index); +} + FAR const struct pkg_manifest_s * pkg_metadata_find_latest(FAR const struct pkg_index_s *index, FAR const char *name) @@ -573,7 +720,7 @@ int pkg_metadata_load_installed(FAR struct pkg_installed_db_s *db) } root = cJSON_Parse(text); - free(text); + pkg_free(text); if (root == NULL) { return -EINVAL; @@ -590,15 +737,23 @@ int pkg_metadata_load_installed(FAR struct pkg_installed_db_s *db) { if (count >= PKG_INSTALLED_MAX) { - cJSON_Delete(root); - return -E2BIG; + pkg_error("installed db has more than %d entries, truncating", + PKG_INSTALLED_MAX); + break; } ret = pkg_metadata_parse_installed_entry(item, &db->entries[count]); if (ret < 0) { - cJSON_Delete(root); - return ret; + /* A single corrupted entry (plausible after a crash mid-write, + * despite the atomic-write mechanism) must not make every + * other installed package look uninstalled - that would drive + * needless reinstalls for everything else. + */ + + pkg_error("skipping malformed installed entry %zu: %d", count, + ret); + continue; } count++; diff --git a/system/nxpkg/pkg_repo.c b/system/nxpkg/pkg_repo.c new file mode 100644 index 00000000000..6ea294ca86a --- /dev/null +++ b/system/nxpkg/pkg_repo.c @@ -0,0 +1,612 @@ +/**************************************************************************** + * apps/system/nxpkg/pkg_repo.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#ifdef CONFIG_NETUTILS_WEBCLIENT +# include "netutils/webclient.h" +#endif + +#include "pkg.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Each webclient_perform() read/sink cycle costs a TCP receive plus an + * SD-card write call; at the old 512-byte size, a sub-1MB file like + * nxdoom's ~940KB ELF took ~1900 round trips and, in practice, close + * to two minutes to install - easily read as "stuck" with only a bare + * spinner for feedback. 4KB cuts that to ~230 round trips and lines + * up with typical SD card erase-block granularity, which also reduces + * write amplification. Still small enough to be a safe stack-local + * buffer against the 16KB (CLI) / 16KB (nxstore install worker) task + * stacks that call into this. + */ + +#define PKG_REPO_FETCH_BUFFER_SIZE 4096 + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +#ifdef CONFIG_NETUTILS_WEBCLIENT +struct pkg_fetch_context_s +{ + int fd; + size_t total; +}; +#endif + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int pkg_repo_copy_string(FAR char *buffer, size_t size, + FAR const char *value) +{ + int ret; + + ret = snprintf(buffer, size, "%s", value); + if (ret < 0) + { + return ret; + } + + return (size_t)ret >= size ? -ENAMETOOLONG : 0; +} + +static int pkg_repo_source_base(FAR char *buffer, size_t size, + FAR const char *source) +{ + FAR const char *slash; + size_t length; + + slash = strrchr(source, '/'); + if (slash == NULL) + { + return -EINVAL; + } + + length = (size_t)(slash - source); + if (length == 0) + { + length = 1; + } + + if (length >= size) + { + return -ENAMETOOLONG; + } + + memcpy(buffer, source, length); + buffer[length] = '\0'; + return 0; +} + +/**************************************************************************** + * Name: pkg_validate_artifact_relative + * + * Description: + * manifest->artifact is repo-relative content and gets spliced into a + * local filesystem path (or used to build a URL) unsanitized. Reject + * absolute paths outright - allowing them let a malicious index turn + * any local file with a known/predictable hash into an "installed" + * package, including making it executable - and reject any ".." path + * segment that would let the artifact escape the repo mirror directory. + * + ****************************************************************************/ + +static bool pkg_validate_artifact_relative(FAR const char *value) +{ + FAR const char *p; + + if (value == NULL || value[0] == '\0' || value[0] == '/') + { + return false; + } + + p = value; + while ((p = strstr(p, "..")) != NULL) + { + bool at_start = p == value || *(p - 1) == '/'; + bool at_end = p[2] == '\0' || p[2] == '/'; + + if (at_start && at_end) + { + return false; + } + + p++; + } + + return true; +} + +static int pkg_repo_read_source(FAR char *buffer, size_t size) +{ + char path[PATH_MAX]; + FAR char *text; + size_t length; + int ret; + + ret = pkg_store_format_repo_source_path(path, sizeof(path)); + if (ret < 0) + { + return ret; + } + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + length = strlen(text); + while (length > 0 && isspace((unsigned char)text[length - 1])) + { + text[--length] = '\0'; + } + + ret = pkg_repo_copy_string(buffer, size, text); + pkg_free(text); + return ret; +} + +#ifdef CONFIG_NETUTILS_WEBCLIENT +static int pkg_repo_sink(FAR char **buffer, int offset, int datend, + FAR int *buflen, FAR void *arg) +{ + FAR struct pkg_fetch_context_s *ctx; + size_t remaining; + FAR char *cursor; + + UNUSED(buffer); + UNUSED(buflen); + + ctx = arg; + cursor = &(*buffer)[offset]; + remaining = (size_t)(datend - offset); + + /* Cap total downloaded bytes: an unbounded/malicious response could + * otherwise exhaust all SD-card space. Checked before writing more so + * the on-disk file never exceeds the cap even mid-chunk. + */ + + if (remaining > 0 && + (ctx->total > PKG_DOWNLOAD_MAX_SIZE || + remaining > PKG_DOWNLOAD_MAX_SIZE - ctx->total)) + { + return -EFBIG; + } + + ctx->total += remaining; + + while (remaining > 0) + { + ssize_t nwritten; + + nwritten = write(ctx->fd, cursor, remaining); + if (nwritten < 0) + { + if (errno == EINTR) + { + continue; + } + + return -errno; + } + + cursor += nwritten; + remaining -= (size_t)nwritten; + } + + return 0; +} + +static int pkg_repo_fetch_url(FAR const char *url, FAR const char *dest) +{ + struct pkg_fetch_context_s fetch; + struct webclient_context client; + char reason[64]; + char buffer[PKG_REPO_FETCH_BUFFER_SIZE]; + int ret; + + fetch.fd = open(dest, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (fetch.fd < 0) + { + return -errno; + } + + fetch.total = 0; + + webclient_set_defaults(&client); + client.method = "GET"; + client.url = url; + client.buffer = buffer; + client.buflen = sizeof(buffer); + client.sink_callback = pkg_repo_sink; + client.sink_callback_arg = &fetch; + client.http_reason = reason; + client.http_reason_len = sizeof(reason); + + ret = webclient_perform(&client); + if (ret < 0) + { + close(fetch.fd); + unlink(dest); + return ret; + } + + if (client.http_status / 100 != 2) + { + close(fetch.fd); + unlink(dest); + return -EPROTO; + } + + if (close(fetch.fd) < 0) + { + unlink(dest); + return -errno; + } + + return 0; +} +#endif + +static int pkg_resolve_relative_source(FAR char *buffer, size_t size, + FAR const char *relative) +{ + char source[PATH_MAX]; + char base[PATH_MAX]; + int ret; + + if (buffer == NULL || relative == NULL || relative[0] == '\0') + { + return -EINVAL; + } + + if (pkg_source_is_url(relative)) + { + return pkg_repo_copy_string(buffer, size, relative); + } + + if (!pkg_validate_artifact_relative(relative)) + { + return -EINVAL; + } + + ret = pkg_repo_read_source(source, sizeof(source)); + if (ret >= 0) + { + ret = pkg_repo_source_base(base, sizeof(base), source); + if (ret < 0) + { + return ret; + } + + ret = snprintf(buffer, size, "%s/%s", base, relative); + if (ret < 0) + { + return ret; + } + + return (size_t)ret >= size ? -ENAMETOOLONG : 0; + } + + ret = snprintf(buffer, size, "%s/%s", PKG_REPO_DIR, relative); + if (ret < 0) + { + return ret; + } + + return (size_t)ret >= size ? -ENAMETOOLONG : 0; +} + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +bool pkg_source_is_url(FAR const char *source) +{ + if (source == NULL) + { + return false; + } + + return strncasecmp(source, "http://", 7) == 0 || + strncasecmp(source, "https://", 8) == 0; +} + +int pkg_resolve_artifact_source(FAR char *buffer, size_t size, + FAR const struct pkg_manifest_s *manifest) +{ + if (manifest == NULL) + { + return -EINVAL; + } + + return pkg_resolve_relative_source(buffer, size, manifest->artifact); +} + +int pkg_resolve_icon_source(FAR char *buffer, size_t size, + FAR const struct pkg_manifest_s *manifest) +{ + if (manifest == NULL) + { + return -EINVAL; + } + + return pkg_resolve_relative_source(buffer, size, manifest->icon); +} + +int pkg_acquire_source(FAR const char *source, FAR const char *dest) +{ + if (source == NULL || dest == NULL) + { + return -EINVAL; + } + + if (pkg_source_is_url(source)) + { +#ifdef CONFIG_NETUTILS_WEBCLIENT + return pkg_repo_fetch_url(source, dest); +#else + return -ENOSYS; +#endif + } + + return pkg_store_copy_file(source, dest); +} + +int pkg_sync(FAR const char *source) +{ + FAR struct pkg_index_s *index; + FAR char *text = NULL; + FAR char *tmp; + FAR char *index_path; + FAR char *source_path; + int ret; + + if (source == NULL || source[0] == '\0') + { + pkg_error("sync requires a non-empty index source"); + return EXIT_FAILURE; + } + + index = pkg_zalloc(sizeof(*index)); + tmp = pkg_path_alloc(); + index_path = pkg_path_alloc(); + source_path = pkg_path_alloc(); + if (index == NULL || tmp == NULL || index_path == NULL || + source_path == NULL) + { + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_error("unable to allocate index metadata buffer"); + return EXIT_FAILURE; + } + + ret = pkg_store_prepare_layout(); + if (ret < 0) + { + pkg_error("unable to prepare package layout: %d", ret); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + return EXIT_FAILURE; + } + + ret = snprintf(tmp, PATH_MAX, "%s/idxsync.jsn", PKG_TMP_DIR); + if (ret < 0 || (size_t)ret >= PATH_MAX) + { + pkg_error("temporary sync path is too long"); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + return EXIT_FAILURE; + } + + ret = pkg_acquire_source(source, tmp); + if (ret < 0) + { + pkg_error("unable to fetch index source '%s': %d", source, ret); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + return EXIT_FAILURE; + } + + ret = pkg_metadata_load_index_path(tmp, index); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_error("downloaded index is invalid: %d", ret); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + return EXIT_FAILURE; + } + + ret = pkg_store_read_text(tmp, &text); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_error("unable to read fetched index: %d", ret); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + return EXIT_FAILURE; + } + + ret = pkg_store_format_index_path(index_path, PATH_MAX); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_error("unable to resolve local index path: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_store_write_text_atomic(index_path, text); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_error("unable to write local index: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_store_format_repo_source_path(source_path, PATH_MAX); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_error("unable to resolve repository source path: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_store_write_text_atomic(source_path, source); + if (ret < 0) + { + pkg_store_remove_file(tmp); + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_error("unable to save repository source: %d", ret); + return EXIT_FAILURE; + } + + pkg_store_remove_file(tmp); + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + pkg_free(source_path); + pkg_info("synced package index from %s", source); + return EXIT_SUCCESS; +} + +int pkg_available(FAR FILE *stream) +{ + FAR struct pkg_index_s *index; + FAR const char *arch; + FAR const char *compat; + size_t i; + int ret; + + if (stream == NULL) + { + return EXIT_FAILURE; + } + + index = pkg_zalloc(sizeof(*index)); + if (index == NULL) + { + pkg_error("unable to allocate index metadata buffer"); + return EXIT_FAILURE; + } + + ret = pkg_store_prepare_layout(); + if (ret < 0) + { + pkg_free(index); + pkg_error("unable to prepare package layout: %d", ret); + return EXIT_FAILURE; + } + + ret = pkg_metadata_load_index(index); + if (ret < 0) + { + pkg_free(index); + pkg_error("unable to load package index: %d", ret); + return EXIT_FAILURE; + } + + arch = pkg_runtime_arch(); + compat = pkg_runtime_compat(); + + for (i = 0; i < index->count; i++) + { + FAR const struct pkg_manifest_s *manifest = &index->manifests[i]; + FAR const struct pkg_manifest_s *latest; + + if (strcmp(manifest->arch, arch) != 0 || + strcmp(manifest->compat, compat) != 0) + { + continue; + } + + latest = pkg_metadata_find_latest(index, manifest->name); + if (latest != manifest) + { + continue; + } + + fprintf(stream, + "%s version=%s type=%s arch=%s compat=%s artifact=%s\n", + manifest->name, + manifest->version, + pkg_manifest_type_str(manifest->type), + manifest->arch, + manifest->compat, + manifest->artifact); + } + + pkg_free(index); + return EXIT_SUCCESS; +} diff --git a/system/nxpkg/pkg_store.c b/system/nxpkg/pkg_store.c index 0b23b054ca3..56966f4287a 100644 --- a/system/nxpkg/pkg_store.c +++ b/system/nxpkg/pkg_store.c @@ -24,6 +24,7 @@ * Included Files ****************************************************************************/ +#include #include #include #include @@ -228,6 +229,11 @@ int pkg_store_format_index_path(FAR char *buffer, size_t size) return pkg_store_format(buffer, size, "%s", PKG_REPO_INDEX, ""); } +int pkg_store_format_repo_source_path(FAR char *buffer, size_t size) +{ + return pkg_store_format(buffer, size, "%s", PKG_REPO_SOURCE, ""); +} + int pkg_store_format_installed_path(FAR char *buffer, size_t size) { return pkg_store_format(buffer, size, "%s", PKG_REPO_INSTALLED, ""); @@ -266,21 +272,52 @@ int pkg_store_format_previous_path(FAR char *buffer, size_t size, int pkg_store_format_txn_path(FAR char *buffer, size_t size, FAR const char *name) { - return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/.txn", name, ""); + return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/txn.tx", name, + ""); } int pkg_store_format_lock_path(FAR char *buffer, size_t size, FAR const char *name) { - return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/.lock", name, ""); + return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/lock.lk", name, + ""); } int pkg_store_format_download_path(FAR char *buffer, size_t size, FAR const char *name, FAR const char *version) { - return pkg_store_format(buffer, size, PKG_TMP_PKG_DIR "/%s-%s.npkg", name, - version); + int ret; + + UNUSED(name); + UNUSED(version); + + /* This used to be "PKG_TMP_PKG_DIR/name-version.pkg", which breaks on + * this SD card's short-name-only FAT mount as soon as name+version + * exceeds the 8.3 8-character base-name limit - e.g. "nxdoom-1" (8 + * chars) fits and installs fine, but "nxdoom-10" or "nxdoom-9.1" (9+ + * chars) fails the open(O_CREAT) in pkg_repo_fetch_url() with + * -EINVAL, surfacing as "acquire source failed: -22" for any + * multi-character version - independent of name/version length here, + * unlike pkg_store_make_tmp_path()'s already-FAT-safe scheme. The + * pid is small, bounded, and unique per concurrently running install + * (each `nxpkg install` is its own process with its own per-name + * lock), so it can't collide the way a single fixed name would if + * two different packages were being installed at once. + */ + + ret = snprintf(buffer, size, PKG_TMP_PKG_DIR "/dl%d.pkg", (int)getpid()); + if (ret < 0) + { + return ret; + } + + if ((size_t)ret >= size) + { + return -ENAMETOOLONG; + } + + return 0; } int pkg_store_format_payload_path(FAR char *buffer, size_t size, @@ -311,16 +348,18 @@ int pkg_store_format_manifest_path(FAR char *buffer, size_t size, FAR const char *name, FAR const char *version) { - return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/%s/manifest.json", + return pkg_store_format(buffer, size, PKG_STORE_DIR "/%s/%s/manifest.jsn", name, version); } int pkg_store_read_text(FAR const char *path, FAR char **buffer) { - FAR FILE *stream; FAR char *data; - long length; + struct stat st; + size_t length; size_t nread; + size_t total; + int fd; if (buffer == NULL) { @@ -329,70 +368,167 @@ int pkg_store_read_text(FAR const char *path, FAR char **buffer) *buffer = NULL; - stream = fopen(path, "rb"); - if (stream == NULL) + fd = open(path, O_RDONLY); + if (fd < 0) { return errno == ENOENT ? -ENOENT : -errno; } - if (fseek(stream, 0, SEEK_END) < 0) + if (fstat(fd, &st) < 0) { - fclose(stream); + close(fd); return -errno; } - length = ftell(stream); - if (length < 0) + if (!S_ISREG(st.st_mode)) { - fclose(stream); - return -errno; + close(fd); + return -EINVAL; } - if (fseek(stream, 0, SEEK_SET) < 0) + /* Reject anything unreasonably large before the size is trusted for an + * allocation: guards both against a malicious/oversized text file (this + * path is used for the network-fetched index.jsn) and against + * "length + 1" wrapping if st_size were ever attacker-influenced up to + * SIZE_MAX. + */ + + if (st.st_size < 0 || st.st_size > (off_t)PKG_TEXT_MAX_SIZE) { - fclose(stream); - return -errno; + close(fd); + return -EFBIG; } - data = malloc((size_t)length + 1); + length = (size_t)st.st_size; + data = pkg_malloc((size_t)length + 1); if (data == NULL) { - fclose(stream); + close(fd); return -ENOMEM; } - nread = fread(data, 1, (size_t)length, stream); - if (nread != (size_t)length) + total = 0; + while (total < length) { - int err = ferror(stream); + ssize_t ret; + + ret = read(fd, data + total, length - total); + if (ret < 0) + { + if (errno == EINTR) + { + continue; + } + + close(fd); + pkg_free(data); + return -errno; + } + + if (ret == 0) + { + break; + } - fclose(stream); - free(data); - return err ? -EIO : -EINVAL; + total += (size_t)ret; } - fclose(stream); + nread = total; + close(fd); + + if (nread != length) + { + pkg_free(data); + return -EINVAL; + } data[length] = '\0'; *buffer = data; return 0; } +#ifndef CONFIG_PSEUDOFS_FILE +/**************************************************************************** + * Name: pkg_store_make_tmp_path + * + * Description: + * Derive a staging path for an atomic write/copy to "path", under a + * short-name-compatible extension instead of appending ".tmp" (which + * would produce a second '.' in the final path component and break on + * FAT filesystems without long file name support). + * + ****************************************************************************/ + +static int pkg_store_make_tmp_path(FAR char *tmp, size_t size, + FAR const char *path) +{ + FAR char *dot; + FAR char *slash; + int ret; + + ret = snprintf(tmp, size, "%s", path); + if (ret < 0) + { + return ret; + } + + if ((size_t)ret >= size) + { + return -ENAMETOOLONG; + } + + slash = strrchr(tmp, '/'); + dot = strrchr(slash != NULL ? slash : tmp, '.'); + if (dot != NULL) + { + *dot = '\0'; + } + + if (strlcat(tmp, ".tm", size) >= size) + { + return -ENAMETOOLONG; + } + + return 0; +} +#endif + int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text) { - char tmp[PATH_MAX]; +#ifdef CONFIG_PSEUDOFS_FILE int fd; int ret; - ret = snprintf(tmp, sizeof(tmp), "%s.tmp", path); + fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0644); + if (fd < 0) + { + return -errno; + } + + ret = pkg_store_write_all(fd, text, strlen(text)); if (ret < 0) { + close(fd); + unlink(path); return ret; } - if ((size_t)ret >= sizeof(tmp)) + if (close(fd) < 0) { - return -ENAMETOOLONG; + unlink(path); + return -errno; + } + + return 0; +#else + char tmp[PATH_MAX]; + int fd; + int ret; + + ret = pkg_store_make_tmp_path(tmp, sizeof(tmp), path); + if (ret < 0) + { + return ret; } fd = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, 0644); @@ -409,6 +545,22 @@ int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text) return ret; } + /* Force the write through to disk before renaming. nxpkg writes + * several small, unrelated files back-to-back during install (per- + * package txn state, then the shared installed-packages database); + * without an explicit sync here, the FAT driver's single shared + * sector cache can still hold a not-yet-committed buffer for this + * file when the very next atomic write starts touching a different + * file, corrupting one or both. + */ + + if (fsync(fd) < 0) + { + close(fd); + unlink(tmp); + return -errno; + } + if (close(fd) < 0) { unlink(tmp); @@ -422,6 +574,7 @@ int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text) } return 0; +#endif } int pkg_store_copy_file(FAR const char *src, FAR const char *dest) @@ -430,6 +583,20 @@ int pkg_store_copy_file(FAR const char *src, FAR const char *dest) int outfd; int ret; char buffer[512]; +#ifndef CONFIG_PSEUDOFS_FILE + char tmp[PATH_MAX]; + FAR const char *outpath; + + ret = pkg_store_make_tmp_path(tmp, sizeof(tmp), dest); + if (ret < 0) + { + return ret; + } + + outpath = tmp; +#else + FAR const char *outpath = dest; +#endif infd = open(src, O_RDONLY); if (infd < 0) @@ -437,7 +604,7 @@ int pkg_store_copy_file(FAR const char *src, FAR const char *dest) return -errno; } - outfd = open(dest, O_WRONLY | O_CREAT | O_TRUNC, 0644); + outfd = open(outpath, O_WRONLY | O_CREAT | O_TRUNC, 0644); if (outfd < 0) { ret = -errno; @@ -475,18 +642,43 @@ int pkg_store_copy_file(FAR const char *src, FAR const char *dest) close(infd); +#ifndef CONFIG_PSEUDOFS_FILE + /* Force the payload through to disk before renaming - this is the + * largest write in the whole install pipeline (WAD/game-ELF-sized + * payloads), so a hard power-loss here is the scenario the atomic + * temp+rename is specifically protecting against. + */ + + if (fsync(outfd) < 0) + { + ret = -errno; + close(outfd); + unlink(outpath); + return ret; + } +#endif + if (close(outfd) < 0) { - unlink(dest); + unlink(outpath); return -errno; } +#ifndef CONFIG_PSEUDOFS_FILE + if (rename(outpath, dest) < 0) + { + ret = -errno; + unlink(outpath); + return ret; + } +#endif + return 0; errout: close(infd); close(outfd); - unlink(dest); + unlink(outpath); return ret; } @@ -499,3 +691,59 @@ int pkg_store_remove_file(FAR const char *path) return 0; } + +int pkg_store_remove_version_dir(FAR const char *name, + FAR const char *version) +{ + char path[PATH_MAX]; + char entry_path[PATH_MAX]; + FAR DIR *dir; + FAR struct dirent *ent; + int ret; + + /* Generic directory-content removal (rather than unlinking the payload + * and manifest.jsn by their known names) so this same helper works both + * to reclaim a partially staged version directory after a failed + * install (pkg_install.c) and to prune/remove a fully-installed + * version, without needing to already know that version's artifact + * filename. Best-effort throughout: this runs from error/cleanup + * paths where a still-failing removal shouldn't itself abort the + * caller. + */ + + ret = pkg_store_format_version_path(path, sizeof(path), name, version); + if (ret < 0) + { + return ret; + } + + dir = opendir(path); + if (dir == NULL) + { + return errno == ENOENT ? 0 : -errno; + } + + while ((ent = readdir(dir)) != NULL) + { + if (strcmp(ent->d_name, ".") == 0 || strcmp(ent->d_name, "..") == 0) + { + continue; + } + + ret = snprintf(entry_path, sizeof(entry_path), "%s/%s", path, + ent->d_name); + if (ret > 0 && (size_t)ret < sizeof(entry_path)) + { + unlink(entry_path); + } + } + + closedir(dir); + + if (rmdir(path) < 0) + { + return errno == ENOENT ? 0 : -errno; + } + + return 0; +} From 455af497caa728886edbe5a799118ef083104133 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Wed, 22 Jul 2026 17:59:47 +0530 Subject: [PATCH 2/5] system/nxpkg: address lifecycle review findings. Fix real correctness/security gaps found during review, on top of the network sync and CLI completion work: - pkg_install(): commit the installed database before writing the current/previous pointer files, and before advancing transaction state past ACTIVATED. Those are recovery bookkeeping and convenience mirrors respectively - once the installed database itself is durably committed, a failure to refresh either one must not trigger the failure-cleanup path, which could otherwise delete a payload the database now legitimately points at. Also stop treating an existing version directory as newly created when reinstalling an already-installed version, so a failed reinstall can't delete a working install. - pkg_uninstall()/pkg_rollback(): acquire the per-package install lock in addition to the installed-db lock, drop the entry from the authoritative database first, and only then remove payload files - a crash between those two steps can leave orphaned files, which are reclaimable, but never leaves the database pointing at a payload that's already gone. - pkg_sync(): stage each sync to a PID-qualified temp filename instead of a single fixed name, so concurrent sync calls can no longer race on the same staging file. Return real negative errno codes instead of EXIT_SUCCESS/EXIT_FAILURE, matching the rest of this API; pkg_main.c maps that back to a process exit code at the CLI boundary. - pkg_metadata_parse_installed_entry(): validate that name/current/ previous/every entry in versions[] are safe path components, and that current (and previous, if set) actually appear in versions[] - a corrupted or tampered installed-packages database can no longer reference a nonexistent version or smuggle a path-traversal sequence through a field this code already trusted implicitly. - pkg_store_write_all()/pkg_repo_sink(): treat a zero-byte write() as -EIO instead of looping on it silently. - Removed the malloc()-falls-back-to-kmm_malloc() logic in pkg_malloc/ pkg_zalloc/pkg_realloc/pkg_free entirely - it required tracking which allocator owned a given pointer via kmm_heapmember(), which is specific to this target's flat, single-heap memory model and not a sound general application API. These are now plain wrappers around malloc/calloc/realloc/free. - Added pkg_metadata_load_manifest_path(), so a caller (e.g. the nxstore GUI frontend, launching an installed package) can load the manifest actually recorded for a specific installed version, instead of combining a rollback-selected version with whatever the current catalog happens to describe for that package name - those can disagree after a rollback if the catalog has since moved on. - Storage root default changed from /tmp/nxpkg to /var/lib/nxpkg, following the conventional persistent application-data location instead of a path whose own name suggests non-persistent storage. A board without persistent storage mounted at /var, or that wants a different location (e.g. an SD card), still overrides this via CONFIG_SYSTEM_NXPKG_ROOT as before. - Moved system/nxpkg/README.txt into the companion NuttX documentation PR rather than shipping user-facing documentation as an in-tree README. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 --- system/nxpkg/Kconfig | 8 +- system/nxpkg/README.txt | 175 ------------------------------------ system/nxpkg/pkg.h | 72 ++------------- system/nxpkg/pkg_install.c | 151 ++++++++++++++++++++----------- system/nxpkg/pkg_main.c | 2 +- system/nxpkg/pkg_manifest.c | 10 +-- system/nxpkg/pkg_metadata.c | 58 ++++++++++++ system/nxpkg/pkg_repo.c | 37 +++++--- system/nxpkg/pkg_store.c | 5 ++ 9 files changed, 205 insertions(+), 313 deletions(-) delete mode 100644 system/nxpkg/README.txt diff --git a/system/nxpkg/Kconfig b/system/nxpkg/Kconfig index 196e7bdfd67..2f08ea6230c 100644 --- a/system/nxpkg/Kconfig +++ b/system/nxpkg/Kconfig @@ -31,11 +31,13 @@ config SYSTEM_NXPKG_STACKSIZE config SYSTEM_NXPKG_ROOT string "'nxpkg' storage root" - default "/tmp/nxpkg" + default "/var/lib/nxpkg" ---help--- Base directory used by nxpkg for its local index, installed metadata, temporary downloads, and package payload storage. - Boards with external storage can point this to a persistent - location such as /mnt/sdcard/nxpkg. + The default follows the conventional persistent application-data + location. A board must mount persistent storage at /var or set + this to another persistent location, such as /mnt/sdcard/nxpkg, + if packages need to survive a reset. endif diff --git a/system/nxpkg/README.txt b/system/nxpkg/README.txt deleted file mode 100644 index c3a48ee80d9..00000000000 --- a/system/nxpkg/README.txt +++ /dev/null @@ -1,175 +0,0 @@ -nxpkg - a package manager for NuttX application images -======================================================== - - nxpkg installs, updates, uninstalls, and rolls back standalone loadable - ELF applications (MODULE=m in an app's Makefile - see games/NXDoom or - examples/calculator for real ports) from a package repository served - over HTTP, onto local storage (an SD card on this board's config). - system/nxstore is an LVGL touchscreen front-end for nxpkg; this - document covers the repository/server side, which nxstore and the - nxpkg CLI both consume identically. - - -Repository layout -================== - - A repository is nothing more than a directory of static files: - - / - index.json - the package catalog - artifacts////// - icons////.bin - optional, see below - - index.json is a single JSON object with one "packages" array. Each - entry is one installable version of one package: - - { - "packages": [ - { - "name": "calc", - "version": "6", - "arch": "xtensa", - "compat": "esp32s3-touch-lcd-7", - "artifact": "artifacts/xtensa/esp32s3/esp32s3-touch-lcd-7/calc/6/calc", - "sha256": "<64 hex chars>", - "type": "elf", - "description": "Simple 4-function calculator", - "category": "Utilities", - "icon": "icons/xtensa/esp32s3/esp32s3-touch-lcd-7/calc.bin" - } - ] - } - - "artifact" and "icon" (both optional except "artifact") are resolved - relative to the repository's own base URL (the URL index.json itself - was fetched from, with the filename stripped) unless they're already - a full http(s):// URL - see pkg_resolve_artifact_source()/ - pkg_resolve_icon_source() in pkg_repo.c. That means the whole - directory tree above can be moved, mirrored, or served from a - completely different host without editing a single path in - index.json, as long as the *relative* structure between index.json - and artifacts/icons/ stays the same. - - "arch"/"compat" must match this board's CONFIG_ARCH ("xtensa") and - CONFIG_ARCH_BOARD ("esp32s3-touch-lcd-7") exactly - see - pkg_compat_check() in pkg_compat.c - so one repository can host - packages for several different boards side by side; each board only - ever installs the entries that match its own identity. - - -What actually serves this - any static file host works -======================================================== - - pkg_repo_fetch_url() (pkg_repo.c) does a plain HTTP GET with no auth, - no custom headers, and no server-side logic required - it just needs - a 2xx response. This means literally any static file server works: - nginx or Apache with a DocumentRoot pointed at the repo directory, a - one-line Python http.server, GitHub Pages, S3/R2/Cloudflare Pages, or - a NAS's built-in web server. There is nothing nxpkg-specific to - install on the server side. - - For local development/testing (what this project actually used while - building and testing every package in this series on real hardware): - - cd /path/to/repo-root - python3 -m http.server 8000 - - That's the entire "server setup." Confirm it's reachable from your - own machine first: - - curl -sI http://:8000/index.json - - sha256 verification (pkg_hash_file_sha256(), checked against the - manifest's "sha256" field before an artifact is ever activated) - already protects payload integrity in transit even over plain HTTP. - Only confidentiality/availability would need HTTPS on top of this if - that matters for a given deployment - nothing in the protocol assumes - HTTP specifically, an https:// URL works exactly the same way. - - -Populating a repository: tools/export_pkg_repo.py -================================================== - - Building the JSON by hand is error-prone (sha256 digests, exact - relative paths); tools/export_pkg_repo.py does it for you from a - built binary: - - python3 tools/export_pkg_repo.py \ - --arch xtensa --chip esp32s3 --compat esp32s3-touch-lcd-7 \ - --package "calc:6:elf:/path/to/apps/bin/calc:Simple 4-function calculator:Utilities" \ - /path/to/repo-root - - --package can be repeated to export several packages/versions in one - call. The colon-separated spec is: - - :::[::[:]] - - is the built artifact on your machine (e.g. apps/bin/calc - after a normal `make`). is optional: any image Pillow can open - (PNG, JPEG, ...) - the tool resizes it to a fixed 48x48 and encodes it - into the small raw RGB565 format system/nxstore's icon rendering - expects (see nxstore_load_icon() in nxstore_main.c for the exact - on-wire layout). Re-running the script is idempotent: it loads - whatever index.json already exists in the target directory, merges in - the packages you just passed (matched on name+version+arch+compat+ - type), and rewrites the file - existing unrelated entries are left - alone. - - This board's FAT-formatted SD card only supports short (8.3) file/ - directory names with no long-name extension - any path component - (package name, artifact leaf filename) over 8 characters before its - extension fails to install with errno 22, not a clear error message. - Keep names short (this is why this series' packages are named "calc", - "brick", "cgol" rather than "calculator", "brickmatch", "conway") - - this is a board/filesystem constraint, not an export_pkg_repo.py or - nxpkg one, and would not apply to a board with a different storage - filesystem. - - -Pointing the board at your repository -====================================== - - Two ways to get an index synced onto the board's local storage, - useful in different situations: - - 1. One-shot from NSH, useful for iterating on a package during - development (this is what building and testing every package in - this series actually looked like): - - nxpkg sync http://:8000/index.json - nxpkg install - nxpkg update # re-check for/install a newer version - nxpkg list # show what's installed, current vs previous - nxpkg rollback # swap back to the previous version - nxpkg remove - - 2. Automatically at boot, via system/nxstore: the repo URL nxstore - syncs from at startup is currently a single hardcoded string in - board_nxstore_autostart() (boards/xtensa/esp32s3/ - esp32s3-touch-lcd-7/src/esp32s3_bringup.c) - change that one line - to point at a different host, or a public one, and every boot - re-syncs the catalog automatically (falling back to whatever was - last synced to local storage if the network/server is unreachable - at boot - nxstore never blocks the app list on a failed sync). - - Development-network note: if your development machine's IP address - changes (switching Wi-Fi networks, a new DHCP lease, a hotspot), - re-run `nxpkg sync` with the new address - the board and the machine - serving the repository just need to be able to reach each other over - IP; nothing else about the setup changes. - - -Concurrency note -================= - - nxpkg protects a single package's own install/update/rollback against - running twice at once (a per-package lock file), and separately - protects the shared installed-packages database against being - corrupted by two *different* packages' installs racing each other - (see pkg_install_acquire_installed_lock() in pkg_install.c) - so it is - safe to have, for example, system/nxstore's own background install - worker and a directly-invoked `nxpkg install` running at the same - time for different packages. Two operations on the *same* package - name at the same time will have the second one fail with -EBUSY - (surfaced as "package has an install/update in progress") rather than - either one silently clobbering the other's work. diff --git a/system/nxpkg/pkg.h b/system/nxpkg/pkg.h index ff515962ab9..8f62e901169 100644 --- a/system/nxpkg/pkg.h +++ b/system/nxpkg/pkg.h @@ -35,8 +35,6 @@ #include #include -#include - /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ @@ -58,14 +56,9 @@ #define PKG_CATEGORY_MAX 31 #define PKG_HASH_HEX_LEN 64 /* Each manifest slot is ~1.7KB (dominated by PKG_LAUNCH_ARGS_MAX slots). - * This used to be 6 to fit inside a static internal-DRAM array, but the - * catalog outgrew that once it held every real GSoC package (calc, the - * four games, NXDoom, and the original examples) - system/nxstore/ - * nxstore_main.c now heap-allocates its struct pkg_index_s g_index via - * pkg_zalloc(), which draws from the PSRAM-backed user heap on this board - * (CONFIG_ESP32S3_SPIRAM_USER_HEAP) instead of internal DRAM, so this can - * grow without the same pressure. Still not unbounded: keep it sized for - * "a real catalog", not arbitrary attacker-controlled growth. + * Keep the catalog bounded so repository-provided metadata cannot cause + * unbounded memory use. Callers should allocate struct pkg_index_s from + * the application heap rather than placing it on a small task stack. */ #define PKG_INDEX_MAX 16 @@ -99,72 +92,22 @@ static inline void *pkg_malloc(size_t size) { - void *ptr = malloc(size); - - if (ptr == NULL) - { - ptr = kmm_malloc(size); - } - - return ptr; + return malloc(size); } static inline void *pkg_zalloc(size_t size) { - void *ptr = calloc(1, size); - - if (ptr == NULL) - { - ptr = kmm_zalloc(size); - } - - return ptr; + return calloc(1, size); } static inline void *pkg_realloc(void *ptr, size_t size) { - if (ptr == NULL) - { - return pkg_malloc(size); - } - - if (size == 0) - { - if (kmm_heapmember(ptr)) - { - kmm_free(ptr); - } - else - { - free(ptr); - } - - return NULL; - } - - if (kmm_heapmember(ptr)) - { - return kmm_realloc(ptr, size); - } - return realloc(ptr, size); } static inline void pkg_free(void *ptr) { - if (ptr == NULL) - { - return; - } - - if (kmm_heapmember(ptr)) - { - kmm_free(ptr); - } - else - { - free(ptr); - } + free(ptr); } static inline FAR char *pkg_path_alloc(void) @@ -241,6 +184,7 @@ struct pkg_installed_db_s const char *pkg_manifest_type_str(enum pkg_payload_type_e type); int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest); +bool pkg_validate_path_component(FAR const char *value); int pkg_manifest_parse_type(FAR const char *value, FAR enum pkg_payload_type_e *type); @@ -291,6 +235,8 @@ int pkg_hash_file_sha256(FAR const char *path, int pkg_metadata_load_index_path(FAR const char *path, FAR struct pkg_index_s *index); int pkg_metadata_load_index(FAR struct pkg_index_s *index); +int pkg_metadata_load_manifest_path(FAR const char *path, + FAR struct pkg_manifest_s *manifest); FAR const struct pkg_manifest_s * pkg_metadata_find_latest(FAR const struct pkg_index_s *index, FAR const char *name); diff --git a/system/nxpkg/pkg_install.c b/system/nxpkg/pkg_install.c index 3048342696e..9456a94671d 100644 --- a/system/nxpkg/pkg_install.c +++ b/system/nxpkg/pkg_install.c @@ -561,6 +561,29 @@ int pkg_install(FAR const char *name) goto errout; } + ret = pkg_store_format_version_path(payload, PATH_MAX, manifest->name, + manifest->version); + if (ret < 0) + { + pkg_error("version path format failed: %d", ret); + goto errout; + } + + if (access(payload, F_OK) == 0) + { + version_dir_created = false; + } + else if (errno == ENOENT) + { + version_dir_created = true; + } + else + { + ret = -errno; + pkg_error("unable to inspect version directory: %d", ret); + goto errout; + } + ret = pkg_store_ensure_version_dir(manifest->name, manifest->version); if (ret < 0) { @@ -568,8 +591,6 @@ int pkg_install(FAR const char *name) goto errout; } - version_dir_created = true; - ret = pkg_store_format_payload_path(payload, PATH_MAX, manifest->name, manifest->version, manifest->artifact); @@ -653,18 +674,22 @@ int pkg_install(FAR const char *name) goto errout; } - ret = pkg_install_write_pointers(installed, manifest->name); + ret = pkg_metadata_save_installed(installed); if (ret < 0) { - pkg_error("write current/previous pointers failed: %d", ret); + pkg_error("save installed metadata failed: %d", ret); goto errout; } - ret = pkg_metadata_save_installed(installed); + ret = pkg_install_write_pointers(installed, manifest->name); if (ret < 0) { - pkg_error("save installed metadata failed: %d", ret); - goto errout; + /* The installed database is authoritative. The pointer files are + * convenience mirrors and can be reconstructed from it, so failure + * to refresh one must not roll back a durably committed install. + */ + + pkg_error("unable to refresh current/previous pointers: %d", ret); } pkg_store_remove_file(installed_lock); @@ -673,8 +698,13 @@ int pkg_install(FAR const char *name) ret = pkg_txn_write_state(name, PKG_TXN_ACTIVATED); if (ret < 0) { + /* The installed database has already been committed. Do not enter + * the failure cleanup path here: it could remove a payload referenced + * by that database. Transaction state is recovery bookkeeping and + * can be cleared below. + */ + pkg_error("txn state activated failed: %d", ret); - goto errout; } pkg_txn_write_state(name, PKG_TXN_CLEANUP); @@ -811,15 +841,17 @@ int pkg_uninstall(FAR const char *name) { FAR struct pkg_installed_db_s *db; FAR struct pkg_installed_entry_s *entry; + struct pkg_installed_entry_s removed; char path[PATH_MAX]; + char package_lock[PATH_MAX]; char installed_lock[PATH_MAX]; size_t index; size_t i; int ret; - if (name == NULL || name[0] == '\0') + if (!pkg_validate_path_component(name)) { - pkg_error("remove requires a package name"); + pkg_error("remove requires a valid package name"); return EXIT_FAILURE; } @@ -838,10 +870,19 @@ int pkg_uninstall(FAR const char *name) return EXIT_FAILURE; } + ret = pkg_install_acquire_lock(name, package_lock, sizeof(package_lock)); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to acquire package lock for '%s': %d", name, ret); + return EXIT_FAILURE; + } + ret = pkg_install_acquire_installed_lock(installed_lock, sizeof(installed_lock)); if (ret < 0) { + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to acquire installed-db lock: %d", ret); return EXIT_FAILURE; @@ -851,6 +892,7 @@ int pkg_uninstall(FAR const char *name) if (ret < 0) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to load installed metadata: %d", ret); return EXIT_FAILURE; @@ -860,34 +902,18 @@ int pkg_uninstall(FAR const char *name) if (entry == NULL) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("package '%s' is not installed", name); return EXIT_FAILURE; } - if (pkg_store_format_lock_path(path, sizeof(path), name) == 0 && - access(path, F_OK) == 0) - { - pkg_store_remove_file(installed_lock); - pkg_free(db); - pkg_error("package '%s' has an install/update in progress", name); - return EXIT_FAILURE; - } - - for (i = 0; i < entry->version_count; i++) - { - pkg_store_remove_version_dir(name, entry->versions[i]); - } - - if (pkg_store_format_txn_path(path, sizeof(path), name) == 0) - { - pkg_store_remove_file(path); - } - - /* Drop this entry from the in-memory db (shift the tail down over it) - * and persist before touching any more of the on-disk layout. + /* Drop this entry from the authoritative database before removing its + * payloads. A power loss can then leave reclaimable orphan files, but + * never a database entry that points at a payload already deleted. */ + removed = *entry; index = (size_t)(entry - db->entries); for (i = index; i + 1 < db->count; i++) { @@ -900,11 +926,22 @@ int pkg_uninstall(FAR const char *name) pkg_store_remove_file(installed_lock); if (ret < 0) { + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to save installed metadata: %d", ret); return EXIT_FAILURE; } + for (i = 0; i < removed.version_count; i++) + { + pkg_store_remove_version_dir(name, removed.versions[i]); + } + + if (pkg_store_format_txn_path(path, sizeof(path), name) == 0) + { + pkg_store_remove_file(path); + } + if (pkg_store_format_current_path(path, sizeof(path), name) == 0) { pkg_store_remove_file(path); @@ -915,6 +952,8 @@ int pkg_uninstall(FAR const char *name) pkg_store_remove_file(path); } + pkg_store_remove_file(package_lock); + if (pkg_store_format_package_root(path, sizeof(path), name) == 0) { rmdir(path); @@ -942,15 +981,15 @@ int pkg_rollback(FAR const char *name) FAR struct pkg_installed_db_s *db; FAR struct pkg_installed_entry_s *entry; char version_path[PATH_MAX]; - char lock_path[PATH_MAX]; + char package_lock[PATH_MAX]; char installed_lock[PATH_MAX]; char swap[PKG_VERSION_MAX + 1]; struct stat st; int ret; - if (name == NULL || name[0] == '\0') + if (!pkg_validate_path_component(name)) { - pkg_error("rollback requires a package name"); + pkg_error("rollback requires a valid package name"); return EXIT_FAILURE; } @@ -969,10 +1008,19 @@ int pkg_rollback(FAR const char *name) return EXIT_FAILURE; } + ret = pkg_install_acquire_lock(name, package_lock, sizeof(package_lock)); + if (ret < 0) + { + pkg_free(db); + pkg_error("unable to acquire package lock for '%s': %d", name, ret); + return EXIT_FAILURE; + } + ret = pkg_install_acquire_installed_lock(installed_lock, sizeof(installed_lock)); if (ret < 0) { + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to acquire installed-db lock: %d", ret); return EXIT_FAILURE; @@ -982,6 +1030,7 @@ int pkg_rollback(FAR const char *name) if (ret < 0) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to load installed metadata: %d", ret); return EXIT_FAILURE; @@ -991,6 +1040,7 @@ int pkg_rollback(FAR const char *name) if (entry == NULL) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("package '%s' is not installed", name); return EXIT_FAILURE; @@ -999,26 +1049,19 @@ int pkg_rollback(FAR const char *name) if (entry->previous[0] == '\0') { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("package '%s' has no previous version to roll back to", name); return EXIT_FAILURE; } - if (pkg_store_format_lock_path(lock_path, sizeof(lock_path), name) == 0 && - access(lock_path, F_OK) == 0) - { - pkg_store_remove_file(installed_lock); - pkg_free(db); - pkg_error("package '%s' has an install/update in progress", name); - return EXIT_FAILURE; - } - ret = pkg_store_format_version_path(version_path, sizeof(version_path), name, entry->previous); if (ret < 0 || stat(version_path, &st) < 0) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("rollback target version '%s' is missing on disk", entry->previous); @@ -1029,6 +1072,7 @@ int pkg_rollback(FAR const char *name) if (ret < 0 || (size_t)ret >= sizeof(swap)) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("current version string too long to swap"); return EXIT_FAILURE; @@ -1039,6 +1083,7 @@ int pkg_rollback(FAR const char *name) if (ret < 0 || (size_t)ret >= sizeof(entry->current)) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to update current version"); return EXIT_FAILURE; @@ -1048,33 +1093,33 @@ int pkg_rollback(FAR const char *name) if (ret < 0 || (size_t)ret >= sizeof(entry->previous)) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); pkg_error("unable to update previous version"); return EXIT_FAILURE; } - /* Persist the pointer-file swap and the db entry together, in that - * order, before releasing anything - a crash between these two writes - * still leaves "current" resolving to the (now rolled-back-to) version - * that's actually on disk, which is the side that must win. + /* The installed database is authoritative, so commit it first. Refresh + * the current/previous pointer files afterwards as convenience mirrors; + * they can be reconstructed from the database if either write fails. */ - ret = pkg_install_write_pointers(db, name); + ret = pkg_metadata_save_installed(db); if (ret < 0) { pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); pkg_free(db); - pkg_error("unable to write current/previous pointers: %d", ret); + pkg_error("unable to save installed metadata: %d", ret); return EXIT_FAILURE; } - ret = pkg_metadata_save_installed(db); + ret = pkg_install_write_pointers(db, name); pkg_store_remove_file(installed_lock); + pkg_store_remove_file(package_lock); if (ret < 0) { - pkg_free(db); - pkg_error("unable to save installed metadata: %d", ret); - return EXIT_FAILURE; + pkg_error("unable to refresh current/previous pointers: %d", ret); } pkg_info("rolled back %s to version %s", name, entry->current); diff --git a/system/nxpkg/pkg_main.c b/system/nxpkg/pkg_main.c index f20137f9ec5..40dfb710746 100644 --- a/system/nxpkg/pkg_main.c +++ b/system/nxpkg/pkg_main.c @@ -162,7 +162,7 @@ int main(int argc, FAR char *argv[]) return EXIT_FAILURE; } - return pkg_sync(argv[2]); + return pkg_sync(argv[2]) == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } fprintf(stderr, "ERROR: Unknown subcommand '%s'\n", cmd); diff --git a/system/nxpkg/pkg_manifest.c b/system/nxpkg/pkg_manifest.c index 4c09803851e..dca17546c21 100644 --- a/system/nxpkg/pkg_manifest.c +++ b/system/nxpkg/pkg_manifest.c @@ -74,6 +74,10 @@ static bool pkg_validate_hex(FAR const char *value) return true; } +/**************************************************************************** + * Public Functions + ****************************************************************************/ + /**************************************************************************** * Name: pkg_validate_path_component * @@ -88,7 +92,7 @@ static bool pkg_validate_hex(FAR const char *value) * ****************************************************************************/ -static bool pkg_validate_path_component(FAR const char *value) +bool pkg_validate_path_component(FAR const char *value) { FAR const char *p; @@ -117,10 +121,6 @@ static bool pkg_validate_path_component(FAR const char *value) return true; } -/**************************************************************************** - * Public Functions - ****************************************************************************/ - const char *pkg_manifest_type_str(enum pkg_payload_type_e type) { switch (type) diff --git a/system/nxpkg/pkg_metadata.c b/system/nxpkg/pkg_metadata.c index 38907f59fd0..c28b217adb8 100644 --- a/system/nxpkg/pkg_metadata.c +++ b/system/nxpkg/pkg_metadata.c @@ -302,6 +302,9 @@ static int pkg_metadata_parse_installed_entry( { FAR cJSON *field; FAR const char *value; + bool current_found = false; + bool previous_found = false; + size_t i; int ret; memset(entry, 0, sizeof(*entry)); @@ -366,6 +369,31 @@ static int pkg_metadata_parse_installed_entry( return ret; } + if (!pkg_validate_path_component(entry->name) || + !pkg_validate_path_component(entry->current) || + (entry->previous[0] != '\0' && + !pkg_validate_path_component(entry->previous))) + { + return -EINVAL; + } + + for (i = 0; i < entry->version_count; i++) + { + if (!pkg_validate_path_component(entry->versions[i])) + { + return -EINVAL; + } + + current_found |= strcmp(entry->versions[i], entry->current) == 0; + previous_found |= strcmp(entry->versions[i], entry->previous) == 0; + } + + if (!current_found || + (entry->previous[0] != '\0' && !previous_found)) + { + return -EINVAL; + } + return 0; } @@ -640,6 +668,36 @@ int pkg_metadata_load_index(FAR struct pkg_index_s *index) return pkg_metadata_load_index_path(path, index); } +int pkg_metadata_load_manifest_path(FAR const char *path, + FAR struct pkg_manifest_s *manifest) +{ + FAR cJSON *root; + FAR char *text; + int ret; + + if (path == NULL || manifest == NULL) + { + return -EINVAL; + } + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + root = cJSON_Parse(text); + pkg_free(text); + if (root == NULL) + { + return -EINVAL; + } + + ret = pkg_metadata_parse_manifest(root, manifest); + cJSON_Delete(root); + return ret; +} + FAR const struct pkg_manifest_s * pkg_metadata_find_latest(FAR const struct pkg_index_s *index, FAR const char *name) diff --git a/system/nxpkg/pkg_repo.c b/system/nxpkg/pkg_repo.c index 6ea294ca86a..64d63403564 100644 --- a/system/nxpkg/pkg_repo.c +++ b/system/nxpkg/pkg_repo.c @@ -229,6 +229,11 @@ static int pkg_repo_sink(FAR char **buffer, int offset, int datend, return -errno; } + if (nwritten == 0) + { + return -EIO; + } + cursor += nwritten; remaining -= (size_t)nwritten; } @@ -404,7 +409,7 @@ int pkg_sync(FAR const char *source) if (source == NULL || source[0] == '\0') { pkg_error("sync requires a non-empty index source"); - return EXIT_FAILURE; + return -EINVAL; } index = pkg_zalloc(sizeof(*index)); @@ -419,7 +424,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to allocate index metadata buffer"); - return EXIT_FAILURE; + return -ENOMEM; } ret = pkg_store_prepare_layout(); @@ -430,10 +435,16 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return EXIT_FAILURE; + return ret; } - ret = snprintf(tmp, PATH_MAX, "%s/idxsync.jsn", PKG_TMP_DIR); + /* Each CLI invocation has its own PID. Use it in the staging name so a + * manual sync cannot truncate the file being validated by nxstore (or a + * second shell). Keep the leaf short for short-name-only FAT mounts. + */ + + ret = snprintf(tmp, PATH_MAX, "%s/s%u.jsn", PKG_TMP_DIR, + (unsigned int)getpid()); if (ret < 0 || (size_t)ret >= PATH_MAX) { pkg_error("temporary sync path is too long"); @@ -441,7 +452,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return EXIT_FAILURE; + return -ENAMETOOLONG; } ret = pkg_acquire_source(source, tmp); @@ -452,7 +463,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return EXIT_FAILURE; + return ret; } ret = pkg_metadata_load_index_path(tmp, index); @@ -464,7 +475,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return EXIT_FAILURE; + return ret; } ret = pkg_store_read_text(tmp, &text); @@ -476,7 +487,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return EXIT_FAILURE; + return ret; } ret = pkg_store_format_index_path(index_path, PATH_MAX); @@ -489,7 +500,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to resolve local index path: %d", ret); - return EXIT_FAILURE; + return ret; } ret = pkg_store_write_text_atomic(index_path, text); @@ -502,7 +513,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to write local index: %d", ret); - return EXIT_FAILURE; + return ret; } ret = pkg_store_format_repo_source_path(source_path, PATH_MAX); @@ -515,7 +526,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to resolve repository source path: %d", ret); - return EXIT_FAILURE; + return ret; } ret = pkg_store_write_text_atomic(source_path, source); @@ -528,7 +539,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to save repository source: %d", ret); - return EXIT_FAILURE; + return ret; } pkg_store_remove_file(tmp); @@ -538,7 +549,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_info("synced package index from %s", source); - return EXIT_SUCCESS; + return 0; } int pkg_available(FAR FILE *stream) diff --git a/system/nxpkg/pkg_store.c b/system/nxpkg/pkg_store.c index 56966f4287a..c5ad84bcb0f 100644 --- a/system/nxpkg/pkg_store.c +++ b/system/nxpkg/pkg_store.c @@ -146,6 +146,11 @@ static int pkg_store_write_all(int fd, FAR const char *buffer, size_t length) return -errno; } + if (ret == 0) + { + return -EIO; + } + offset += (size_t)ret; } From 8571175bb998a5d953ff9443d1e3061780a07fdf Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Thu, 23 Jul 2026 16:06:58 +0530 Subject: [PATCH 3/5] system/nxpkg: fix concurrency and versioning bugs found in review. - pkg_sync(): index.jsn and repo.url were updated as two independent atomic writes with nothing serializing the pair against a second, concurrent pkg_sync() call - each write stayed internally consistent, but the pair didn't, so one sync's index could end up on disk next to a different sync's source URL. Add a dedicated sync lock (pkg_repo_acquire_sync_lock(), mirroring the existing installed- db lock's blocking-retry-with-stale-reclaim pattern) around the whole read-fetch-write sequence. Also renew the lock's mtime as data actually arrives (pkg_repo_sink(), via a new renew_lock_path field threaded through pkg_acquire_source()) rather than only stamping it once at acquire time - a lock acquired once and then measured against a fixed 10-minute staleness window could otherwise be reclaimed mid-download on a large-enough file over a slow-enough link, even though the download was still genuinely in progress. pkg_reclaim_stale_lock() (renamed from pkg_install_reclaim_stale_lock, made public) is shared between both lock kinds rather than duplicated. - pkg_install_prune_oldest_version(): deleted the pruned version's on-disk payload directory before the updated installed database was even durably saved. If pkg_metadata_save_installed() subsequently failed, the payload was already gone but the last successfully-saved instpkg.jsn could still list that version as installed. The victim version is now handed back to the caller (threaded through pkg_install_add_version()/pkg_install_update_installed()) so pkg_install() can defer the actual directory removal until after the save succeeds. - pkg_metadata_version_token_cmp(): two version tokens with equal numeric prefixes (e.g. "1a" and "1b", both parsing as 1) compared as equal instead of falling back to a lexical comparison of what follows the number, contradicting this function's own documented behavior and silently treating genuinely different versions as the same one. Compare the non-numeric remainder lexically when the numeric prefixes match instead of falling through. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: aviralgarg05 --- system/nxpkg/pkg.h | 4 +- system/nxpkg/pkg_install.c | 124 +++++++++++++++++++----------- system/nxpkg/pkg_metadata.c | 39 +++++++--- system/nxpkg/pkg_repo.c | 149 ++++++++++++++++++++++++++++++++---- 4 files changed, 246 insertions(+), 70 deletions(-) diff --git a/system/nxpkg/pkg.h b/system/nxpkg/pkg.h index 8f62e901169..28c8c19a805 100644 --- a/system/nxpkg/pkg.h +++ b/system/nxpkg/pkg.h @@ -259,7 +259,9 @@ int pkg_resolve_artifact_source(FAR char *buffer, size_t size, FAR const struct pkg_manifest_s *manifest); int pkg_resolve_icon_source(FAR char *buffer, size_t size, FAR const struct pkg_manifest_s *manifest); -int pkg_acquire_source(FAR const char *source, FAR const char *dest); +int pkg_acquire_source(FAR const char *source, FAR const char *dest, + FAR const char *renew_lock_path); +void pkg_reclaim_stale_lock(FAR const char *path); int pkg_sync(FAR const char *source); int pkg_install(FAR const char *name); int pkg_uninstall(FAR const char *name); diff --git a/system/nxpkg/pkg_install.c b/system/nxpkg/pkg_install.c index 9456a94671d..856b5e19f67 100644 --- a/system/nxpkg/pkg_install.c +++ b/system/nxpkg/pkg_install.c @@ -40,39 +40,6 @@ * Private Functions ****************************************************************************/ -/**************************************************************************** - * Name: pkg_install_reclaim_stale_lock - * - * Description: - * Remove "path" if it is old enough that it cannot belong to a - * still-running install (see PKG_LOCK_STALE_SECONDS). Best-effort: any - * stat()/unlink() failure just falls through to the normal -EBUSY - * result, since a lock we can't inspect should be treated as held. - * - ****************************************************************************/ - -static void pkg_install_reclaim_stale_lock(FAR const char *path) -{ - struct stat st; - time_t now; - - if (stat(path, &st) < 0) - { - return; - } - - now = time(NULL); - if (now < st.st_mtime || - (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) - { - return; - } - - pkg_error("reclaiming stale lock '%s' (age %ld s)", - path, (long)(now - st.st_mtime)); - unlink(path); -} - static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, size_t size) { @@ -94,7 +61,7 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); if (fd < 0 && errno == EEXIST) { - pkg_install_reclaim_stale_lock(path); + pkg_reclaim_stale_lock(path); fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); } @@ -176,7 +143,7 @@ static int pkg_install_acquire_installed_lock(FAR char *path, size_t size) return -errno; } - pkg_install_reclaim_stale_lock(path); + pkg_reclaim_stale_lock(path); usleep(20 * 1000); } @@ -201,7 +168,8 @@ static bool pkg_install_has_version( } static int pkg_install_prune_oldest_version( - FAR struct pkg_installed_entry_s *entry) + FAR struct pkg_installed_entry_s *entry, + FAR char *pruned_version, size_t pruned_version_size) { size_t victim = entry->version_count; size_t i; @@ -228,7 +196,19 @@ static int pkg_install_prune_oldest_version( return -E2BIG; } - pkg_store_remove_version_dir(entry->name, entry->versions[victim]); + /* Deleting the pruned version's on-disk directory here, before this + * in-memory db update is even durably saved, left a real inconsistency + * window: if pkg_metadata_save_installed() subsequently failed (full + * SD card, I/O error), the payload was already gone but the last + * successfully-saved instpkg.jsn could still list that version as + * installed. Hand the victim's version string back to the caller + * instead, so it can defer the actual directory removal until after + * the save succeeds - mirroring how this file already treats the + * installed db as authoritative everywhere else. + */ + + snprintf(pruned_version, pruned_version_size, "%s", + entry->versions[victim]); for (i = victim; i + 1 < entry->version_count; i++) { @@ -241,7 +221,9 @@ static int pkg_install_prune_oldest_version( } static int pkg_install_add_version(FAR struct pkg_installed_entry_s *entry, - FAR const char *version) + FAR const char *version, + FAR char *pruned_version, + size_t pruned_version_size) { int ret; @@ -252,7 +234,8 @@ static int pkg_install_add_version(FAR struct pkg_installed_entry_s *entry, if (entry->version_count >= PKG_INSTALLED_VERSIONS_MAX) { - ret = pkg_install_prune_oldest_version(entry); + ret = pkg_install_prune_oldest_version(entry, pruned_version, + pruned_version_size); if (ret < 0) { return ret; @@ -278,7 +261,9 @@ static int pkg_install_add_version(FAR struct pkg_installed_entry_s *entry, static int pkg_install_update_installed(FAR struct pkg_installed_db_s *db, FAR const struct pkg_manifest_s - *manifest) + *manifest, + FAR char *pruned_version, + size_t pruned_version_size) { FAR struct pkg_installed_entry_s *entry; int ret; @@ -332,7 +317,8 @@ static int pkg_install_update_installed(FAR struct pkg_installed_db_s *db, } entry->type = manifest->type; - return pkg_install_add_version(entry, manifest->version); + return pkg_install_add_version(entry, manifest->version, pruned_version, + pruned_version_size); } static int pkg_install_write_pointers( @@ -380,6 +366,42 @@ static int pkg_install_write_pointers( * Public Functions ****************************************************************************/ +/**************************************************************************** + * Name: pkg_reclaim_stale_lock + * + * Description: + * Remove "path" if it is old enough that it cannot belong to a still- + * running holder (see PKG_LOCK_STALE_SECONDS). Best-effort: any + * stat()/unlink() failure just falls through to the normal -EBUSY + * result, since a lock we can't inspect should be treated as held. + * Generic across every lock file this package uses (per-package + * install locks, the shared installed-db lock, pkg_repo.c's sync + * lock) - not install-specific despite living in this file. + * + ****************************************************************************/ + +void pkg_reclaim_stale_lock(FAR const char *path) +{ + struct stat st; + time_t now; + + if (stat(path, &st) < 0) + { + return; + } + + now = time(NULL); + if (now < st.st_mtime || + (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) + { + return; + } + + pkg_error("reclaiming stale lock '%s' (age %ld s)", + path, (long)(now - st.st_mtime)); + unlink(path); +} + int pkg_install(FAR const char *name) { FAR struct pkg_index_s *index; @@ -393,11 +415,14 @@ int pkg_install(FAR const char *name) FAR char *installed_lock; FAR const char *artifact; char digest[PKG_HASH_HEX_LEN + 1]; + char pruned_version[PKG_VERSION_MAX + 1]; bool staged_to_tmp; bool version_dir_created; bool installed_lock_held; int ret; + pruned_version[0] = '\0'; + index = pkg_zalloc(sizeof(*index)); installed = pkg_zalloc(sizeof(*installed)); source = pkg_path_alloc(); @@ -525,7 +550,7 @@ int pkg_install(FAR const char *name) goto errout; } - ret = pkg_acquire_source(source, tmp); + ret = pkg_acquire_source(source, tmp, lock); if (ret < 0) { pkg_error("acquire source failed: %d", ret); @@ -667,7 +692,8 @@ int pkg_install(FAR const char *name) goto errout; } - ret = pkg_install_update_installed(installed, manifest); + ret = pkg_install_update_installed(installed, manifest, pruned_version, + sizeof(pruned_version)); if (ret < 0) { pkg_error("update installed metadata failed: %d", ret); @@ -681,6 +707,18 @@ int pkg_install(FAR const char *name) goto errout; } + /* Only remove the pruned version's payload directory now that the db + * update naming it gone is durably saved - see + * pkg_install_prune_oldest_version()'s comment for why doing this + * before the save could leave a saved db entry pointing at an + * already-deleted version if the save had failed instead. + */ + + if (pruned_version[0] != '\0') + { + pkg_store_remove_version_dir(manifest->name, pruned_version); + } + ret = pkg_install_write_pointers(installed, manifest->name); if (ret < 0) { diff --git a/system/nxpkg/pkg_metadata.c b/system/nxpkg/pkg_metadata.c index c28b217adb8..62658f798dc 100644 --- a/system/nxpkg/pkg_metadata.c +++ b/system/nxpkg/pkg_metadata.c @@ -404,6 +404,9 @@ static int pkg_metadata_version_token_cmp(FAR const char *lhs, long rightnum; FAR char *leftend; FAR char *rightend; + FAR const char *cmpleft; + FAR const char *cmpright; + int ret; leftnum = strtol(lhs, &leftend, 10); rightnum = strtol(rhs, &rightend, 10); @@ -419,22 +422,36 @@ static int pkg_metadata_version_token_cmp(FAR const char *lhs, { return 1; } + + /* Equal numeric prefixes don't make the tokens equal if what + * follows the number differs - e.g. "1a" and "1b" both parse as + * 1, but are documented to compare lexically by their non-numeric + * component. Falling through to "equal" here (as this used to) + * silently treated any such pair as the same version, which is + * exactly backwards for tokens whose whole point is to be + * distinguishable. Compare what's left after the numeric prefix + * - leftend/rightend - lexically instead of falling through. + */ + + cmpleft = leftend; + cmpright = rightend; } else { - int ret; + cmpleft = lhs; + cmpright = rhs; + } - ret = pkg_string_cmp(lhs, PKG_VERSION_MAX + 1, - rhs, PKG_VERSION_MAX + 1); - if (ret < 0) - { - return -1; - } + ret = pkg_string_cmp(cmpleft, PKG_VERSION_MAX + 1, + cmpright, PKG_VERSION_MAX + 1); + if (ret < 0) + { + return -1; + } - if (ret > 0) - { - return 1; - } + if (ret > 0) + { + return 1; } return 0; diff --git a/system/nxpkg/pkg_repo.c b/system/nxpkg/pkg_repo.c index 64d63403564..c41894f1097 100644 --- a/system/nxpkg/pkg_repo.c +++ b/system/nxpkg/pkg_repo.c @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include @@ -67,6 +69,15 @@ struct pkg_fetch_context_s { int fd; size_t total; + + /* Optional path to a lock file whose mtime should be refreshed as data + * arrives - see pkg_repo_sink()'s comment on why a lock acquired once + * up front isn't enough for a download that can run past the stale- + * lock timeout on its own. NULL if there's nothing to renew (e.g. a + * plain local-file copy, which is fast enough not to need it). + */ + + FAR const char *renew_lock_path; }; #endif @@ -238,10 +249,28 @@ static int pkg_repo_sink(FAR char **buffer, int offset, int datend, remaining -= (size_t)nwritten; } + /* The lock this download is running under was only ever stamped once, + * at acquire time - PKG_LOCK_STALE_SECONDS then measures from that + * single timestamp regardless of how long the download actually + * takes, so a large-enough file over a slow-enough link can still be + * genuinely mid-transfer when another install for the same package + * decides the lock looks stale and reclaims it out from under this + * one. Touching it here means its age reflects time since the last + * byte actually arrived instead of total operation time - best- + * effort: a failed touch just means this one chunk didn't renew it, + * not that the download itself should fail. + */ + + if (ctx->renew_lock_path != NULL) + { + utime(ctx->renew_lock_path, NULL); + } + return 0; } -static int pkg_repo_fetch_url(FAR const char *url, FAR const char *dest) +static int pkg_repo_fetch_url(FAR const char *url, FAR const char *dest, + FAR const char *renew_lock_path) { struct pkg_fetch_context_s fetch; struct webclient_context client; @@ -256,6 +285,7 @@ static int pkg_repo_fetch_url(FAR const char *url, FAR const char *dest) } fetch.total = 0; + fetch.renew_lock_path = renew_lock_path; webclient_set_defaults(&client); client.method = "GET"; @@ -378,7 +408,8 @@ int pkg_resolve_icon_source(FAR char *buffer, size_t size, return pkg_resolve_relative_source(buffer, size, manifest->icon); } -int pkg_acquire_source(FAR const char *source, FAR const char *dest) +int pkg_acquire_source(FAR const char *source, FAR const char *dest, + FAR const char *renew_lock_path) { if (source == NULL || dest == NULL) { @@ -388,7 +419,7 @@ int pkg_acquire_source(FAR const char *source, FAR const char *dest) if (pkg_source_is_url(source)) { #ifdef CONFIG_NETUTILS_WEBCLIENT - return pkg_repo_fetch_url(source, dest); + return pkg_repo_fetch_url(source, dest, renew_lock_path); #else return -ENOSYS; #endif @@ -397,6 +428,64 @@ int pkg_acquire_source(FAR const char *source, FAR const char *dest) return pkg_store_copy_file(source, dest); } +/**************************************************************************** + * Name: pkg_repo_acquire_sync_lock + * + * Description: + * pkg_sync() below updates index.jsn and repo.url as two separate + * atomic writes with nothing serializing the pair against a second, + * concurrent pkg_sync() call (e.g. two different sources, or a manual + * sync racing nxstore's own retry loop). Each individual write stays + * internally consistent, but the *pair* doesn't: one sync's index.jsn + * can end up on disk next to a different sync's repo.url, and the two + * syncs' temp/staging files (already PID-qualified against each + * other) can still be committed out of order relative to one another. + * A single lock file around the whole read-fetch-write sequence + * closes that, mirroring pkg_install.c's installed-db lock: blocking + * with a bounded retry rather than instant -EBUSY, since this + * critical section already includes the network fetch and a slow + * sync should make a second one wait rather than fail outright. + * + ****************************************************************************/ + +static int pkg_repo_acquire_sync_lock(FAR char *path, size_t size) +{ + int fd; + int ret; + int tries; + + ret = snprintf(path, size, PKG_ROOT_DIR "/sync.lk"); + if (ret < 0) + { + return ret; + } + + if ((size_t)ret >= size) + { + return -ENAMETOOLONG; + } + + for (tries = 0; tries < 100; tries++) + { + fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + if (fd >= 0) + { + close(fd); + return 0; + } + + if (errno != EEXIST) + { + return -errno; + } + + pkg_reclaim_stale_lock(path); + usleep(20 * 1000); + } + + return -EBUSY; +} + int pkg_sync(FAR const char *source) { FAR struct pkg_index_s *index; @@ -404,6 +493,8 @@ int pkg_sync(FAR const char *source) FAR char *tmp; FAR char *index_path; FAR char *source_path; + FAR char *lock; + bool lock_held = false; int ret; if (source == NULL || source[0] == '\0') @@ -412,6 +503,23 @@ int pkg_sync(FAR const char *source) return -EINVAL; } + lock = pkg_path_alloc(); + if (lock == NULL) + { + pkg_error("unable to allocate sync lock path buffer"); + return -ENOMEM; + } + + ret = pkg_repo_acquire_sync_lock(lock, PATH_MAX); + if (ret < 0) + { + pkg_error("unable to acquire sync lock: %d", ret); + pkg_free(lock); + return ret; + } + + lock_held = true; + index = pkg_zalloc(sizeof(*index)); tmp = pkg_path_alloc(); index_path = pkg_path_alloc(); @@ -424,7 +532,8 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to allocate index metadata buffer"); - return -ENOMEM; + ret = -ENOMEM; + goto out; } ret = pkg_store_prepare_layout(); @@ -435,7 +544,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return ret; + goto out; } /* Each CLI invocation has its own PID. Use it in the staging name so a @@ -452,10 +561,11 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return -ENAMETOOLONG; + ret = -ENAMETOOLONG; + goto out; } - ret = pkg_acquire_source(source, tmp); + ret = pkg_acquire_source(source, tmp, lock); if (ret < 0) { pkg_error("unable to fetch index source '%s': %d", source, ret); @@ -463,7 +573,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return ret; + goto out; } ret = pkg_metadata_load_index_path(tmp, index); @@ -475,7 +585,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return ret; + goto out; } ret = pkg_store_read_text(tmp, &text); @@ -487,7 +597,7 @@ int pkg_sync(FAR const char *source) pkg_free(tmp); pkg_free(index_path); pkg_free(source_path); - return ret; + goto out; } ret = pkg_store_format_index_path(index_path, PATH_MAX); @@ -500,7 +610,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to resolve local index path: %d", ret); - return ret; + goto out; } ret = pkg_store_write_text_atomic(index_path, text); @@ -513,7 +623,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to write local index: %d", ret); - return ret; + goto out; } ret = pkg_store_format_repo_source_path(source_path, PATH_MAX); @@ -526,7 +636,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to resolve repository source path: %d", ret); - return ret; + goto out; } ret = pkg_store_write_text_atomic(source_path, source); @@ -539,7 +649,7 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_error("unable to save repository source: %d", ret); - return ret; + goto out; } pkg_store_remove_file(tmp); @@ -549,7 +659,16 @@ int pkg_sync(FAR const char *source) pkg_free(index_path); pkg_free(source_path); pkg_info("synced package index from %s", source); - return 0; + ret = 0; + +out: + if (lock_held) + { + unlink(lock); + } + + pkg_free(lock); + return ret; } int pkg_available(FAR FILE *stream) From 38c608326a5c71f48669fd25f05acb7af5a9cff5 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Thu, 23 Jul 2026 20:43:22 +0530 Subject: [PATCH 4/5] system/nxpkg: Address follow-up review findings. Shorten the installed-database lock description and centralize pkg_sync() cleanup, as requested in review. Replace repeated protocol literals with named constants and preserve errno before cleanup calls can overwrite it. Store the synchronized catalog source in the catalog itself so the catalog and its relative-artifact base are committed atomically. Continue reading the former sidecar format for upgrade compatibility, and normalize array-form catalogs before adding the private source field. Compare numeric version prefixes without strtol() overflow and retain lexical comparison of suffixes. Assisted-by: Codex:gpt-5 Signed-off-by: aviralgarg05 --- system/nxpkg/pkg_install.c | 33 +---- system/nxpkg/pkg_metadata.c | 63 +++++++--- system/nxpkg/pkg_repo.c | 244 ++++++++++++++++++++---------------- system/nxpkg/pkg_store.c | 30 +++-- 4 files changed, 204 insertions(+), 166 deletions(-) diff --git a/system/nxpkg/pkg_install.c b/system/nxpkg/pkg_install.c index 856b5e19f67..cef908c5106 100644 --- a/system/nxpkg/pkg_install.c +++ b/system/nxpkg/pkg_install.c @@ -78,37 +78,8 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, * Name: pkg_install_acquire_installed_lock * * Description: - * The per-package lock above (pkg_install_acquire_lock()) only ever - * protects one package's own version directory/manifest - it says - * nothing about the single shared installed-packages database - * (instpkg.jsn) that every install/uninstall/rollback reads, modifies - * in memory, and writes back as a whole. Two of those operations for - * *different* packages (their own per-package locks don't conflict) - * can each load a snapshot of that shared file, add/change their own - * entry, and save - and whichever one saves last wins, silently - * discarding whatever the other one had just added. This is exactly - * what "a previously-installed package vanishes from `nxpkg list` - * with no error" looks like, without needing any corrupted file or - * non-atomic write at all: the write path is already atomic - * (pkg_store_write_text_atomic() writes to a temp file, fsyncs, then - * renames), so the *file* is always internally consistent - it just - * might be a consistent snapshot that's missing an entry a concurrent - * operation already believed it had durably saved. - * - * A separate, single global lock file (independent of any specific - * package's own lock) closes that window: acquire it right before - * pkg_metadata_load_installed() and hold it until immediately after - * pkg_metadata_save_installed(), so that whole read-modify-write - * sequence is atomic with respect to every other caller of this - * function, regardless of which package(s) they're touching. - * - * Blocking (bounded retry loop) rather than instant-EBUSY like the - * per-package lock: the critical section this protects is a handful - * of local SD-card JSON operations with no network I/O in it, so a - * real holder releases it within milliseconds - a caller should wait - * that out rather than fail an otherwise-healthy install/uninstall/ - * rollback just because another one's shared-db update was in - * flight at the same instant. + * Acquire the global installed-database lock. This serializes the + * read-modify-write sequence used by install, uninstall, and rollback. * ****************************************************************************/ diff --git a/system/nxpkg/pkg_metadata.c b/system/nxpkg/pkg_metadata.c index 62658f798dc..951b4772bf8 100644 --- a/system/nxpkg/pkg_metadata.c +++ b/system/nxpkg/pkg_metadata.c @@ -24,6 +24,7 @@ * Included Files ****************************************************************************/ +#include #include #include #include @@ -400,41 +401,63 @@ static int pkg_metadata_parse_installed_entry( static int pkg_metadata_version_token_cmp(FAR const char *lhs, FAR const char *rhs) { - long leftnum; - long rightnum; - FAR char *leftend; - FAR char *rightend; FAR const char *cmpleft; FAR const char *cmpright; + FAR const char *leftdigits; + FAR const char *rightdigits; + size_t leftlen; + size_t rightlen; int ret; - leftnum = strtol(lhs, &leftend, 10); - rightnum = strtol(rhs, &rightend, 10); + leftdigits = lhs; + rightdigits = rhs; + while (isdigit((unsigned char)*leftdigits)) + { + leftdigits++; + } + + while (isdigit((unsigned char)*rightdigits)) + { + rightdigits++; + } - if (leftend != lhs && rightend != rhs) + if (leftdigits != lhs && rightdigits != rhs) { - if (leftnum < rightnum) + while (*lhs == '0' && lhs + 1 < leftdigits) + { + lhs++; + } + + while (*rhs == '0' && rhs + 1 < rightdigits) + { + rhs++; + } + + leftlen = (size_t)(leftdigits - lhs); + rightlen = (size_t)(rightdigits - rhs); + if (leftlen < rightlen) { return -1; } - if (leftnum > rightnum) + if (leftlen > rightlen) { return 1; } - /* Equal numeric prefixes don't make the tokens equal if what - * follows the number differs - e.g. "1a" and "1b" both parse as - * 1, but are documented to compare lexically by their non-numeric - * component. Falling through to "equal" here (as this used to) - * silently treated any such pair as the same version, which is - * exactly backwards for tokens whose whole point is to be - * distinguishable. Compare what's left after the numeric prefix - * - leftend/rightend - lexically instead of falling through. - */ + ret = memcmp(lhs, rhs, leftlen); + if (ret < 0) + { + return -1; + } + + if (ret > 0) + { + return 1; + } - cmpleft = leftend; - cmpright = rightend; + cmpleft = leftdigits; + cmpright = rightdigits; } else { diff --git a/system/nxpkg/pkg_repo.c b/system/nxpkg/pkg_repo.c index c41894f1097..3cd201e8e02 100644 --- a/system/nxpkg/pkg_repo.c +++ b/system/nxpkg/pkg_repo.c @@ -37,6 +37,8 @@ #include +#include + #ifdef CONFIG_NETUTILS_WEBCLIENT # include "netutils/webclient.h" #endif @@ -59,6 +61,9 @@ */ #define PKG_REPO_FETCH_BUFFER_SIZE 4096 +#define PKG_REPO_HTTP "http://" +#define PKG_REPO_HTTPS "https://" +#define PKG_REPO_SOURCE_KEY "_nxpkg_source" /**************************************************************************** * Private Types @@ -108,7 +113,7 @@ static int pkg_repo_source_base(FAR char *buffer, size_t size, slash = strrchr(source, '/'); if (slash == NULL) { - return -EINVAL; + return pkg_repo_copy_string(buffer, size, "."); } length = (size_t)(slash - source); @@ -168,12 +173,14 @@ static bool pkg_validate_artifact_relative(FAR const char *value) static int pkg_repo_read_source(FAR char *buffer, size_t size) { + FAR cJSON *root; + FAR cJSON *source; + FAR char *text = NULL; char path[PATH_MAX]; - FAR char *text; size_t length; int ret; - ret = pkg_store_format_repo_source_path(path, sizeof(path)); + ret = pkg_store_format_index_path(path, sizeof(path)); if (ret < 0) { return ret; @@ -185,17 +192,113 @@ static int pkg_repo_read_source(FAR char *buffer, size_t size) return ret; } - length = strlen(text); - while (length > 0 && isspace((unsigned char)text[length - 1])) + root = cJSON_Parse(text); + pkg_free(text); + if (root == NULL) { - text[--length] = '\0'; + return -EINVAL; } - ret = pkg_repo_copy_string(buffer, size, text); - pkg_free(text); + source = cJSON_GetObjectItemCaseSensitive(root, PKG_REPO_SOURCE_KEY); + if (!cJSON_IsString(source) || source->valuestring == NULL) + { + cJSON_Delete(root); + + /* Read the sidecar written by older nxpkg versions once, so an + * existing cached catalog remains usable until the next sync. + */ + + ret = pkg_store_format_repo_source_path(path, sizeof(path)); + if (ret < 0) + { + return ret; + } + + ret = pkg_store_read_text(path, &text); + if (ret < 0) + { + return ret; + } + + length = strlen(text); + while (length > 0 && isspace((unsigned char)text[length - 1])) + { + text[--length] = '\0'; + } + + ret = pkg_repo_copy_string(buffer, size, text); + pkg_free(text); + return ret; + } + + ret = pkg_repo_copy_string(buffer, size, source->valuestring); + cJSON_Delete(root); return ret; } +static int pkg_repo_attach_source(FAR char **text, + FAR const char *source_value) +{ + FAR cJSON *root; + FAR cJSON *wrapper; + FAR char *updated; + + root = cJSON_Parse(*text); + if (root == NULL) + { + return -EINVAL; + } + + if (cJSON_IsArray(root)) + { + wrapper = cJSON_CreateObject(); + if (wrapper == NULL) + { + cJSON_Delete(root); + return -ENOMEM; + } + + cJSON_AddItemToObject(wrapper, "packages", root); + if (cJSON_GetObjectItemCaseSensitive(wrapper, "packages") != root) + { + cJSON_Delete(root); + cJSON_Delete(wrapper); + return -ENOMEM; + } + + root = wrapper; + } + else if (!cJSON_IsObject(root)) + { + cJSON_Delete(root); + return -EINVAL; + } + + while (cJSON_GetObjectItemCaseSensitive(root, + PKG_REPO_SOURCE_KEY) != NULL) + { + cJSON_DeleteItemFromObjectCaseSensitive(root, PKG_REPO_SOURCE_KEY); + } + + if (cJSON_AddStringToObject(root, PKG_REPO_SOURCE_KEY, + source_value) == NULL) + { + cJSON_Delete(root); + return -ENOMEM; + } + + updated = cJSON_PrintUnformatted(root); + cJSON_Delete(root); + if (updated == NULL) + { + return -ENOMEM; + } + + pkg_free(*text); + *text = updated; + return 0; +} + #ifdef CONFIG_NETUTILS_WEBCLIENT static int pkg_repo_sink(FAR char **buffer, int offset, int datend, FAR int *buflen, FAR void *arg) @@ -312,10 +415,12 @@ static int pkg_repo_fetch_url(FAR const char *url, FAR const char *dest, return -EPROTO; } - if (close(fetch.fd) < 0) + ret = close(fetch.fd); + if (ret < 0) { + ret = -errno; unlink(dest); - return -errno; + return ret; } return 0; @@ -382,8 +487,8 @@ bool pkg_source_is_url(FAR const char *source) return false; } - return strncasecmp(source, "http://", 7) == 0 || - strncasecmp(source, "https://", 8) == 0; + return strncasecmp(source, PKG_REPO_HTTP, strlen(PKG_REPO_HTTP)) == 0 || + strncasecmp(source, PKG_REPO_HTTPS, strlen(PKG_REPO_HTTPS)) == 0; } int pkg_resolve_artifact_source(FAR char *buffer, size_t size, @@ -432,19 +537,8 @@ int pkg_acquire_source(FAR const char *source, FAR const char *dest, * Name: pkg_repo_acquire_sync_lock * * Description: - * pkg_sync() below updates index.jsn and repo.url as two separate - * atomic writes with nothing serializing the pair against a second, - * concurrent pkg_sync() call (e.g. two different sources, or a manual - * sync racing nxstore's own retry loop). Each individual write stays - * internally consistent, but the *pair* doesn't: one sync's index.jsn - * can end up on disk next to a different sync's repo.url, and the two - * syncs' temp/staging files (already PID-qualified against each - * other) can still be committed out of order relative to one another. - * A single lock file around the whole read-fetch-write sequence - * closes that, mirroring pkg_install.c's installed-db lock: blocking - * with a bounded retry rather than instant -EBUSY, since this - * critical section already includes the network fetch and a slow - * sync should make a second one wait rather than fail outright. + * Serialize catalog synchronization so concurrent downloads cannot + * commit out of order. * ****************************************************************************/ @@ -488,13 +582,12 @@ static int pkg_repo_acquire_sync_lock(FAR char *path, size_t size) int pkg_sync(FAR const char *source) { - FAR struct pkg_index_s *index; + FAR struct pkg_index_s *index = NULL; FAR char *text = NULL; - FAR char *tmp; - FAR char *index_path; - FAR char *source_path; + FAR char *tmp = NULL; + FAR char *index_path = NULL; FAR char *lock; - bool lock_held = false; + bool remove_tmp = false; int ret; if (source == NULL || source[0] == '\0') @@ -518,19 +611,11 @@ int pkg_sync(FAR const char *source) return ret; } - lock_held = true; - index = pkg_zalloc(sizeof(*index)); tmp = pkg_path_alloc(); index_path = pkg_path_alloc(); - source_path = pkg_path_alloc(); - if (index == NULL || tmp == NULL || index_path == NULL || - source_path == NULL) + if (index == NULL || tmp == NULL || index_path == NULL) { - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); pkg_error("unable to allocate index metadata buffer"); ret = -ENOMEM; goto out; @@ -540,10 +625,6 @@ int pkg_sync(FAR const char *source) if (ret < 0) { pkg_error("unable to prepare package layout: %d", ret); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); goto out; } @@ -557,10 +638,6 @@ int pkg_sync(FAR const char *source) if (ret < 0 || (size_t)ret >= PATH_MAX) { pkg_error("temporary sync path is too long"); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); ret = -ENAMETOOLONG; goto out; } @@ -569,104 +646,61 @@ int pkg_sync(FAR const char *source) if (ret < 0) { pkg_error("unable to fetch index source '%s': %d", source, ret); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); goto out; } + remove_tmp = true; ret = pkg_metadata_load_index_path(tmp, index); if (ret < 0) { - pkg_store_remove_file(tmp); pkg_error("downloaded index is invalid: %d", ret); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); goto out; } ret = pkg_store_read_text(tmp, &text); if (ret < 0) { - pkg_store_remove_file(tmp); pkg_error("unable to read fetched index: %d", ret); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); goto out; } - ret = pkg_store_format_index_path(index_path, PATH_MAX); - if (ret < 0) - { - pkg_store_remove_file(tmp); - pkg_free(text); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); - pkg_error("unable to resolve local index path: %d", ret); - goto out; - } + /* Commit the catalog and its source in one atomic file replacement. */ - ret = pkg_store_write_text_atomic(index_path, text); + ret = pkg_repo_attach_source(&text, source); if (ret < 0) { - pkg_store_remove_file(tmp); - pkg_free(text); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); - pkg_error("unable to write local index: %d", ret); + pkg_error("unable to record repository source: %d", ret); goto out; } - ret = pkg_store_format_repo_source_path(source_path, PATH_MAX); + ret = pkg_store_format_index_path(index_path, PATH_MAX); if (ret < 0) { - pkg_store_remove_file(tmp); - pkg_free(text); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); - pkg_error("unable to resolve repository source path: %d", ret); + pkg_error("unable to resolve local index path: %d", ret); goto out; } - ret = pkg_store_write_text_atomic(source_path, source); + ret = pkg_store_write_text_atomic(index_path, text); if (ret < 0) { - pkg_store_remove_file(tmp); - pkg_free(text); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); - pkg_error("unable to save repository source: %d", ret); + pkg_error("unable to write local index: %d", ret); goto out; } - pkg_store_remove_file(tmp); - pkg_free(text); - pkg_free(index); - pkg_free(tmp); - pkg_free(index_path); - pkg_free(source_path); pkg_info("synced package index from %s", source); ret = 0; out: - if (lock_held) + if (remove_tmp) { - unlink(lock); + pkg_store_remove_file(tmp); } + pkg_free(text); + pkg_free(index); + pkg_free(tmp); + pkg_free(index_path); + unlink(lock); pkg_free(lock); return ret; } diff --git a/system/nxpkg/pkg_store.c b/system/nxpkg/pkg_store.c index c5ad84bcb0f..562c0e3df30 100644 --- a/system/nxpkg/pkg_store.c +++ b/system/nxpkg/pkg_store.c @@ -518,10 +518,12 @@ int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text) return ret; } - if (close(fd) < 0) + ret = close(fd); + if (ret < 0) { + ret = -errno; unlink(path); - return -errno; + return ret; } return 0; @@ -559,23 +561,29 @@ int pkg_store_write_text_atomic(FAR const char *path, FAR const char *text) * file, corrupting one or both. */ - if (fsync(fd) < 0) + ret = fsync(fd); + if (ret < 0) { + ret = -errno; close(fd); unlink(tmp); - return -errno; + return ret; } - if (close(fd) < 0) + ret = close(fd); + if (ret < 0) { + ret = -errno; unlink(tmp); - return -errno; + return ret; } - if (rename(tmp, path) < 0) + ret = rename(tmp, path); + if (ret < 0) { + ret = -errno; unlink(tmp); - return -errno; + return ret; } return 0; @@ -663,10 +671,12 @@ int pkg_store_copy_file(FAR const char *src, FAR const char *dest) } #endif - if (close(outfd) < 0) + ret = close(outfd); + if (ret < 0) { + ret = -errno; unlink(outpath); - return -errno; + return ret; } #ifndef CONFIG_PSEUDOFS_FILE From f363a8f4448c15a8b7bb5ff9f5f65be562dbf786 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Thu, 23 Jul 2026 23:08:07 +0530 Subject: [PATCH 5/5] system/nxpkg: Protect live locks from filesystem clock skew. Record a per-boot token and owner PID in every package, installed-database, and synchronization lock. A contender now keeps a lock held while its recorded owner task is alive, regardless of unreliable FAT modification timestamps. Reclaim locks from exited owners or earlier boots immediately, while retaining timestamp-based migration handling for legacy empty lock files. This prevents a just-created live lock from being mistaken for a decades-old stale file on targets whose mounted filesystem clock does not match CLOCK_REALTIME. Assisted-by: OpenAI Codex:gpt-5.6-sol Signed-off-by: aviralgarg05 --- system/nxpkg/pkg.h | 1 + system/nxpkg/pkg_install.c | 63 ++---------- system/nxpkg/pkg_repo.c | 10 +- system/nxpkg/pkg_store.c | 191 +++++++++++++++++++++++++++++++++++++ 4 files changed, 204 insertions(+), 61 deletions(-) diff --git a/system/nxpkg/pkg.h b/system/nxpkg/pkg.h index 28c8c19a805..8c1f6850a51 100644 --- a/system/nxpkg/pkg.h +++ b/system/nxpkg/pkg.h @@ -261,6 +261,7 @@ int pkg_resolve_icon_source(FAR char *buffer, size_t size, FAR const struct pkg_manifest_s *manifest); int pkg_acquire_source(FAR const char *source, FAR const char *dest, FAR const char *renew_lock_path); +int pkg_lock_create(FAR const char *path); void pkg_reclaim_stale_lock(FAR const char *path); int pkg_sync(FAR const char *source); int pkg_install(FAR const char *name); diff --git a/system/nxpkg/pkg_install.c b/system/nxpkg/pkg_install.c index cef908c5106..7b326cfe836 100644 --- a/system/nxpkg/pkg_install.c +++ b/system/nxpkg/pkg_install.c @@ -25,13 +25,11 @@ ****************************************************************************/ #include -#include #include #include #include #include #include -#include #include #include "pkg.h" @@ -43,7 +41,6 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, size_t size) { - int fd; int ret; ret = pkg_store_ensure_package_root(name); @@ -58,20 +55,14 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, return ret; } - fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); - if (fd < 0 && errno == EEXIST) + ret = pkg_lock_create(path); + if (ret == -EEXIST) { pkg_reclaim_stale_lock(path); - fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + ret = pkg_lock_create(path); } - if (fd < 0) - { - return errno == EEXIST ? -EBUSY : -errno; - } - - close(fd); - return 0; + return ret == -EEXIST ? -EBUSY : ret; } /**************************************************************************** @@ -85,7 +76,6 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path, static int pkg_install_acquire_installed_lock(FAR char *path, size_t size) { - int fd; int ret; int tries; @@ -102,16 +92,15 @@ static int pkg_install_acquire_installed_lock(FAR char *path, size_t size) for (tries = 0; tries < 100; tries++) { - fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); - if (fd >= 0) + ret = pkg_lock_create(path); + if (ret == 0) { - close(fd); return 0; } - if (errno != EEXIST) + if (ret != -EEXIST) { - return -errno; + return ret; } pkg_reclaim_stale_lock(path); @@ -337,42 +326,6 @@ static int pkg_install_write_pointers( * Public Functions ****************************************************************************/ -/**************************************************************************** - * Name: pkg_reclaim_stale_lock - * - * Description: - * Remove "path" if it is old enough that it cannot belong to a still- - * running holder (see PKG_LOCK_STALE_SECONDS). Best-effort: any - * stat()/unlink() failure just falls through to the normal -EBUSY - * result, since a lock we can't inspect should be treated as held. - * Generic across every lock file this package uses (per-package - * install locks, the shared installed-db lock, pkg_repo.c's sync - * lock) - not install-specific despite living in this file. - * - ****************************************************************************/ - -void pkg_reclaim_stale_lock(FAR const char *path) -{ - struct stat st; - time_t now; - - if (stat(path, &st) < 0) - { - return; - } - - now = time(NULL); - if (now < st.st_mtime || - (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) - { - return; - } - - pkg_error("reclaiming stale lock '%s' (age %ld s)", - path, (long)(now - st.st_mtime)); - unlink(path); -} - int pkg_install(FAR const char *name) { FAR struct pkg_index_s *index; diff --git a/system/nxpkg/pkg_repo.c b/system/nxpkg/pkg_repo.c index 3cd201e8e02..2a4e0ef2775 100644 --- a/system/nxpkg/pkg_repo.c +++ b/system/nxpkg/pkg_repo.c @@ -544,7 +544,6 @@ int pkg_acquire_source(FAR const char *source, FAR const char *dest, static int pkg_repo_acquire_sync_lock(FAR char *path, size_t size) { - int fd; int ret; int tries; @@ -561,16 +560,15 @@ static int pkg_repo_acquire_sync_lock(FAR char *path, size_t size) for (tries = 0; tries < 100; tries++) { - fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); - if (fd >= 0) + ret = pkg_lock_create(path); + if (ret == 0) { - close(fd); return 0; } - if (errno != EEXIST) + if (ret != -EEXIST) { - return -errno; + return ret; } pkg_reclaim_stale_lock(path); diff --git a/system/nxpkg/pkg_store.c b/system/nxpkg/pkg_store.c index 562c0e3df30..c73b7b5831e 100644 --- a/system/nxpkg/pkg_store.c +++ b/system/nxpkg/pkg_store.c @@ -27,6 +27,10 @@ #include #include #include +#include +#include +#include +#include #include #include #include @@ -36,10 +40,84 @@ #include "pkg.h" +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define PKG_LOCK_RECORD_MAGIC "NXPKG1" +#define PKG_LOCK_RECORD_SIZE 64 + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static pthread_once_t g_pkg_lock_boot_once = PTHREAD_ONCE_INIT; +static uint64_t g_pkg_lock_boot_id; + /**************************************************************************** * Private Functions ****************************************************************************/ +static void pkg_lock_init_boot_id(void) +{ + arc4random_buf(&g_pkg_lock_boot_id, sizeof(g_pkg_lock_boot_id)); + if (g_pkg_lock_boot_id == 0) + { + g_pkg_lock_boot_id = 1; + } +} + +static uint64_t pkg_lock_get_boot_id(void) +{ + if (pthread_once(&g_pkg_lock_boot_once, pkg_lock_init_boot_id) != 0) + { + return 1; + } + + return g_pkg_lock_boot_id; +} + +static int pkg_lock_read_owner(FAR const char *path, + FAR uint64_t *boot_id, + FAR pid_t *owner) +{ + char record[PKG_LOCK_RECORD_SIZE]; + unsigned long long parsed_boot; + long parsed_owner; + ssize_t nread; + int fd; + int ret; + + fd = open(path, O_RDONLY); + if (fd < 0) + { + return -errno; + } + + nread = read(fd, record, sizeof(record) - 1); + if (nread < 0) + { + ret = -errno; + close(fd); + return ret; + } + + close(fd); + record[nread] = '\0'; + + ret = sscanf(record, PKG_LOCK_RECORD_MAGIC " %llx %ld", + &parsed_boot, &parsed_owner); + if (ret != 2 || parsed_owner <= 0 || + (long)(pid_t)parsed_owner != parsed_owner) + { + return -EINVAL; + } + + *boot_id = (uint64_t)parsed_boot; + *owner = (pid_t)parsed_owner; + return 0; +} + static int pkg_store_format(FAR char *buffer, size_t size, FAR const char *fmt, FAR const char *name, @@ -194,6 +272,119 @@ int pkg_store_prepare_layout(void) return pkg_store_mkdirs(PKG_TMP_PKG_DIR); } +int pkg_lock_create(FAR const char *path) +{ + char record[PKG_LOCK_RECORD_SIZE]; + int fd; + int ret; + + fd = open(path, O_WRONLY | O_CREAT | O_EXCL, 0644); + if (fd < 0) + { + return -errno; + } + + ret = snprintf(record, sizeof(record), PKG_LOCK_RECORD_MAGIC + " %016" PRIx64 " %ld\n", + pkg_lock_get_boot_id(), (long)getpid()); + if (ret < 0 || (size_t)ret >= sizeof(record)) + { + ret = ret < 0 ? ret : -ENAMETOOLONG; + goto errout; + } + + ret = pkg_store_write_all(fd, record, (size_t)ret); + if (ret < 0) + { + goto errout; + } + + if (fsync(fd) < 0) + { + ret = -errno; + goto errout; + } + + if (close(fd) < 0) + { + ret = -errno; + unlink(path); + return ret; + } + + return 0; + +errout: + close(fd); + unlink(path); + return ret; +} + +void pkg_reclaim_stale_lock(FAR const char *path) +{ + struct stat st; + uint64_t boot_id; + pid_t owner; + time_t now; + int ret; + + ret = pkg_lock_read_owner(path, &boot_id, &owner); + if (ret == -EINVAL) + { + /* A creator may have completed open(O_EXCL) but not its first write. + * Give that very small window time to close before treating the file + * as a legacy timestamp-only lock. + */ + + usleep(20 * 1000); + ret = pkg_lock_read_owner(path, &boot_id, &owner); + } + + if (ret == 0) + { + if (boot_id != pkg_lock_get_boot_id()) + { + pkg_error("reclaiming lock from an earlier boot '%s'", path); + unlink(path); + return; + } + + if (kill(owner, 0) == 0 || errno == EPERM) + { + return; + } + + if (errno == ESRCH) + { + pkg_error("reclaiming lock from exited task %ld '%s'", + (long)owner, path); + unlink(path); + } + + return; + } + + /* Compatibility for empty lock files created by older nxpkg images. + * Their only ownership information is the filesystem timestamp. + */ + + if (stat(path, &st) < 0) + { + return; + } + + now = time(NULL); + if (now < st.st_mtime || + (now - st.st_mtime) < PKG_LOCK_STALE_SECONDS) + { + return; + } + + pkg_error("reclaiming legacy stale lock '%s' (age %ld s)", + path, (long)(now - st.st_mtime)); + unlink(path); +} + int pkg_store_ensure_package_root(FAR const char *name) { char path[PATH_MAX];