[libvirt] [PATCH v2 2/3] virtestmock: Print invalid file accesses into a file

[Peter Krempa]

On Thu, May 12, 2016 at 14:33:08 +0200, Michal Privoznik wrote:
> On 11.05.2016 17:15, Peter Krempa wrote:
> > On Tue, May 10, 2016 at 17:24:13 +0200, Michal Privoznik wrote:

[...]

> >> +static void
> >> +printFile(const char *output,
> >> +          const char *file)
> >> +{
> >> +    FILE *fp;
> >> +    const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
> >> +
> >> +    if (!progname) {
> >> +        progname = getenv("VIR_TEST_MOCK_PROGNAME");
> >> +
> >> +        if (!progname)
> >> +            return;
> >> +    }
> >> +
> >> +    if (!(fp = realfopen(output, "a"))) {
> >> +        fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno));
> >> +        abort();
> > 
> > I'm a bit worried that if this wrapper fails the testsuite will abort.
> > Couldn't we just skip creating/opening the file at this point? You can
> > always error out in the phase where the file is checked.
> 
> But how would I know then that this has failed? Note that this should
> also work in case a single test is ran. Why is aborting each test that
> failed to open the file a bad thing anyway?

In such case it's more than likely that the file will fail to be opened
even for the analysis.

> 
> > 
> >> +    }
> >> +
> >> +    if (flock(fileno(fp), LOCK_EX) < 0) {
> >> +        fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno));
> >> +        fclose(fp);
> >> +        abort();
> > 
> > Won't there be a possibility of collision if two tests are running in
> > parallel?
> 
> I don't know what you mean. That's why I lock the file here so that
> there's no collision.

Never mind. I re-read the man page for flock and noticed that LOCK_EX
whithout LOCK_NB will actually block until the lock is released.

[...]

> >> +
> >> +static void
> >> +cutOffLastComponent(char *path)
> >> +{
> >> +    char *base = path, *p = base;
> >> +
> >> +    while (*p) {
> >> +        if (*p == '/')
> >> +            base = p;
> >> +        p++;
> >> +    }
> >> +
> >> +    if (base != p)
> >> +        *base = '\0';
> > 
> > We already have a function like this:
> > virStorageFileRemoveLastPathComponent.
> > 
> > Also 'strrchr' instead of the while loop?
> 
> Hm.. okay. I can expose the function. I've looked around virfile.c and
> haven't find anything. I didn't check storage driver sources though.

src/util/virstoragefile.c AFAIK

> >>  static void
> >>  checkPath(const char *path)
> >>  {
> >> +    char *fullPath = NULL;
> >> +    char *relPath = NULL;
> >> +    char *crippledPath = NULL;
> >> +
> >> +    if (path[0] != '/' &&
> >> +        asprintf(&relPath, "./%s", path) < 0)
> >> +        goto error;
> >> +
> >> +    /* Le sigh. Both canonicalize_file_name() and realpath()
> >> +     * expect @path to exist otherwise they return an error. So
> >> +     * if we are called over an non-existent file, this could
> >> +     * return an error. In that case do our best and hope we will
> >> +     * catch possible error. */
> > 
> > You can always stat the file to check if it exists before trying to get
> > it's canonical path.
> 
> Why? if stat() succeeds so does canonicalize_file_name() and vice versa.
> Therefore I think calling stat() would be just redundant.

Hmm, fair enough.

> 
> > 
> >> +    if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) {
> >> +        path = fullPath;
> >> +    } else {
> >> +        /* Yeah, our worst nightmares just became true. Path does
> >> +         * not exist. Cut off the last component and retry. */
> >> +        if (!(crippledPath = strdup(relPath ? relPath : path)))
> >> +            goto error;
> > 
> > So if the parrent directory won't exist either you will attempt to do
> > the checking in the non-canonical path?
> > 
> > Maybe you could try to match the paths according to getcwd() to resolve
> > relative paths at first or maybe do this in a loop until you find an
> > existing object. (I'm mostly worried about paths like
> > ../../../../../../something/that/does/not/exist where at least the root
> > directory should be accessible)
> 
> I think this is overcomplicated approach.

Hmm, let's see if it will bite in the future then.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160512/11bfcf43/attachment-0001.sig>