Skip to content

Commit 455af49

Browse files
committed
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 <gargaviral99@gmail.com>
1 parent 81b8892 commit 455af49

9 files changed

Lines changed: 205 additions & 313 deletions

File tree

system/nxpkg/Kconfig

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,13 @@ config SYSTEM_NXPKG_STACKSIZE
3131

3232
config SYSTEM_NXPKG_ROOT
3333
string "'nxpkg' storage root"
34-
default "/tmp/nxpkg"
34+
default "/var/lib/nxpkg"
3535
---help---
3636
Base directory used by nxpkg for its local index, installed
3737
metadata, temporary downloads, and package payload storage.
38-
Boards with external storage can point this to a persistent
39-
location such as /mnt/sdcard/nxpkg.
38+
The default follows the conventional persistent application-data
39+
location. A board must mount persistent storage at /var or set
40+
this to another persistent location, such as /mnt/sdcard/nxpkg,
41+
if packages need to survive a reset.
4042

4143
endif

system/nxpkg/README.txt

Lines changed: 0 additions & 175 deletions
This file was deleted.

system/nxpkg/pkg.h

Lines changed: 9 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@
3535
#include <stdio.h>
3636
#include <stdlib.h>
3737

38-
#include <nuttx/kmalloc.h>
39-
4038
/****************************************************************************
4139
* Pre-processor Definitions
4240
****************************************************************************/
@@ -58,14 +56,9 @@
5856
#define PKG_CATEGORY_MAX 31
5957
#define PKG_HASH_HEX_LEN 64
6058
/* Each manifest slot is ~1.7KB (dominated by PKG_LAUNCH_ARGS_MAX slots).
61-
* This used to be 6 to fit inside a static internal-DRAM array, but the
62-
* catalog outgrew that once it held every real GSoC package (calc, the
63-
* four games, NXDoom, and the original examples) - system/nxstore/
64-
* nxstore_main.c now heap-allocates its struct pkg_index_s g_index via
65-
* pkg_zalloc(), which draws from the PSRAM-backed user heap on this board
66-
* (CONFIG_ESP32S3_SPIRAM_USER_HEAP) instead of internal DRAM, so this can
67-
* grow without the same pressure. Still not unbounded: keep it sized for
68-
* "a real catalog", not arbitrary attacker-controlled growth.
59+
* Keep the catalog bounded so repository-provided metadata cannot cause
60+
* unbounded memory use. Callers should allocate struct pkg_index_s from
61+
* the application heap rather than placing it on a small task stack.
6962
*/
7063

7164
#define PKG_INDEX_MAX 16
@@ -99,72 +92,22 @@
9992

10093
static inline void *pkg_malloc(size_t size)
10194
{
102-
void *ptr = malloc(size);
103-
104-
if (ptr == NULL)
105-
{
106-
ptr = kmm_malloc(size);
107-
}
108-
109-
return ptr;
95+
return malloc(size);
11096
}
11197

11298
static inline void *pkg_zalloc(size_t size)
11399
{
114-
void *ptr = calloc(1, size);
115-
116-
if (ptr == NULL)
117-
{
118-
ptr = kmm_zalloc(size);
119-
}
120-
121-
return ptr;
100+
return calloc(1, size);
122101
}
123102

124103
static inline void *pkg_realloc(void *ptr, size_t size)
125104
{
126-
if (ptr == NULL)
127-
{
128-
return pkg_malloc(size);
129-
}
130-
131-
if (size == 0)
132-
{
133-
if (kmm_heapmember(ptr))
134-
{
135-
kmm_free(ptr);
136-
}
137-
else
138-
{
139-
free(ptr);
140-
}
141-
142-
return NULL;
143-
}
144-
145-
if (kmm_heapmember(ptr))
146-
{
147-
return kmm_realloc(ptr, size);
148-
}
149-
150105
return realloc(ptr, size);
151106
}
152107

153108
static inline void pkg_free(void *ptr)
154109
{
155-
if (ptr == NULL)
156-
{
157-
return;
158-
}
159-
160-
if (kmm_heapmember(ptr))
161-
{
162-
kmm_free(ptr);
163-
}
164-
else
165-
{
166-
free(ptr);
167-
}
110+
free(ptr);
168111
}
169112

170113
static inline FAR char *pkg_path_alloc(void)
@@ -241,6 +184,7 @@ struct pkg_installed_db_s
241184

242185
const char *pkg_manifest_type_str(enum pkg_payload_type_e type);
243186
int pkg_manifest_validate(FAR const struct pkg_manifest_s *manifest);
187+
bool pkg_validate_path_component(FAR const char *value);
244188
int pkg_manifest_parse_type(FAR const char *value,
245189
FAR enum pkg_payload_type_e *type);
246190

@@ -291,6 +235,8 @@ int pkg_hash_file_sha256(FAR const char *path,
291235
int pkg_metadata_load_index_path(FAR const char *path,
292236
FAR struct pkg_index_s *index);
293237
int pkg_metadata_load_index(FAR struct pkg_index_s *index);
238+
int pkg_metadata_load_manifest_path(FAR const char *path,
239+
FAR struct pkg_manifest_s *manifest);
294240
FAR const struct pkg_manifest_s *
295241
pkg_metadata_find_latest(FAR const struct pkg_index_s *index,
296242
FAR const char *name);

0 commit comments

Comments
 (0)