Merge ~ahasenack/ubuntu/+source/samba:bionic-samba-memleak-1814532 into ubuntu/+source/samba:ubuntu/bionic-devel

[Andreas Hasenack]

Fix for a memleak.

There is no real test case for this. I tried running smbd under valgrind before and after installing the fixed packages, but I'm not sure if it's robust enough. I added the results to https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1814532/comments/8

Bileto ticket: https://bileto.ubuntu.com/#/ticket/3631

It's all green: gvfs on s390x is a known and already hinted failure.

Bug has the SRU template, but the test case is lacking as explained above.

[Christian Ehrhardt  (paelzer)]

I'm still reviewing, but to make the valgrind data better I'd suggest to install debug packages of the to be debugged program.
When calling it with valgrind add this to the arguments:
  --leak-check=full --track-origins=yes

Post back that info here or on the bug, it might help to feel more convinced of the change.

[Christian Ehrhardt  (paelzer)]

FYI (to read and be aware) https://bugzilla.samba.org/show_bug.cgi?id=13069
I think our build toolchain is new enough to not be affected, but that might be what haunts trusty so you might want to take a look.

[Christian Ehrhardt  (paelzer)]

Better valgrind data would help, and that we don't have a clear testcase makes the SUR harder.
Never the less I think this is small, comprehensive and reviewable as code-only as well.

That I did and it LGTM (the patch as well as the packaging)
+1 for the MP

Bonus for renewed valgrind data and checking bug 13069

review: Approve

[Andreas Hasenack (ahasenack)]

Ok, got something.

First, the full logs:
old samba: http://paste.ubuntu.com/p/CCR4fbFf84/
new samba: http://paste.ubuntu.com/p/Hmw6QGHVqP/

In the old log, we can see the affected vfswrap_getwd function being called and flagged in a few places:

==9522== 36 bytes in 3 blocks are definitely lost in loss record 123 of 498
==9522== at 0x4C31D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9522== by 0x8A8B15F: getcwd (getcwd.c:84)
==9522== by 0x55A452D: vfswrap_getwd (vfs_default.c:2227)

In thw new log file, there is no "vfs" match anywhere.

This is the call I used:
smbclient //localhost/pub -U ubuntu%ubuntu -c "pwd;dir;cd dir1;dir;pwd;cd dir11; pwd; dir; cd /; cd dir2; pwd; dir; cd /"

And pub was created as:
# tree /pub
/pub
├── dir1
│   └── dir11
└── dir2

[pub]
 path = /pub
 read only = no
 guest ok = no

And I had this global option set, to not use a cache and provoke more leaks:
[global]
...
 getwd cache = no

[Andreas Hasenack (ahasenack)]

Tagged and uploaded:

$ git push pkg upload/2%4.7.6+dfsg_ubuntu-0ubuntu2.7
Enumerating objects: 20, done.
Counting objects: 100% (20/20), done.
Delta compression using up to 4 threads
Compressing objects: 100% (13/13), done.
Writing objects: 100% (13/13), 2.03 KiB | 692.00 KiB/s, done.
Total 13 (delta 9), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/samba
 * [new tag] upload/2%4.7.6+dfsg_ubuntu-0ubuntu2.7 -> upload/2%4.7.6+dfsg_ubuntu-0ubuntu2.7

$ dput ubuntu ../samba_4.7.6+dfsg~ubuntu-0ubuntu2.7_source.changes
Checking signature on .changes
gpg: ../samba_4.7.6+dfsg~ubuntu-0ubuntu2.7_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../samba_4.7.6+dfsg~ubuntu-0ubuntu2.7.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading samba_4.7.6+dfsg~ubuntu-0ubuntu2.7.dsc: done.
  Uploading samba_4.7.6+dfsg~ubuntu-0ubuntu2.7.debian.tar.xz: done.
  Uploading samba_4.7.6+dfsg~ubuntu-0ubuntu2.7_source.buildinfo: done.
  Uploading samba_4.7.6+dfsg~ubuntu-0ubuntu2.7_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 2a821f6..9f97d93 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.7) bionic; urgency=medium
+
+  * d/p/memleak-fix-13372.patch: Fix memory leak in vfswrap_getwd().
+    (LP: #1814532)
+
+ -- Andreas Hasenack <[e-mail]>  Mon, 04 Feb 2019 17:37:51 -0200
+
 samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.6) bionic; urgency=medium
 
   * d/p/auth-fail-eexist.diff: smbc_opendir should not return EEXIST with
diff --git a/debian/patches/memleak-fix-13372.patch b/debian/patches/memleak-fix-13372.patch
 new file mode 100644
index 0000000..caf40e6
--- /dev/null
+++ b/debian/patches/memleak-fix-13372.patch
@@ -0,0 +1,44 @@
+From 461a1172ff819692aa0a2dc5ce7fc5379c8a529e Mon Sep 17 00:00:00 2001
+From: Jeremy Allison <[e-mail]>
+Date: Fri, 6 Apr 2018 13:52:52 -0700
+Subject: [PATCH] s3: smbd: Fix memory leak in vfswrap_getwd()
+
+BUG: https://bugzilla.samba.org/show_bug.cgi?id=13372
+
+Signed-off-by: Andrew Walker <[e-mail]>.
+Reviewed-by: Jeremy Allison <[e-mail]>
+Reviewed-by: Ralph Boehme <[e-mail]>
+
+Autobuild-User(master): Jeremy Allison <[e-mail]>
+Autobuild-Date(master): Mon Apr  9 21:48:12 CEST 2018 on sn-devel-144
+
+Origin: https://gitlab.com/samba-team/samba/commit/461a1172ff819692aa0a2dc5ce7fc5379c8a529e
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1814532
+Last-Update: 2019-02-04
+---
+ source3/modules/vfs_default.c | 9 ++++++---
+ 1 file changed, 6 insertions(+), 3 deletions(-)
+
+diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
+index a26bec457ae..a9c87e444fe 100644
+--- a/source3/modules/vfs_default.c
++++ b/source3/modules/vfs_default.c
+@@ -2229,9 +2229,12 @@ static struct smb_filename *vfswrap_getwd(vfs_handle_struct *handle,
+ 				NULL,
+ 				NULL,
+ 				0);
+-	if (smb_fname == NULL) {
+-		SAFE_FREE(result);
+-	}
++	/*
++	 * sys_getwd() *always* returns malloced memory.
++	 * We must free here to avoid leaks:
++	 * BUG:https://bugzilla.samba.org/show_bug.cgi?id=13372
++	 */
++	SAFE_FREE(result);
+ 	return smb_fname;
+ }
+ 
+-- 
+2.18.1
+
diff --git a/debian/patches/series b/debian/patches/series
index 288dfdf..1b2c8ec 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -38,3 +38,4 @@ CVE-2018-16841-1.patch
 CVE-2018-16841-2.patch
 CVE-2018-16851.patch
 auth-fail-eexist.diff
+memleak-fix-13372.patch