Ubuntu
samba package

Merge ~sergiodj/ubuntu/+source/samba:samba-v1-file-as-directory-bug1872476-v2 into ubuntu/+source/samba:ubuntu/focal-devel

Git
lp:~sergiodj/ubuntu/+source/samba
samba-v1-file-as-directory-bug1872476-v2
Merge into ubuntu/focal-devel

Proposed by Sergio Durigan Junior on 2020-05-04

Status:

Merged

Approved by:

Rafael David Tinoco on 2020-05-05

Approved revision:

2f4206c0fae5109c07f1b86ce05a8c3b5e9c4243

Merged at revision:

2f4206c0fae5109c07f1b86ce05a8c3b5e9c4243

Proposed branch:

~sergiodj/ubuntu/+source/samba:samba-v1-file-as-directory-bug1872476-v2

Merge into:

ubuntu/+source/samba:ubuntu/focal-devel

Diff against target:

120 lines (+98/-0)

3 files modified

debian/changelog (+9/-0)
debian/patches/0023-libsmb-Don-t-try-to-find-posix-stat-info-in-SMBC_get.patch (+88/-0)
debian/patches/series (+1/-0)

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Rafael David Tinoco (community)	2020-05-04	Approve on 2020-05-05
Canonical Server	2020-05-05	Pending
Canonical Server Core Reviewers	2020-05-04	Pending
Review via email: mp+383358@code.staging.launchpad.net

Description of the change

The current samba package on focal suffers from a bug that manifests itselft when the SMB1 (NT1) protocol is being used. In this scenario, the user will see regular files being presented as directories when using GIO/gvfs to browse the share (e.g., nautilus, or via the "gio" command). Even though the SMB1 protocol is old, it is still used by some old NAS devices and therefore this issue can impact a non-trivial amount of users.

The fix, which had to be cherry-picked from upstream, is to revert a specific libsmb code that attempts to obtain some POSIX stat(2) information when running on UNIX-like environments. The upstream commit can be found here:

https://gitlab.com/samba-team/devel/samba/-/commit/39c910fd9cba3caf7414274b678b9eee33d7e20b

In order to reproduce the bug, one can do:

$ lxc launch ubuntu-daily:focal samba-bug1872476
$ lxc shell samba-bug1872476
# apt install samba smbclient gvfs gvfs-backends dbus-x11
# sed -i 's/^\[global\]/[global]\n client min protocol = NT1\n client max protocol = NT1\n server min protocol = NT1\n server max protocol = NT1/g' /etc/samba/smb.conf
# cat >> /etc/samba/smb.conf << __EOF__
[testshare]
  path = /testshare
  readonly = no
  browsable = yes
  guest ok = yes
__EOF__
# mkdir /testshare
# cat > /testshare/123.txt << __EOF__
Hi, this is a test for samba bug 1872476.
This is a regular file, and not a directory.
__EOF__
# ufw allow samba
# eval $(dbus-launch --sh-syntax)
# gio mount -a smb://127.0.0.1/testshare
# gio info smb://127.0.0.1/testshare/123.txt | grep '^type:'
type: directory

As can be seen above, the regular file '123.txt' is mistakenly listed as a directory.

There's a PPA with a proposed fix here:

https://launchpad.net/~sergiodj/+archive/ubuntu/samba-bug1872476-v2

When trying it out, it is important to umount the share and remount it with the new package:

# gio mount -u smb://127.0.0.1/testshare
# add-apt-repository ppa:sergiodj/samba-bug1872476-v2
# apt update && apt upgrade
# gio mount -a smb://127.0.0.1/testshare
# gio info smb://127.0.0.1/testshare/123.txt | grep '^type:'
type: regular

autopkg is still happy:

autopkgtest [14:38:14]: test smbclient-share-access: -----------------------]
autopkgtest [14:38:14]: test smbclient-share-access: - - - - - - - - - - results - - - - - - - - - -
smbclient-share-access PASS
autopkgtest [14:38:15]: @@@@@@@@@@@@@@@@@@@@ summary
cifs-share-access PASS
python-smoke PASS
smbclient-anonymous-share-list PASS
smbclient-authenticated-share-list PASS
smbclient-share-access PASS

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

I'll review this one.

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

# checklist for fixes
----------------------------
[x] changelog entry correct:
     - do not mention changes to d/p/series file in changelog
[.] targeted to correct codename
[.] version number is correct
[.] update-maintainer has been run before
----
[.] changes forwarded upstream/debian (if appropriate)
[.] patches match what was proposed upstream
----
[x] patches correctly included in debian/patches/series?
[.] patches have correct DEP3 metadata
----
[-] relying on PPA only for build check ?
[-] if relying on PPA, did it install correctly ?
----
[.] building it locally ?
[.] if building locally, was source build good ?
[.] if building locally, was binary build good ?
----
[-] was autopkgtest tested ?
----
[.] is this a SRU ?
[x] if a SRU, does the public bug have a template ?
     - please add a SRU template to the public bug
       (https://wiki.ubuntu.com/StableReleaseUpdates)
[-] is this a bundle of fixes ?
[.] is this a single fix ?
----
[.] if single fix, was testcase provided ?
[-] if single fix, and testcase provided, could I reproduce it ?
[-] if single fix, and testcase provided, did it work ?
----------------------------
[.] = ok
[x] = not ok
[?] = question
[!] = note
[-] = n/a
----------------------------
# comments:

- Could you please fix the changelog and add the SRU template to the public
bug ? After you do that, I'll follow your testcase from the SRU (usually
we don't put test cases in the merge proposal, it all goes to the public bug
in the SRU template format).

This happens because first I'll verify your testcase, and, instead of seeking
for it I know where to find. After I have verified and sponsored your patches,
by uploading your source package, the SRU team - since this case is not a fix
to a -devel release - will verify it again AND allow the upload into the
-proposed archive.

It makes it all easier to have this format (thats why we enforce it).

It was a very nice work overall! Congrats!

# checklist for fixes
----------------------------
 [x] changelog entry correct:
     - do not mention changes to d/p/series file in changelog
 [.] targeted to correct codename
 [.] version number is correct
 [.] update-maintainer has been run before
 ----
 [.] changes forwarded upstream/debian (if appropriate)
 [.] patches match what was proposed upstream
 ----
 [x] patches correctly included in debian/patches/series?
 [.] patches have correct DEP3 metadata
 ----
 [-] relying on PPA only for build check ?
 [-] if relying on PPA, did it install correctly ?
 ----
 [.] building it locally ?
 [.] if building locally, was source build good ?
 [.] if building locally, was binary build good ?
 ----
 [-] was autopkgtest tested ?
 ----
 [.] is this a SRU ?
 [x] if a SRU, does the public bug have a template ?
     - please add a SRU template to the public bug
       (https://wiki.ubuntu.com/StableReleaseUpdates)
 [-] is this a bundle of fixes ?
 [.] is this a single fix ?
 ----
 [.] if single fix, was testcase provided ?
 [-] if single fix, and testcase provided, could I reproduce it ?
 [-] if single fix, and testcase provided, did it work ?
----------------------------
 [.] = ok
 [x] = not ok
 [?] = question
 [!] = note
 [-] = n/a
----------------------------
# comments:

It makes it all easier to have this format (thats why we enforce it).

It was a very nice work overall! Congrats!

review: Needs Fixing

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

I just did a small SRU template at:

https://bugs.launchpad.net/ubuntu/+source/monitoring-plugins/+bug/1875517

you can use, together with the SRU template wiki page, to make yours.

Revision history for this message

Andreas Hasenack (ahasenack) wrote on 2020-05-05:

Groovy is also affected and will need a fix before this SRU can be accepted. I'll bump the samba version in groovy: we will have to go ahead of debian there.

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-05-05:

On Tuesday, May 05 2020, Rafael David Tinoco wrote:

Thanks for the review!

> # checklist for fixes
> ----------------------------
> [x] changelog entry correct:
> - do not mention changes to d/p/series file in changelog

Done.

> [.] targeted to correct codename
> [.] version number is correct
> [.] update-maintainer has been run before
> ----
> [.] changes forwarded upstream/debian (if appropriate)
> [.] patches match what was proposed upstream
> ----
> [x] patches correctly included in debian/patches/series?

I don't understand why this was marked as not ok. Is it because of the
changelog entry?

> [.] patches have correct DEP3 metadata
> ----
> [-] relying on PPA only for build check ?
> [-] if relying on PPA, did it install correctly ?
> ----
> [.] building it locally ?
> [.] if building locally, was source build good ?
> [.] if building locally, was binary build good ?
> ----
> [-] was autopkgtest tested ?
> ----
> [.] is this a SRU ?
> [x] if a SRU, does the public bug have a template ?
> - please add a SRU template to the public bug
> (https://wiki.ubuntu.com/StableReleaseUpdates)

I'm creating the habit of filing the MP first, and then add the SRU
template just after it was reviewed. I will make sure to do both the MP
and the SRU template at the same time from now on.

> [-] is this a bundle of fixes ?
> [.] is this a single fix ?
> ----
> [.] if single fix, was testcase provided ?
> [-] if single fix, and testcase provided, could I reproduce it ?
> [-] if single fix, and testcase provided, did it work ?
> ----------------------------
> [.] = ok
> [x] = not ok
> [?] = question
> [!] = note
> [-] = n/a
> ----------------------------
> # comments:
>
> - Could you please fix the changelog and add the SRU template to the public
> bug ? After you do that, I'll follow your testcase from the SRU (usually
> we don't put test cases in the merge proposal, it all goes to the public bug
> in the SRU template format).

Yep!

> This happens because first I'll verify your testcase, and, instead of seeking
> for it I know where to find. After I have verified and sponsored your patches,
> by uploading your source package, the SRU team - since this case is not a fix
> to a -devel release - will verify it again AND allow the upload into the
> -proposed archive.
>
> It makes it all easier to have this format (thats why we enforce it).

Makes sense.

> It was a very nice work overall! Congrats!

Thanks!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

On Tuesday, May 05 2020, Rafael David Tinoco wrote:

Thanks for the review!

> # checklist for fixes
> ----------------------------
>  [x] changelog entry correct:
>      - do not mention changes to d/p/series file in changelog

Done.

>  [.] targeted to correct codename
>  [.] version number is correct
>  [.] update-maintainer has been run before
>  ----
>  [.] changes forwarded upstream/debian (if appropriate)
>  [.] patches match what was proposed upstream
>  ----
>  [x] patches correctly included in debian/patches/series?

I don't understand why this was marked as not ok.  Is it because of the
changelog entry?

>  [.] patches have correct DEP3 metadata
>  ----
>  [-] relying on PPA only for build check ?
>  [-] if relying on PPA, did it install correctly ?
>  ----
>  [.] building it locally ?
>  [.] if building locally, was source build good ?
>  [.] if building locally, was binary build good ?
>  ----
>  [-] was autopkgtest tested ?
>  ----
>  [.] is this a SRU ?
>  [x] if a SRU, does the public bug have a template ?
>      - please add a SRU template to the public bug
>        (https://wiki.ubuntu.com/StableReleaseUpdates)

I'm creating the habit of filing the MP first, and then add the SRU
template just after it was reviewed.  I will make sure to do both the MP
and the SRU template at the same time from now on.

>  [-] is this a bundle of fixes ?
>  [.] is this a single fix ?
>  ----
>  [.] if single fix, was testcase provided ?
>  [-] if single fix, and testcase provided, could I reproduce it ?
>  [-] if single fix, and testcase provided, did it work ?
> ----------------------------
>  [.] = ok
>  [x] = not ok
>  [?] = question
>  [!] = note
>  [-] = n/a
> ----------------------------
> # comments:
>
> - Could you please fix the changelog and add the SRU template to the public
> bug ? After you do that, I'll follow your testcase from the SRU (usually
> we don't put test cases in the merge proposal, it all goes to the public bug
> in the SRU template format).

Yep!

Makes sense.

> It was a very nice work overall! Congrats!

Thanks!

-- 
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0  EB2F 106D A1C8 C3CB BF14

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-05-05:

On Tuesday, May 05 2020, Andreas Hasenack wrote:

> Groovy is also affected and will need a fix before this SRU can be accepted. I'll bump the samba version in groovy: we will have to go ahead of debian there.

Should I work on backporting the patch to groovy as well?

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

Revision history for this message

Andreas Hasenack (ahasenack) wrote on 2020-05-05:

No, in groovy I'll prefer to go with the latest upstream 4.12.x. This will need that d/README.source dance to make sure us and debian get the same dfsg orig tarball.

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-05-05:

Rafael, I force-pushed my branch with the changelog changes, and also wrote the SRU template. Let me know what you think :-). Thanks!

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

>> [x] patches correctly included in debian/patches/series?
>
> I don't understand why this was marked as not ok. Is it because of the
changelog entry?
>

I should have changed this to [.] after moving the changelog entry to the top!

Thanks Sergio. Let me check test case and upload, will post it here.

Revision history for this message

Sergio Durigan Junior (sergiodj) wrote on 2020-05-05:

On Tuesday, May 05 2020, Rafael David Tinoco wrote:

>>> [x] patches correctly included in debian/patches/series?
>>
>> I don't understand why this was marked as not ok. Is it because of the
> changelog entry?
>>
>
> I should have changed this to [.] after moving the changelog entry to the top!

Aha! :-)

> Thanks Sergio. Let me check test case and upload, will post it here.

Thank you, Rafael!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

----
[.] if single fix, was testcase provided ?
[.] if single fix, and testcase provided, could I reproduce it ?
[.] if single fix, and testcase provided, did it work ?
----------------------------
[.] = ok
[x] = not ok
[?] = question
[!] = note
[-] = n/a
----------------------------

All good! Thanks for the fixes. Uploading it now...

review: Approve

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2020-05-05:

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git ubuntu tag --upload

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git describe --tags
upload/2%4.11.6+dfsg-0ubuntu1.2

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ git push pkg upload/2%4.11.6+dfsg-0ubuntu1.2
Enumerating objects: 16, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 8 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 2.92 KiB | 124.00 KiB/s, done.
Total 11 (delta 7), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/samba
* [new tag] upload/2%4.11.6+dfsg-0ubuntu1.2 -> upload/2%4.11.6+dfsg-0ubuntu1.2

-----

(c)rafaeldtinoco@focal:~/.../sources/ubuntu/samba$ dput ubuntu ../samba_4.11.6+dfsg-0ubuntu1.2_source.changes
Checking signature on .changes
gpg: ../samba_4.11.6+dfsg-0ubuntu1.2_source.changes: Valid signature from A93E0E0AD83C0D0F
Checking signature on .dsc
gpg: ../samba_4.11.6+dfsg-0ubuntu1.2.dsc: Valid signature from A93E0E0AD83C0D0F
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading samba_4.11.6+dfsg-0ubuntu1.2.dsc: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2.debian.tar.xz: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2_source.buildinfo: done.
  Uploading samba_4.11.6+dfsg-0ubuntu1.2_source.changes: done.
Successfully uploaded packages.