Created attachment 1594823 [details]
the nfs-server test that originally uncovered the problem

Not too familiar with this code so I'm not sure about this but I think libblkid may just be missing a call to blkid_free_probe(pr->disk_probe); inside blkid_probe_get_wholedisk_probe() or thereabouts.

Actually it's inside blkid_partitions_probe_partition since that is the caller of blkid_probe_get_wholedisk_probe. This is along the lines of I think we need

commit c90afaf082f76d2d7446001353c90bf2cb662c18
Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Tue Jul 30 17:41:03 2019 -0400

    blkid: free blkid_probe at end of partition probe to avoid leak
    
    fixes https://bugzilla.redhat.com/show_bug.cgi?id=1734545
    
    Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

diff --git a/libblkid/src/partitions/partitions.c b/libblkid/src/partitions/partitions.c
index f12638e..8e2540d 100644
--- a/libblkid/src/partitions/partitions.c
+++ b/libblkid/src/partitions/partitions.c
@@ -797,10 +797,14 @@ static int blkid_partitions_probe_partition(blkid_probe pr)
                                major(disk), minor(disk));
        }
 
+       if (disk_pr)
+               blkid_free_probe(pr->disk_probe);
        DBG(LOWPROBE, ul_debug("parts: end probing for partition entry [success]"));
        return BLKID_PROBE_OK;
 
 nothing:
+       if (disk_pr)
+               blkid_free_probe(pr->disk_probe);
        DBG(LOWPROBE, ul_debug("parts: end probing for partition entry [nothing]"));
        return BLKID_PROBE_NONE;

(
> --- a/libblkid/src/partitions/partitions.c
> +++ b/libblkid/src/partitions/partitions.c
> @@ -797,10 +797,14 @@ static int
> blkid_partitions_probe_partition(blkid_probe pr)
>                                 major(disk), minor(disk));
>         }
>  
> +       if (disk_pr)
> +               blkid_free_probe(pr->disk_probe);

It seems incorrect.

See libblkid/src/probe.c:blkid_free_probe(), it deallocates the disk_probe when the main @pr 
struct is deallocated. It also means that your patch introduces double free as it does not
set pr->disk_probe to NULL.

It seems the problem is in the application rather than in the library.

The probing struct is associated with the cache. You need to call blkid_put_cache() to close and 
free all related stuff. And there is a small typo in the test; you wants to use st_rdev, no st_dev.

Try this patch for your test:

-- blkid-test-org.c	2019-07-31 12:38:26.486762301 +0200
+++ blkid-test.c	2019-07-31 12:42:13.028521484 +0200
@@ -40,12 +40,11 @@
 }
 /* END from https://stackoverflow.com/questions/6583158/finding-open-file-descriptors-for-a-process-linux-c-code */
 
-static const char *get_uuid_blkdev(char *path)
+static const char *get_uuid_blkdev(char *path, blkid_cache *cache)
 {
 	/* We set *safe if we know that we need the
 	 * fsid from statfs too.
 	 */
-	static blkid_cache cache = NULL;
 	struct stat stb;
 	char *devname;
 	blkid_tag_iterate iter;
@@ -53,17 +52,17 @@
 	const char *type;
 	const char *val, *uuid = NULL;
 
-	if (cache == NULL)
-		blkid_get_cache(&cache, NULL);
+	if (!*cache)
+		blkid_get_cache(cache, NULL);
 
 	if (stat(path, &stb) != 0)
 		return NULL;
-	devname = blkid_devno_to_devname(stb.st_dev);
+	devname = blkid_devno_to_devname(stb.st_rdev);
 	if (!devname)
 		return NULL;
 	printf("calling blkid_get_dev(cache, devname=%s, BLKID_DEV_NORMAL)\n",
 		devname);
-	dev = blkid_get_dev(cache, devname, BLKID_DEV_NORMAL);
+	dev = blkid_get_dev(*cache, devname, BLKID_DEV_NORMAL);
 	free(devname);
 	if (!dev)
 		return NULL;
@@ -88,6 +87,7 @@
 {
 	const char *uuid;
 	int before, after;
+	blkid_cache cache = NULL;;
 
 	if (argc < 1) {
 		printf("Please enter path to device");
@@ -95,7 +95,9 @@
 	}
 	before = get_num_fds();
 	// printf("BEFORE get_uuid_blkdev num_fds = %d\n", before);
-	uuid = get_uuid_blkdev(argv[1]);
+	uuid = get_uuid_blkdev(argv[1], &cache);
+	printf("uuid = %s\n", uuid);
+	blkid_put_cache(cache);
 	after = get_num_fds();
 	// printf("AFTER get_uuid_blkdev num_fds = %d\n", after);
 	if (before != after)
@@ -105,6 +107,5 @@
 		printf("ERROR: uuid = NULL\n");
 		exit(1);
 	}
-	printf("uuid = %s\n", uuid);
 	exit(0);
 }

Note that after blkid_put_cache() the uuid string will not be available, you have to use it before blkid_put_cache() (or strdup()).

I don't see any leak. Not a bug from my point of view.

(In reply to Karel Zak from comment #4)
> (
> > --- a/libblkid/src/partitions/partitions.c
> > +++ b/libblkid/src/partitions/partitions.c
> > @@ -797,10 +797,14 @@ static int
> > blkid_partitions_probe_partition(blkid_probe pr)
> >                                 major(disk), minor(disk));
> >         }
> >  
> > +       if (disk_pr)
> > +               blkid_free_probe(pr->disk_probe);
> 
> It seems incorrect.
> 
> See libblkid/src/probe.c:blkid_free_probe(), it deallocates the disk_probe
> when the main @pr 
> struct is deallocated. It also means that your patch introduces double free
> as it does not
> set pr->disk_probe to NULL.

Yes I agree this is all wrong - you beat me to it.

(In reply to Karel Zak from comment #5)
> It seems the problem is in the application rather than in the library.
> 
> The probing struct is associated with the cache. You need to call
> blkid_put_cache() to close and 
> free all related stuff. And there is a small typo in the test; you wants to
> use st_rdev, no st_dev.
> 
> Try this patch for your test:
> 

I wonder why RHEL6 does not have this problem.

I cut/pasted that from rpc.mountd cache.c code though so if you're right it means this is an nfs-utils bug.


> -- blkid-test-org.c	2019-07-31 12:38:26.486762301 +0200
> +++ blkid-test.c	2019-07-31 12:42:13.028521484 +0200
> @@ -40,12 +40,11 @@
>  }
>  /* END from
> https://stackoverflow.com/questions/6583158/finding-open-file-descriptors-
> for-a-process-linux-c-code */
>  
> -static const char *get_uuid_blkdev(char *path)
> +static const char *get_uuid_blkdev(char *path, blkid_cache *cache)
>  {
>  	/* We set *safe if we know that we need the
>  	 * fsid from statfs too.
>  	 */
> -	static blkid_cache cache = NULL;
>  	struct stat stb;
>  	char *devname;
>  	blkid_tag_iterate iter;
> @@ -53,17 +52,17 @@
>  	const char *type;
>  	const char *val, *uuid = NULL;
>  
> -	if (cache == NULL)
> -		blkid_get_cache(&cache, NULL);
> +	if (!*cache)
> +		blkid_get_cache(cache, NULL);
>  
>  	if (stat(path, &stb) != 0)
>  		return NULL;
> -	devname = blkid_devno_to_devname(stb.st_dev);
> +	devname = blkid_devno_to_devname(stb.st_rdev);
>  	if (!devname)
>  		return NULL;
>  	printf("calling blkid_get_dev(cache, devname=%s, BLKID_DEV_NORMAL)\n",
>  		devname);
> -	dev = blkid_get_dev(cache, devname, BLKID_DEV_NORMAL);
> +	dev = blkid_get_dev(*cache, devname, BLKID_DEV_NORMAL);
>  	free(devname);
>  	if (!dev)
>  		return NULL;
> @@ -88,6 +87,7 @@
>  {
>  	const char *uuid;
>  	int before, after;
> +	blkid_cache cache = NULL;;
>  
>  	if (argc < 1) {
>  		printf("Please enter path to device");
> @@ -95,7 +95,9 @@
>  	}
>  	before = get_num_fds();
>  	// printf("BEFORE get_uuid_blkdev num_fds = %d\n", before);
> -	uuid = get_uuid_blkdev(argv[1]);
> +	uuid = get_uuid_blkdev(argv[1], &cache);
> +	printf("uuid = %s\n", uuid);
> +	blkid_put_cache(cache);
>  	after = get_num_fds();
>  	// printf("AFTER get_uuid_blkdev num_fds = %d\n", after);
>  	if (before != after)
> @@ -105,6 +107,5 @@
>  		printf("ERROR: uuid = NULL\n");
>  		exit(1);
>  	}
> -	printf("uuid = %s\n", uuid);
>  	exit(0);
>  }
> 
> Note that after blkid_put_cache() the uuid string will not be available, you
> have to use it before blkid_put_cache() (or strdup()).
> 
> I don't see any leak. Not a bug from my point of view.

Can you explain why the library needs to hold the underlying block device open even after the call to blkid_get_dev()?  Is it something to do with blkid_cache validity?

SteveD - it does not look like rpc.mountd calls blkid_put_cache() in get_uuid_blkdev() but Karel is saying we need to do that (see comment #5).  Also we need to somehow copy the UUID string here to retain its value or it will go away apparently or we need to rework get_uuid_blkdev() to retain blkid_cache for the lifetime of its use.

Left unexplained still is why libblkid keeps underlying device open (maybe for cache validity or optimization?) or why RHEL6 does not have this problem (maybe a blkid_put_cache was not required prior and/or some bug was fixed in the library?)

Ok it looks intentional get_uuid_blkdev() is not calling blkid_put_cache() because we have a static variable and we're checking for NULL before calling blkid_get_cache().  Unclear where to go from here...

Good question Dave, ... I've checked another part of the library (where it re-validate the cache) and it seems there is a leak in blkid_verify(). It does not call blkid_free_probe() because the probe handler is later re-used, unfortunately if does not care about internally used disk_prober. 

So, blkid_put_cache() will fix the problem for rpc.mountd, but it seems we need to improve libblkid too.

Fixed by util-linux upstream commit: https://github.com/karelzak/util-linux/commit/c4d6d1c54dcd0eff701236d396d88b1fc6251768

I'm going to fix it in RHEL7 and RHEL8 too. Thanks for help and test program, it's pretty old bug (v2.20, year 2011).

Thanks Karel!  I also confirm with a test build that patch in comment #10 fixes the problem.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:1102