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.
>
> > 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
>
