On Wed, Jan 26, 2022 at 02:39:47PM +0100, Claudio wrote:
> 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.
Yes, I came to the same conclusion that it is likely that I made this
modification and tested this. I definitely did no longer hit it after
making sure my tree was clean and carried only your diff (except for the
printing of the file name in the errx()).
Sorry about that.
Diff's ok
> --
> :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;
> }
>
