On Wed, Jan 26, 2022 at 12:36:31PM +0100, Sebastian Benoit wrote:
> Claudio Jeker([email protected]) 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;
}
