On Wed, Jan 26, 2022 at 12:36:31PM +0100, Sebastian Benoit wrote: > Claudio Jeker(cje...@diehard.n-r-g.com) on 2022.01.26 11:54:41 +0100: > > On Wed, Jan 26, 2022 at 11:43:25AM +0100, Theo Buehler wrote: > > > On Wed, Jan 26, 2022 at 10:06:37AM +0100, Claudio Jeker wrote: > > > > This diff removes the valid/ subdir in favor of a more direct directory > > > > layout for all valid CA repository files. > > > > It moves rrdp and rsync to .rsync and .rrdp but keeps ta/ because trust > > > > anchors are special. > > > > > > > > The biggest change is probably in the FTS code to cleanup the repo since > > > > the traversing now starts at ".". Apart from that removing valid results > > > > in some simplified code (esp. in -f mode). > > > > > > > > This diff is mostly from job@, I just did fixed the FTS bits so the > > > > cleanup works again. > > > > > > If I do a full run with this diff, I run into > > > > > > rpki-client: both possibilities of file present > > > > > > Instrumenting this error (should it include fn?) it turns out to be > > > > Yes, it should show at least one fn. > > > > > pki-repo.registro.br/repo/A2x6icaKpVWP5uUBzJQzRvEpjvS7PV2qt1Ct/1/3230302e3138392e34302e302f32332d3234203d3e20313roa > > > > > > I have not yet figured out why. > > > > Grrrrr. I once hit this but thought it was something that was wrong with > > some of my other diffs I have around. Need to look into this. It it for > > sure something that should not happen. > > interestingly, i did not get that. > > diff reads ok to me otherwise.
I added a check to leave empty .rsync and .rrdp dirs behind. Also the 'both possibilities of file present' error now prints the filename. Apart from that this is the same diff. tb@ did you manage to reproduce this error? I only hit it if I free(fp->file) first and with that create the use-after-free on the filename. -- :wq Claudio Index: parser.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v retrieving revision 1.54 diff -u -p -r1.54 parser.c --- parser.c 24 Jan 2022 17:29:37 -0000 1.54 +++ parser.c 25 Jan 2022 16:36:12 -0000 @@ -737,9 +737,9 @@ parse_entity(struct entityq *q, struct m * verification. */ static void -parse_load_crl(const char *uri) +parse_load_crl(char *uri) { - char *nfile, *f; + char *f; size_t flen; if (uri == NULL) @@ -750,19 +750,14 @@ parse_load_crl(const char *uri) } uri += strlen("rsync://"); - if (asprintf(&nfile, "valid/%s", uri) == -1) - err(1, NULL); - - f = load_file(nfile, &flen); + f = load_file(uri, &flen); if (f == NULL) { - warn("parse file %s", nfile); - goto done; + warn("parse file %s", uri); + return; } - proc_parser_crl(nfile, f, flen); + proc_parser_crl(uri, f, flen); -done: - free(nfile); free(f); } @@ -773,10 +768,10 @@ done: * necessary certs were loaded. Returns NULL on failure. */ static struct cert * -parse_load_cert(const char *uri) +parse_load_cert(char *uri) { struct cert *cert = NULL; - char *nfile, *f; + char *f; size_t flen; if (uri == NULL) @@ -788,33 +783,28 @@ parse_load_cert(const char *uri) } uri += strlen("rsync://"); - if (asprintf(&nfile, "valid/%s", uri) == -1) - err(1, NULL); - - f = load_file(nfile, &flen); + f = load_file(uri, &flen); if (f == NULL) { - warn("parse file %s", nfile); + warn("parse file %s", uri); goto done; } - cert = cert_parse(nfile, f, flen); + cert = cert_parse(uri, f, flen); free(f); if (cert == NULL) goto done; if (cert->purpose != CERT_PURPOSE_CA) { - warnx("AIA reference to bgpsec cert %s", nfile); + warnx("AIA reference to bgpsec cert %s", uri); goto done; } /* try to load the CRL of this cert */ parse_load_crl(cert->crl); - free(nfile); return cert; -done: + done: cert_free(cert); - free(nfile); return NULL; } Index: repo.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v retrieving revision 1.27 diff -u -p -r1.27 repo.c --- repo.c 24 Jan 2022 15:50:34 -0000 1.27 +++ repo.c 26 Jan 2022 11:48:48 -0000 @@ -224,8 +224,14 @@ repo_dir(const char *uri, const char *di local = uri; } - if (asprintf(&out, "%s/%s", dir, local) == -1) - err(1, NULL); + if (dir == NULL) { + if ((out = strdup(local)) == NULL) + err(1, NULL); + } else { + if (asprintf(&out, "%s/%s", dir, local) == -1) + err(1, NULL); + } + free(hdir); return out; } @@ -436,7 +442,7 @@ rsync_get(const char *uri, const char *v SLIST_INSERT_HEAD(&rsyncrepos, rr, entry); rr->repouri = repo; - rr->basedir = repo_dir(repo, "rsync", 0); + rr->basedir = repo_dir(repo, ".rsync", 0); /* create base directory */ if (mkpath(rr->basedir) == -1) { @@ -484,13 +490,16 @@ rrdp_filename(const struct rrdprepo *rr, char *nfile; const char *dir = rr->basedir; - if (valid) - dir = "valid"; if (!valid_uri(uri, strlen(uri), "rsync://")) errx(1, "%s: bad URI %s", rr->basedir, uri); uri += strlen("rsync://"); /* skip proto */ - if (asprintf(&nfile, "%s/%s", dir, uri) == -1) - err(1, NULL); + if (valid) { + if ((nfile = strdup(uri)) == NULL) + err(1, NULL); + } else { + if (asprintf(&nfile, "%s/%s", dir, uri) == -1) + err(1, NULL); + } return nfile; } @@ -734,7 +743,7 @@ rrdp_get(const char *uri) if ((rr->notifyuri = strdup(uri)) == NULL) err(1, NULL); - rr->basedir = repo_dir(uri, "rrdp", 1); + rr->basedir = repo_dir(uri, ".rrdp", 1); RB_INIT(&rr->deleted); @@ -1058,7 +1067,7 @@ repo_lookup(int talid, const char *uri, } rp = repo_alloc(talid); - rp->basedir = repo_dir(repouri, "valid", 0); + rp->basedir = repo_dir(repouri, NULL, 0); rp->repouri = repouri; if (notify != NULL) if ((rp->notifyuri = strdup(notify)) == NULL) @@ -1270,42 +1279,40 @@ static void repo_move_valid(struct filepath_tree *tree) { struct filepath *fp, *nfp; - size_t rsyncsz = strlen("rsync/"); - size_t rrdpsz = strlen("rrdp/"); + size_t rsyncsz = strlen(".rsync/"); + size_t rrdpsz = strlen(".rrdp/"); char *fn, *base; RB_FOREACH_SAFE(fp, filepath_tree, tree, nfp) { - if (strncmp(fp->file, "rsync/", rsyncsz) != 0 && - strncmp(fp->file, "rrdp/", rrdpsz) != 0) + if (strncmp(fp->file, ".rsync/", rsyncsz) != 0 && + strncmp(fp->file, ".rrdp/", rrdpsz) != 0) continue; /* not a temporary file path */ - if (strncmp(fp->file, "rsync/", rsyncsz) == 0) { - if (asprintf(&fn, "valid/%s", fp->file + rsyncsz) == -1) - err(1, NULL); + if (strncmp(fp->file, ".rsync/", rsyncsz) == 0) { + fn = fp->file + rsyncsz; } else { base = strchr(fp->file + rrdpsz, '/'); assert(base != NULL); - if (asprintf(&fn, "valid/%s", base + 1) == -1) - err(1, NULL); + fn = base + 1; } - if (repo_mkpath(fn) == -1) { - free(fn); + if (repo_mkpath(fn) == -1) continue; - } if (rename(fp->file, fn) == -1) { warn("rename %s", fp->file); - free(fn); continue; } /* switch filepath node to new path */ RB_REMOVE(filepath_tree, tree, fp); - free(fp->file); - fp->file = fn; + base = fp->file; + if ((fp->file = strdup(fn)) == NULL) + err(1, NULL); + free(base); if (RB_INSERT(filepath_tree, tree, fp) != NULL) - errx(1, "both possibilities of file present"); + errx(1, "%s: both possibilities of file present", + fp->file); } } @@ -1313,12 +1320,20 @@ repo_move_valid(struct filepath_tree *tr #define RSYNC_DIR (void *)0x73796e63 #define RRDP_DIR (void *)0x52524450 +static inline char * +skip_dotslash(char *in) +{ + if (memcmp(in, "./", 2) == 0) + return in + 2; + return in; +} + void repo_cleanup(struct filepath_tree *tree) { size_t i, cnt, delsz = 0, dirsz = 0; char **del = NULL, **dir = NULL; - char *argv[5] = { "ta", "rsync", "rrdp", "valid", NULL }; + char *argv[2] = { ".", NULL }; FTS *fts; FTSENT *e; @@ -1331,38 +1346,38 @@ repo_cleanup(struct filepath_tree *tree) err(1, "fts_open"); errno = 0; while ((e = fts_read(fts)) != NULL) { + char *path = skip_dotslash(e->fts_path); switch (e->fts_info) { case FTS_NSOK: - /* handle rrdp .state files explicitly */ - if (e->fts_parent->fts_pointer == RRDP_DIR && - e->fts_level == 2 && - strcmp(e->fts_name, ".state") == 0) { + if (filepath_exists(tree, path)) { e->fts_parent->fts_number++; - } else if (filepath_exists(tree, e->fts_path)) { + break; + } + if (e->fts_parent->fts_pointer == RRDP_DIR) { e->fts_parent->fts_number++; - } else if (e->fts_parent->fts_pointer == RRDP_DIR) { + /* handle rrdp .state files explicitly */ + if (e->fts_level == 3 && + strcmp(e->fts_name, ".state") == 0) + break; /* can't delete these extra files */ - e->fts_parent->fts_number++; stats.extra_files++; if (verbose > 1) - logx("superfluous %s", e->fts_path); - } else { - if (e->fts_parent->fts_pointer == RSYNC_DIR) { - /* no need to keep rsync files */ - stats.extra_files++; - if (verbose > 1) - logx("superfluous %s", - e->fts_path); - } - del = add_to_del(del, &delsz, - e->fts_path); + logx("superfluous %s", path); + break; + } + if (e->fts_parent->fts_pointer == RSYNC_DIR) { + /* no need to keep rsync files */ + stats.extra_files++; + if (verbose > 1) + logx("superfluous %s", path); } + del = add_to_del(del, &delsz, path); break; case FTS_D: - if (e->fts_level == FTS_ROOTLEVEL) { - if (strcmp("rsync", e->fts_path) == 0) + if (e->fts_level == 1) { + if (strcmp(".rsync", e->fts_name) == 0) e->fts_pointer = RSYNC_DIR; - else if (strcmp("rrdp", e->fts_path) == 0) + else if (strcmp(".rrdp", e->fts_name) == 0) e->fts_pointer = RRDP_DIR; else e->fts_pointer = BASE_DIR; @@ -1375,35 +1390,40 @@ repo_cleanup(struct filepath_tree *tree) * only if rrdp is active. */ if (e->fts_pointer == RRDP_DIR && !noop && rrdpon && - e->fts_level == 1) { - if (!rrdp_is_active(e->fts_path)) + e->fts_level == 2) { + if (!rrdp_is_active(path)) e->fts_pointer = NULL; } break; case FTS_DP: if (e->fts_level == FTS_ROOTLEVEL) break; + if (e->fts_level == 1) + /* do not remove .rsync and .rrdp */ + if (e->fts_pointer == RRDP_DIR || + e->fts_pointer == RSYNC_DIR) + break; if (e->fts_number == 0) - dir = add_to_del(dir, &dirsz, e->fts_path); + dir = add_to_del(dir, &dirsz, path); e->fts_parent->fts_number += e->fts_number; break; case FTS_SL: case FTS_SLNONE: - warnx("symlink %s", e->fts_path); - del = add_to_del(del, &delsz, e->fts_path); + warnx("symlink %s", path); + del = add_to_del(del, &delsz, path); break; case FTS_NS: case FTS_ERR: if (e->fts_errno == ENOENT && e->fts_level == FTS_ROOTLEVEL) continue; - warnx("fts_read %s: %s", e->fts_path, + warnx("fts_read %s: %s", path, strerror(e->fts_errno)); break; default: - warnx("unhandled[%x] %s", e->fts_info, - e->fts_path); + warnx("fts_read %s: unhandled[%x]", path, + e->fts_info); break; }