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.
> One tiny nit inline.
>
> > @@ -1270,40 +1279,38 @@ 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);
> > 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);
>
> The dance with base is not necessary:
>
> free(fp->file)
> if ((fp->file = strdup(fn)) == NULL)
> err(1, NULL);
>
fn points into fp->file (see further up e.g. fn = fp->file + rsyncsz;
So free() before strdup will result in a use after free.
--
:wq Claudio
