linguini1 commented on code in PR #3642:
URL: https://github.com/apache/nuttx-apps/pull/3642#discussion_r3761600743
##########
system/nxpkg/pkg.h:
##########
@@ -27,30 +27,93 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+
#include <limits.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
+#include <stdlib.h>
/****************************************************************************
* 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).
+ * 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
#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) -
Review Comment:
How slow of a link? Did you test this on something or is this hypothesizing.
10 minutes seems pretty reasonable for now, but I would remove this claim.
##########
system/nxpkg/pkg_store.c:
##########
@@ -188,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);
+}
+
Review Comment:
Is compatibility necessary? This application is quite new and unfinished
anyways.
##########
system/nxpkg/pkg_store.c:
##########
@@ -266,21 +468,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.
Review Comment:
This function just checks a length. We don't need to know about another
function's safe naming scheme here, it's confusing. Check the AI generated
comments for relevance.
##########
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
Review Comment:
Why check this if you'll just return the empty string anyways?
##########
system/nxpkg/pkg_log.c:
##########
@@ -26,20 +26,28 @@
#include <stdarg.h>
#include <stdio.h>
+#include <string.h>
+#include <syslog.h>
#include "pkg.h"
/****************************************************************************
* 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);
Review Comment:
Don't add level to the message. There is already a syslog option that allows
level to be logged.
I would suggest a macro that just compile time prefixes the message with
`nxpkg` if you need that, but there is also a syslog option that allows logging
the process name as well which is a better choice.
##########
system/nxpkg/pkg.h:
##########
@@ -27,30 +27,93 @@
* Included Files
****************************************************************************/
+#include <nuttx/config.h>
+
#include <limits.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
+#include <stdlib.h>
/****************************************************************************
* 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).
+ * 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
#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)
+{
+ return malloc(size);
+}
+
+static inline void *pkg_zalloc(size_t size)
+{
+ return calloc(1, size);
+}
+
+static inline void *pkg_realloc(void *ptr, size_t size)
+{
+ return realloc(ptr, size);
+}
+
+static inline void pkg_free(void *ptr)
+{
+ free(ptr);
+}
+
+static inline FAR char *pkg_path_alloc(void)
+{
+ return pkg_malloc(PATH_MAX);
+}
Review Comment:
Why
##########
system/nxpkg/pkg_store.c:
##########
@@ -266,21 +468,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
Review Comment:
Don't mention "this SD card". Just refer to short name FAT. This is an
application
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
