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

Proposed by Andreas Hasenack on 2019-02-04
Status: Merged
Approved by: Christian Ehrhardt  on 2019-02-05
Approved revision: 444cd5f91791b08365d84fea9ccb952a47bc6ed6
Merge reported by: Andreas Hasenack
Merged at revision: 444cd5f91791b08365d84fea9ccb952a47bc6ed6
Proposed branch: ~ahasenack/ubuntu/+source/samba:bionic-samba-memleak-1814532
Merge into: ubuntu/+source/samba:ubuntu/bionic-devel
Diff against target: 74 lines (+52/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/memleak-fix-13372.patch (+44/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  2019-02-04 Approve on 2019-02-05
Canonical Server Team 2019-02-04 Pending
Review via email: mp+362700@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 2a821f6..9f97d93 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.7) bionic; urgency=medium
7+
8+ * d/p/memleak-fix-13372.patch: Fix memory leak in vfswrap_getwd().
9+ (LP: #1814532)
10+
11+ -- Andreas Hasenack <andreas@canonical.com> Mon, 04 Feb 2019 17:37:51 -0200
12+
13 samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.6) bionic; urgency=medium
14
15 * d/p/auth-fail-eexist.diff: smbc_opendir should not return EEXIST with
16diff --git a/debian/patches/memleak-fix-13372.patch b/debian/patches/memleak-fix-13372.patch
17new file mode 100644
18index 0000000..caf40e6
19--- /dev/null
20+++ b/debian/patches/memleak-fix-13372.patch
21@@ -0,0 +1,44 @@
22+From 461a1172ff819692aa0a2dc5ce7fc5379c8a529e Mon Sep 17 00:00:00 2001
23+From: Jeremy Allison <jra@samba.org>
24+Date: Fri, 6 Apr 2018 13:52:52 -0700
25+Subject: [PATCH] s3: smbd: Fix memory leak in vfswrap_getwd()
26+
27+BUG: https://bugzilla.samba.org/show_bug.cgi?id=13372
28+
29+Signed-off-by: Andrew Walker <awalker@ixsystems.com>.
30+Reviewed-by: Jeremy Allison <jra@samba.org>
31+Reviewed-by: Ralph Boehme <slow@samba.org>
32+
33+Autobuild-User(master): Jeremy Allison <jra@samba.org>
34+Autobuild-Date(master): Mon Apr 9 21:48:12 CEST 2018 on sn-devel-144
35+
36+Origin: https://gitlab.com/samba-team/samba/commit/461a1172ff819692aa0a2dc5ce7fc5379c8a529e
37+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1814532
38+Last-Update: 2019-02-04
39+---
40+ source3/modules/vfs_default.c | 9 ++++++---
41+ 1 file changed, 6 insertions(+), 3 deletions(-)
42+
43+diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
44+index a26bec457ae..a9c87e444fe 100644
45+--- a/source3/modules/vfs_default.c
46++++ b/source3/modules/vfs_default.c
47+@@ -2229,9 +2229,12 @@ static struct smb_filename *vfswrap_getwd(vfs_handle_struct *handle,
48+ NULL,
49+ NULL,
50+ 0);
51+- if (smb_fname == NULL) {
52+- SAFE_FREE(result);
53+- }
54++ /*
55++ * sys_getwd() *always* returns malloced memory.
56++ * We must free here to avoid leaks:
57++ * BUG:https://bugzilla.samba.org/show_bug.cgi?id=13372
58++ */
59++ SAFE_FREE(result);
60+ return smb_fname;
61+ }
62+
63+--
64+2.18.1
65+
66diff --git a/debian/patches/series b/debian/patches/series
67index 288dfdf..1b2c8ec 100644
68--- a/debian/patches/series
69+++ b/debian/patches/series
70@@ -38,3 +38,4 @@ CVE-2018-16841-1.patch
71 CVE-2018-16841-2.patch
72 CVE-2018-16851.patch
73 auth-fail-eexist.diff
74+memleak-fix-13372.patch

Subscribers

People subscribed via source and target branches