Merge ~ahasenack/ubuntu/+source/nfs-utils:jammy-blkmapd-invalid-free into ubuntu/+source/nfs-utils:ubuntu/jammy-devel

[Andreas Hasenack]

Fix for a core dump in blkmapd. The linked SRU bug has testing instructions.

I haven't heard back from upstream yet, maybe the final commit will be different. I'll evaluate if/when that happens, and submit to debian.

Once LL is open, I'll propose the fix there too, and then proceed with the SRU.

PPA: https://launchpad.net/~ahasenack/+archive/ubuntu/blkmapd-invalid-free

[Bryce Harrington (bryce)]

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-ahasenack-blkmapd-invalid-free/?format=plain)
  nfs-utils @ amd64:
    20.10.22 18:51:48 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
    20.10.22 23:22:57 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
  nfs-utils @ arm64:
    20.10.22 23:30:06 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
  nfs-utils @ armhf:
    20.10.22 23:12:11 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
  nfs-utils @ ppc64el:
    20.10.22 23:22:27 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
    20.10.22 23:25:51 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1
  nfs-utils @ s390x:
    20.10.22 23:18:30 Log 🗒️ ✅ Triggers: nfs-utils/1:2.6.1-1ubuntu1.2~ppa1

It's interesting that this reports a core dump has occurred, yet I'm not spotting a stack trace in any of the comments. Despite that, looks like the discussion went through some thorough debugging to find the root cause of the error.

Looks like the serial memory gets initialized with:

 struct bl_serial {
     int len;
     char *data;
 };

 struct bl_serial *serial = NULL;
        ...
 serial = bldev_read_serial(fd, filepath);

The object itself seems to be created with this:

    static struct bl_serial *bl_create_scsi_string(int len, const char *bytes)
    {
        struct bl_serial *s;

        s = malloc(sizeof(*s) + len);
        if (s) {
                s->data = (char *)&s[1];
                s->len = len;
                memcpy(s->data, bytes, len);
        }
        return s;
    }

Indeed that is malloc'ing the entire bl_serial structure, and not alloc'ing serial->data separately. So yes confirmed this needs only the one free. And this matches to what you've written in the SRU [Impact].

LGTM, +1

Just as a nitpick, I would suggest mentioning in the changelog the text of the error associated with the crash, i.e. "nfs-blkmap.service: Failed with result 'core-dump'." so users can more easily identify what this fixes. Maybe even mention that it occurs with SCSI devices.

Oh, also in the [Where problems could occur] section, in general whenever I have C code and in particular with C code that does memory alloc, I mention memory leaks and segfaults as other types of regressions to watch for. Gotta love pointers. &(<3)

review: Approve

[Andreas Hasenack (ahasenack)]

> It's interesting that this reports a core dump has occurred, yet I'm not spotting a stack trace
> in any of the comments.

The coredump is in this crash file: https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1979885/comments/27

[Andreas Hasenack (ahasenack)]

I updated the "Where problems could occur" section, good call. Nasty pointers. On the changelog, I think I'll leave it. It mentions the service and that it crashes.

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: ahasenack, bryce
Uploaders: ahasenack, bryce
MP auto-approved

review: Approve

[Andreas Hasenack (ahasenack)]

lunar isn't open yet, but it exists, and can be uploaded to (unapproved, though), which I did. I'll now proceed with this SRU.

[Andreas Hasenack (ahasenack)]

Uploaded:

Uploading nfs-utils_2.6.1-1ubuntu1.2.dsc
Uploading nfs-utils_2.6.1-1ubuntu1.2.debian.tar.xz
Uploading nfs-utils_2.6.1-1ubuntu1.2_source.buildinfo
Uploading nfs-utils_2.6.1-1ubuntu1.2_source.changes

diff --git a/debian/changelog b/debian/changelog
index 92e7b2e..49ab19f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+nfs-utils (1:2.6.1-1ubuntu1.2) jammy; urgency=medium
+
+  * d/p/blkmapd-fix-invalid-free.patch: fix blkmapd crash due to invalid
+    free() (LP: #1979885)
+
+ -- Andreas Hasenack <[e-mail]>  Thu, 20 Oct 2022 11:50:13 -0300
+
 nfs-utils (1:2.6.1-1ubuntu1.1) jammy; urgency=medium
 
   * rpc.svcgssd fixes and improvements (LP: #1977745):
diff --git a/debian/patches/blkmapd-fix-invalid-free.patch b/debian/patches/blkmapd-fix-invalid-free.patch
new file mode 100644
index 0000000..bdcdc19
--- /dev/null
+++ b/debian/patches/blkmapd-fix-invalid-free.patch
@@ -0,0 +1,39 @@
+Description: remove invalid free() call
+ The data pointer is not allocated separatedly, it's the struct that needs to
+ be free()ed.
+ Originally reported by lixiaokeng via
+ https://www.spinics.net/lists/linux-nfs/msg87598.html with a slightly different
+ fix proposal.
+Author: Andreas Hasenack <[e-mail]>
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1022185
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1979885
+Forwarded: https://lore.kernel.org/linux-nfs/CANYNYEG=utJ2pe+FtMWh8O+dz63R2wbzOC7ZVrvoqD=[e-mail]/T/#u
+Last-Update: 2022-10-20
+diff --git a/utils/blkmapd/device-discovery.c b/utils/blkmapd/device-discovery.c
+index 2736ac89..b3786a1f 100644
+--- a/utils/blkmapd/device-discovery.c
++++ b/utils/blkmapd/device-discovery.c
+@@ -188,7 +188,6 @@ static void bl_add_disk(char *filepath)
+ 
+ 	if (disk && diskpath) {
+ 		if (serial) {
+-			free(serial->data);
+ 			free(serial);
+ 		}
+ 		return;
+@@ -229,7 +228,6 @@ static void bl_add_disk(char *filepath)
+ 			disk->valid_path = path;
+ 		}
+ 		if (serial) {
+-			free(serial->data);
+ 			free(serial);
+ 		}
+ 	}
+@@ -242,7 +240,6 @@ static void bl_add_disk(char *filepath)
+ 		free(path);
+ 	}
+ 	if (serial) {
+-		free(serial->data);
+ 		free(serial);
+ 	}
+ 	return;
diff --git a/debian/patches/series b/debian/patches/series
index 5626161..8d52ce0 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -8,3 +8,4 @@ svcgssd-fix-use-after-free.patch
 svcgssd-display-principal-if-set.patch
 svcgssd-document-missing-options.patch
 nfs-conf-manpage-missing-svcgssd-options.patch
+blkmapd-fix-invalid-free.patch