rmdir on non-empty samba directory fails silently

[Rolando Gorgs]

[Impact]

The samba client doesn't check the result of rmdir, returning success when the directory is not empty. This results in clients reporting successful directory removal when in fact the directory was not removed.

This bug affects anything that uses the samba client, including file managers, mount, and gvfs.

[Test Case]

# lxc launch ubuntu-daily:bionic sambatest
# lxc exec sambatest bash

Inside container:

# apt update
# apt dist-upgrade -y
# apt install -y samba samba-client
# mkdir -p /tmp/shared/testdir/testsubdir
# chown -R nobody:nogroup /tmp/shared
# printf "%s" "[global]
    workgroup = WORKGROUP
    server role = standalone server
    security = user
    map to guest = Bad User
    guest ok = yes
    guest only = yes
    guest account = nobody
[shared]
    path = /tmp/shared
    writable = yes
    browsable = yes
    guest ok = yes
" | tee "/etc/samba/smb.conf" >/dev/null
# service smbd restart

# CLI_FORCE_INTERACTIVE=yes smbclient -U " "%" " //localhost/shared <<EOF
rmdir testdir
quit
EOF

* Should get NT_STATUS_DIRECTORY_NOT_EMPTY
  - with the bug present one will get no error message, but in both cases
    the delete didn't happen

[Upstream Report and Fix]

https://bugzilla.samba.org/show_bug.cgi?id=13204

https://attachments.samba.org/attachment.cgi?id=14408

[Regression Potential]

If the fix is implemented incorrectly, clients may receive false error messages, and report failure even on success when removing directories.

[Original Description]

When you delete a non-empty directory in Nautilus or Nemo, the Folder-Icon disappears and stays away but when you refresh the current directory or navigate back there it reappears. No error message at all. This bug is very annoying in two ways:

1) When you only have smb access to the share you have to delete all files and directories in sub, subsub and subsubsubdirecories by hand to actually delete a directory.

2) You don't get warned that the delete operation failed. In our case this produced a horrible mess in our shares before we recognized what was going wrong.

You can find many people complaining in various forums. I found bug reports in bugtrackers of Nemo, Nautilus and Gnome but the root seems to be this (upstream fixed) bug in samba:

https://bugzilla.samba.org/show_bug.cgi?id=13204

Please cherry pick the fix into current version of samba.

Thanks a lot for your help!

[Andreas Hasenack (ahasenack)]

Thanks for this detailed bug report. While I gear up to reproduce it, could you just let us know in which ubuntu release and samba version you experienced this? The upstream bug report has fixes for 4.8.x and 4.9.x. Currently only Cosmic 18.10 has 4.8.x, and we don't ship 4.9.x yet.

[Andreas Hasenack (ahasenack)]

From my testing, looks like bionic and cosmic are affected. Xenial returns a proper error:

smb: \> rmdir somedir
NT_STATUS_DIRECTORY_NOT_EMPTY removing remote directory file \somedir

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.8.4+dfsg-2ubuntu2

---------------
samba (2:4.8.4+dfsg-2ubuntu2) cosmic; urgency=high

  [ Karl Stenerud ]
  * d/p/fix-rmdir.patch: Fix to make the samba client library report
    directory-not-empty errors (LP: #1795772)

 -- Andreas Hasenack <email address hidden> Tue, 09 Oct 2018 14:32:16 -0300

[Rolando Gorgs (rolandogorgs)]

Thanks for your work! Progress looks promising, but I run samba 2:4.7.6+dfsg~ubuntu-0ubuntu2.2 on bionic. Would you please push your fix also to this package so that LTS users benefit from a working delete function. Please excuse my impatientness. I'm not familiar with the process of bug fixing at all. Just want to know when the fix reaches my affected system. :-)

[Andreas Hasenack (ahasenack)]

Hi Rolando. I totally understand your eagerness for this fix, and a bionic update will follow. We just needed to make sure that the current development release of Ubuntu had the fix before starting to address the stable releases, specially since today is the Final Freeze day for Ubuntu 18.10. Missing this date would mean an even longer delay to get the fix into the stable releases.

I'll start working on it right away, I know how annoying this bug is, to say the least.

[Rolando Gorgs (rolandogorgs)]

Hello Andreas,
this answer makes me calm me down. I thought for a second bionic has been forgotten. And yes, this bug is indeed very very annoying. I'm surprised that I'm the first one who reports this. Probably most affected users don't even noticed yet that they are constantly leaving a mess in their shares. This bug also produced (minor) financial losses and major disputes and extra work in our office because people used wrong (allegedly deleted) data files for weeks instead of the right ones. You can imagine I'm really looking forward to this bug be fixed. :) So thank you very much for your efforts.

[Andreas Hasenack (ahasenack)]

Uploaded to bionic unapproved, waiting for SRU team's approval.

[Rolando Gorgs (rolandogorgs)]

Any good news? :-)

[Vonschutter (stephan-schutter)]

I am on traveling foot... so no chance to test with latest ubuntu
packages...

On Fri, Oct 26, 2018, 23:25 Rolando Gorgs <email address hidden>
wrote:

> Any good news? :-)
>
> --
> You received this bug notification because you are subscribed to a
> duplicate bug report (1773634).
> https://bugs.launchpad.net/bugs/1795772
>
> Title:
> rmdir on non-empty samba directory fails silently
>
> Status in samba:
> Unknown
> Status in samba package in Ubuntu:
> Fix Released
> Status in samba source package in Bionic:
> In Progress
>
> Bug description:
> [Impact]
>
> The samba client doesn't check the result of rmdir, returning success
> when the directory is not empty. This results in clients reporting
> successful directory removal when in fact the directory was not
> removed.
>
> This bug affects anything that uses the samba client, including file
> managers, mount, and gvfs.
>
> [Test Case]
>
> # lxc launch ubuntu-daily:cosmic sambatest
> # lxc exec sambatest bash
>
> Inside container:
>
> # apt update
> # apt dist-upgrade -y
> # apt install -y samba samba-client
> # mkdir -p /tmp/shared/testdir/testsubdir
> # chown -R nobody:nogroup /tmp/shared
> # printf "%s" "[global]
> workgroup = WORKGROUP
> server role = standalone server
> security = user
> map to guest = Bad User
> guest ok = yes
> guest only = yes
> guest account = nobody
> [shared]
> path = /tmp/shared
> writable = yes
> browsable = yes
> guest ok = yes
> " | tee "/etc/samba/smb.conf" >/dev/null
> # service smbd restart
>
> # CLI_FORCE_INTERACTIVE=yes smbclient -U " "%" " //localhost/shared <<EOF
> rmdir testdir
> quit
> EOF
>
> * Should get NT_STATUS_DIRECTORY_NOT_EMPTY
>
> [Upstream Report and Fix]
>
> https://bugzilla.samba.org/show_bug.cgi?id=13204
>
> https://attachments.samba.org/attachment.cgi?id=14408
>
> [Regression Potential]
>
> If the fix is implemented incorrectly, clients may receive false error
> messages, and report failure even on success when removing
> directories.
>
> [Original Description]
>
> When you delete a non-empty directory in Nautilus or Nemo, the Folder-
> Icon disappears and stays away but when you refresh the current
> directory or navigate back there it reappears. No error message at
> all. This bug is very annoying in two ways:
>
> 1) When you only have smb access to the share you have to delete all
> files and directories in sub, subsub and subsubsubdirecories by hand
> to actually delete a directory.
>
> 2) You don't get warned that the delete operation failed. In our case
> this produced a horrible mess in our shares before we recognized what
> was going wrong.
>
> You can find many people complaining in various forums. I found bug
> reports in bugtrackers of Nemo, Nautilus and Gnome but the root seems
> to be this (upstream fixed) bug in samba:
>
> https://bugzilla.samba.org/show_bug.cgi?id=13204
>
> Please cherry pick the fix into current version of samba.
>
> Thanks a lot for your help!
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/samba/+bu...

[Andreas Hasenack (ahasenack)]

My previous upload was rejected because I didn't prefix the bug number in debian/changelog with a "#". I uploaded again, and it's in the bionic unapproved queue again (see https://launchpad.net/ubuntu/bionic/+queue?queue_state=1&queue_text=samba)

[Brian Murray (brian-murray)]

Hello Rolando, or anyone else affected,

Accepted samba into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.7.6+dfsg~ubuntu-0ubuntu2.3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Rolando Gorgs (rolandogorgs)]

I added proposed repo and installed samba version 2:4.7.6+dfsg~ubuntu-0ubuntu2.3 from proposed, but to be honest I don't see any difference in Nemo.

I tested with Nemo 3.8.6 to delete a non empty directory within a smb share before and after the update and both failed silently. Just to be sure: This problem is on the smb client side, right? Because the samba server I have access to runs some unknown debian version and is not under my control.

[Rolando Gorgs (rolandogorgs)]

I also rebooted my client but the problem persists. I searched Synaptic for "samba" and all samba packages are version 2:4.7.6+dfsg~ubuntu-0ubuntu2.3 now.

[Rolando Gorgs (rolandogorgs)]

Ok, I did further testing and the fix made things even worse!
(I used two - apart from this new samba version - identical VMs to verify the behavior.)

Before the fix I was not able to delete non-empty directories.
With installed fix I'm not able to delete ANY directory - regardless of content!

[Rolando Gorgs (rolandogorgs)]

Thunar (1.6.15) is also affected now. Without the fix Thunar was able to delete empty and non-empty directories. Now it cannot delete ANY directory too. :(

[Andreas Hasenack (ahasenack)]

Yes, it's a client-side fix. I'm taking a look at what is going on in bionic.

[Andreas Hasenack (ahasenack)]

Confirmed not working in bionic, thanks for the verification. Looks like the upstream patch is not enough for the case of samba 4.7.

[Andreas Hasenack (ahasenack)]

4.7.11, latest upstream, still affected.

[Andreas Hasenack (ahasenack)]

Found the issue. I made a silly mistake while applying the patch to the 4.7 code tree.

[Andreas Hasenack (ahasenack)]

I'm confident in the fix for bionic now:
smb: \> rmdir testdir
NT_STATUS_DIRECTORY_NOT_EMPTY removing remote directory file \testdir
smb: \> quit
root@bionic-samba-rmdir:~#

It's in review. Apologies for the mistake.

[Rolando Gorgs (rolandogorgs)]

No problem. Thank you for your time! Will review the new patch when its out. :)

[Andreas Hasenack (ahasenack)]

New package uploaded, waiting for the approval of the SRU team.

[Brian Murray (brian-murray)]

Hello Rolando, or anyone else affected,

Accepted samba into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.7.6+dfsg~ubuntu-0ubuntu2.4 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Rolando Gorgs (rolandogorgs)]

I added proposed repo again and installed samba version 2:4.7.6+dfsg~ubuntu-0ubuntu2.4 from proposed and I'm happy to say, that this version fixed the bug. I tested Nemo, Nautilus and Thunar and all of them are able to delete all kind of directories within Samba shares now. Thanks a lot for your help! Please push this fix to bionic-updates! :-)

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for samba has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package samba - 2:4.7.6+dfsg~ubuntu-0ubuntu2.4

---------------
samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.4) bionic; urgency=medium

  * d/p/fix-rmdir.patch: fix the patch to not apply with offset, which
    previously made it change the wrong, almost identical, function.
    (LP: #1795772)

samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.3) bionic; urgency=medium

  * d/p/fix-rmdir.patch: Fix to make smbclient report directory-not-empty
    errors (LP: #1795772)

 -- Andreas Hasenack <email address hidden> Thu, 08 Nov 2018 16:09:36 -0200