diff --git a/rust/flintlang/c-src/src/pkg.c b/rust/flintlang/c-src/src/pkg.c index aca7b39..1d3564c 100644 --- a/rust/flintlang/c-src/src/pkg.c +++ b/rust/flintlang/c-src/src/pkg.c @@ -82,7 +82,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, int depth, char *error, size_t error_size, - int from); + int from, + bool *success); /* * Run git and capture its stdout. argv[0] is "git", the rest are plain @@ -2093,6 +2094,7 @@ static int pkg_install_core(bool update_all, const char *update_one) int count = 0; if (!pkg_collect_deps(&manifest, &deps, &count, error, sizeof(error))) { fprintf(stderr, "flint pkg: %s\n", error); + pkg_free_deps(deps, count); pkg_free_manifest(&manifest); return 65; } @@ -2152,6 +2154,7 @@ static int pkg_install_core(bool update_all, const char *update_one) * realloc, and reading the old pointer afterwards is the * use-after-free that produced a garbage path here. */ int filled = count; + bool expand_ok = true; PkgDep *expanded = pkg_expand_deps(deps, count, &capacity, @@ -2160,15 +2163,16 @@ static int pkg_install_core(bool update_all, const char *update_one) 0, error, sizeof(error), - 0); - if (expanded == NULL) { + 0, + &expand_ok); + deps = expanded; + count = filled; + if (!expand_ok) { fprintf(stderr, "flint pkg: %s\n", error); pkg_free_deps(deps, count); pkg_free_locked(locked, locked_count); return 65; } - deps = expanded; - count = filled; /* dedupe by name: two packages can reach the same one by * different routes, and it should be installed once. A name @@ -2926,14 +2930,16 @@ static PkgDep *pkg_expand_deps(PkgDep *list, int depth, char *error, size_t error_size, - int from) + int from, + bool *success) { - if (from == 0 && depth > 32) { + if (depth > 32) { snprintf(error, error_size, "dependency chain is deeper than 32 -- is there a " "cycle?"); - return NULL; + *success = false; + return list; } /* count first: every append can move the array, so the loop below @@ -2951,19 +2957,27 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * reads as though it does not, and the lockfile should not * carry that spelling either. */ + int path_len; if (strcmp(base_dir, ".") == 0) - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/flint.toml", list[i].path); else - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/%s/flint.toml", base_dir, list[i].path); - if (!pkg_load(manifest_path, &child, error, error_size)) - return NULL; + if (path_len < 0 || (size_t)path_len >= sizeof(manifest_path)) { + snprintf(error, error_size, "dependency manifest path too long"); + *success = false; + return list; + } + if (!pkg_load(manifest_path, &child, error, error_size)) { + *success = false; + return list; + } PkgDep *nested = NULL; int nested_count = 0; bool ok = pkg_collect_deps( @@ -2971,7 +2985,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, pkg_free_manifest(&child); if (!ok) { pkg_free_deps(nested, nested_count); - return NULL; + *success = false; + return list; } added += nested_count; pkg_free_deps(nested, nested_count); @@ -2990,7 +3005,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, PkgDep *bigger = realloc(list, (size_t)grown * sizeof(PkgDep)); if (bigger == NULL) { snprintf(error, error_size, "out of memory"); - return NULL; + *success = false; + return list; } list = bigger; *capacity = grown; @@ -3016,19 +3032,27 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * reads as though it does not, and the lockfile should not * carry that spelling either. */ + int path_len; if (strcmp(base_dir, ".") == 0) - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/flint.toml", list[i].path); else - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/%s/flint.toml", base_dir, list[i].path); - if (!pkg_load(manifest_path, &child, error, error_size)) - return NULL; + if (path_len < 0 || (size_t)path_len >= sizeof(manifest_path)) { + snprintf(error, error_size, "dependency manifest path too long"); + *success = false; + return list; + } + if (!pkg_load(manifest_path, &child, error, error_size)) { + *success = false; + return list; + } PkgDep *nested = NULL; int nested_count = 0; bool ok = pkg_collect_deps( @@ -3036,7 +3060,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, pkg_free_manifest(&child); if (!ok) { pkg_free_deps(nested, nested_count); - return NULL; + *success = false; + return list; } for (int k = 0; k < nested_count; k++) { /* a nested path is relative to its parent package */ @@ -3048,24 +3073,46 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * it, and "." must not contribute a * component or the lockfile carries a * spelling nobody wrote. */ + int joined_len; if (strcmp(base_dir, ".") == 0) - snprintf(joined, + joined_len = snprintf(joined, sizeof(joined), "%s", list[i].path); else - snprintf(joined, + joined_len = snprintf(joined, sizeof(joined), "%s/%s", base_dir, list[i].path); + if (joined_len < 0 || + (size_t)joined_len >= sizeof(joined)) { + pkg_free_deps(nested, nested_count); + free(nested); + snprintf(error, + error_size, + "dependency path is too long"); + *success = false; + return list; + } char *absolute = pkg_normalize_path( joined, nested[k].path); free(nested[k].path); + if (absolute == NULL) { + nested[k].path = NULL; + pkg_free_deps(nested, nested_count); + free(nested); + snprintf(error, error_size, "out of memory"); + *success = false; + return list; + } nested[k].path = absolute; } list[count++] = nested[k]; + memset(&nested[k], 0, sizeof(nested[k])); + *filled = count; } + free(nested); } *filled = count; @@ -3078,5 +3125,6 @@ static PkgDep *pkg_expand_deps(PkgDep *list, depth + 1, error, error_size, - start); + start, + success); } diff --git a/src/pkg.c b/src/pkg.c index fb01878..a0a80c5 100644 --- a/src/pkg.c +++ b/src/pkg.c @@ -82,7 +82,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, int depth, char *error, size_t error_size, - int from); + int from, + bool *success); /* * Run git and capture its stdout. argv[0] is "git", the rest are plain @@ -2123,6 +2124,7 @@ static int pkg_install_core(bool update_all, const char *update_one) int count = 0; if (!pkg_collect_deps(&manifest, &deps, &count, error, sizeof(error))) { fprintf(stderr, "flint pkg: %s\n", error); + pkg_free_deps(deps, count); pkg_free_manifest(&manifest); return 65; } @@ -2182,6 +2184,7 @@ static int pkg_install_core(bool update_all, const char *update_one) * realloc, and reading the old pointer afterwards is the * use-after-free that produced a garbage path here. */ int filled = count; + bool expand_ok = true; PkgDep *expanded = pkg_expand_deps(deps, count, &capacity, @@ -2190,15 +2193,16 @@ static int pkg_install_core(bool update_all, const char *update_one) 0, error, sizeof(error), - 0); - if (expanded == NULL) { + 0, + &expand_ok); + deps = expanded; + count = filled; + if (!expand_ok) { fprintf(stderr, "flint pkg: %s\n", error); pkg_free_deps(deps, count); pkg_free_locked(locked, locked_count); return 65; } - deps = expanded; - count = filled; /* dedupe by name: two packages can reach the same one by * different routes, and it should be installed once. A name @@ -2956,14 +2960,16 @@ static PkgDep *pkg_expand_deps(PkgDep *list, int depth, char *error, size_t error_size, - int from) + int from, + bool *success) { - if (from == 0 && depth > 32) { + if (depth > 32) { snprintf(error, error_size, "dependency chain is deeper than 32 -- is there a " "cycle?"); - return NULL; + *success = false; + return list; } /* count first: every append can move the array, so the loop below @@ -2981,19 +2987,27 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * reads as though it does not, and the lockfile should not * carry that spelling either. */ + int path_len; if (strcmp(base_dir, ".") == 0) - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/flint.toml", list[i].path); else - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/%s/flint.toml", base_dir, list[i].path); - if (!pkg_load(manifest_path, &child, error, error_size)) - return NULL; + if (path_len < 0 || (size_t)path_len >= sizeof(manifest_path)) { + snprintf(error, error_size, "dependency manifest path too long"); + *success = false; + return list; + } + if (!pkg_load(manifest_path, &child, error, error_size)) { + *success = false; + return list; + } PkgDep *nested = NULL; int nested_count = 0; bool ok = pkg_collect_deps( @@ -3001,7 +3015,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, pkg_free_manifest(&child); if (!ok) { pkg_free_deps(nested, nested_count); - return NULL; + *success = false; + return list; } added += nested_count; pkg_free_deps(nested, nested_count); @@ -3020,7 +3035,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, PkgDep *bigger = realloc(list, (size_t)grown * sizeof(PkgDep)); if (bigger == NULL) { snprintf(error, error_size, "out of memory"); - return NULL; + *success = false; + return list; } list = bigger; *capacity = grown; @@ -3046,19 +3062,27 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * reads as though it does not, and the lockfile should not * carry that spelling either. */ + int path_len; if (strcmp(base_dir, ".") == 0) - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/flint.toml", list[i].path); else - snprintf(manifest_path, + path_len = snprintf(manifest_path, sizeof(manifest_path), "%s/%s/flint.toml", base_dir, list[i].path); - if (!pkg_load(manifest_path, &child, error, error_size)) - return NULL; + if (path_len < 0 || (size_t)path_len >= sizeof(manifest_path)) { + snprintf(error, error_size, "dependency manifest path too long"); + *success = false; + return list; + } + if (!pkg_load(manifest_path, &child, error, error_size)) { + *success = false; + return list; + } PkgDep *nested = NULL; int nested_count = 0; bool ok = pkg_collect_deps( @@ -3066,7 +3090,8 @@ static PkgDep *pkg_expand_deps(PkgDep *list, pkg_free_manifest(&child); if (!ok) { pkg_free_deps(nested, nested_count); - return NULL; + *success = false; + return list; } for (int k = 0; k < nested_count; k++) { /* a nested path is relative to its parent package */ @@ -3078,24 +3103,46 @@ static PkgDep *pkg_expand_deps(PkgDep *list, * it, and "." must not contribute a * component or the lockfile carries a * spelling nobody wrote. */ + int joined_len; if (strcmp(base_dir, ".") == 0) - snprintf(joined, + joined_len = snprintf(joined, sizeof(joined), "%s", list[i].path); else - snprintf(joined, + joined_len = snprintf(joined, sizeof(joined), "%s/%s", base_dir, list[i].path); + if (joined_len < 0 || + (size_t)joined_len >= sizeof(joined)) { + pkg_free_deps(nested, nested_count); + free(nested); + snprintf(error, + error_size, + "dependency path is too long"); + *success = false; + return list; + } char *absolute = pkg_normalize_path( joined, nested[k].path); free(nested[k].path); + if (absolute == NULL) { + nested[k].path = NULL; + pkg_free_deps(nested, nested_count); + free(nested); + snprintf(error, error_size, "out of memory"); + *success = false; + return list; + } nested[k].path = absolute; } list[count++] = nested[k]; + memset(&nested[k], 0, sizeof(nested[k])); + *filled = count; } + free(nested); } *filled = count; @@ -3108,5 +3155,6 @@ static PkgDep *pkg_expand_deps(PkgDep *list, depth + 1, error, error_size, - start); + start, + success); } diff --git a/tests/pkg_test.sh b/tests/pkg_test.sh index 7d1d2d6..dd290a9 100644 --- a/tests/pkg_test.sh +++ b/tests/pkg_test.sh @@ -297,6 +297,37 @@ expect_contains "the lock records the nested source" "../base" \ "$FLINT" pkg install >/dev/null 2>&1 expect "reinstalling the chain succeeds" 0 "$?" +# Cyclic path dependencies must stop with an error, not recurse indefinitely. +mkdir -p "$WORK/cycle/a" "$WORK/cycle/b" "$WORK/cycle/app" +cat > "$WORK/cycle/a/flint.toml" <<'EOF3' +[package] +name = "cycle-a" +version = "1.0.0" + +[dependencies] +cycle-b = { path = "../b" } +EOF3 +cat > "$WORK/cycle/b/flint.toml" <<'EOF3' +[package] +name = "cycle-b" +version = "1.0.0" + +[dependencies] +cycle-a = { path = "../a" } +EOF3 +cat > "$WORK/cycle/app/flint.toml" <<'EOF3' +[package] +name = "cycle-app" +version = "0.1.0" + +[dependencies] +cycle-a = { path = "../a" } +EOF3 +cd "$WORK/cycle/app" || exit 1 +out=$("$FLINT" pkg install 2>&1) && status=0 || status=$? +expect "cyclic dependencies fail" "1" "$([ "$status" -ne 0 ] && echo 1 || echo 0)" +expect_contains "cycle failure is bounded and clear" "dependency chain is deeper than 32" "$out" + cd "$WORK" || exit 1 if [ "$failed" -ne 0 ]; then echo "pkg tests: $checks checks, $failed failures"