search

mate-desktop / caja

Caja, the file manager for the MATE desktop
https://mate-desktop.org/
Other
264 stars 145 forks source link

Fix crash while browsing Windows Network #1600

Closed dougg3 closed 2 years ago

dougg3 commented 2 years ago

This commit fixes two problems that each cause a crash while browsing the Windows Network if a computer on the network has an empty name. See #689 for more detail about how the problem happens. It sounds pretty weird for a computer to have an empty name, but it's definitely possible -- nmbd used to have a bug that caused the autogenerated NetBIOS name to be empty if the computer's hostname was too long.

Both of these changes are basically just bringing in small fixes that have already been in Nautilus for years.

lukefromdc commented 2 years ago

Someone who has access to a Windows machine will need to test this, I don't have one

dougg3 commented 2 years ago

All the testing I did was without Windows -- I just pulled up a few Ubuntu MATE VMs and installed Samba on one of them. The easiest way to make an empty server name show up is to set up Samba with netbios name = "" in /etc/samba/smb.conf.

lukefromdc commented 2 years ago

OK, that lets more folks here work on it than if a Windows machine was needed for a test connection, though in my case I also do everything on bare metal and have never learned to work with VM's. I have never used Samba so not familiar with it.

dougg3 commented 2 years ago

Makes sense -- I will post detailed test instructions here for reproducing the problem.

dougg3 commented 2 years ago

Here are instructions to reproduce the crash. Unfortunately, Windows networking can be extremely finicky and its behavior often depends on what other devices are on your LAN, so it's best to test this with a pair of VMware VMs set up with NAT networking so they're isolated from the rest of your network but can still talk to each other. Note that NAT won't work with VirtualBox, they are kept isolated in VirtualBox. If you're a VirtualBox expert, you may be able to figure out how to do it here.

One VM will be the server, configured to output an empty server name. The other one will be the client that crashes.

Set up the server

The server is now ready to go. Keep it running.

Set up the client

If you apply the fix in this pull request and run the patched version of Caja in the VM, it will no longer crash. The setup for this bug might sound crazy and convoluted, but this situation does happen. I experienced it myself while at work.

lukefromdc commented 2 years ago

I am indeed going to have to leave this for others on the team: this is completely out of my league and with no landline I cannot even download the disc images.

raveit65 commented 2 years ago

Which samba version does old Ubuntu MATE 16.04 use? Looks like 16.04 is EOL. https://news.itsfoss.com/ubuntu-16-04-reaches-end-of-life/ I am using modern fedora but i will try reproduce the issue with an EOL version of fedora.

rbuj commented 2 years ago

I'm not sure the NetBIOS name can be set to "". In such case, the NetBIOS name comes from the hostname, which cannot be null. Anyway, it looks like a user mistake or, a name resolution error. Did you fill a bug report on samba or gvfsd? https://www.oreilly.com/library/view/using-samba/1565924495/ch01s03.html#ch01-SECT-3.2

dougg3 commented 2 years ago

Yeah, 16.04 is pretty old. It's using Samba 4.3.11. FWIW, an 18.04 VM as the server (Samba 4.7.6) also reproduces the problem.

I can't reproduce the problem with an Ubuntu MATE 20.04 VM as the server - it seems with newer servers, the "WORKGROUP" icon never shows up inside of Windows Network on clients. I'm not sure why.

However, the underlying problem isn't necessarily just a Samba thing though -- this is simply the only way I know of to cause a NULL details->display_name to happen.

dougg3 commented 2 years ago

If you look at the link I posted in the original message, there was a problem on older versions of Samba that caused the NetBIOS name to be empty if the computer's hostname was too long. https://bugzilla.samba.org/show_bug.cgi?id=10896

That bug is how I originally ran into this problem a few years ago.

rbuj commented 2 years ago

There is no segfault on Fedora 34. Therefore, it isn't reproducible anymore. The samba bug about the long hostnames was fixed in 2014.

Screenshot at 2022-01-31 09-26-12

[robert@localhost ~]$ sudo cat /etc/samba/smb.conf
# See smb.conf.example for a more detailed config file or
# read the smb.conf manpage.
# Run 'testparm' to verify the config is correct after
# you modified it.
#
# Note:
# SMB1 is disabled by default. This means clients without support for SMB2 or
# SMB3 are no longer able to connect to smbd (by default).

[global]
    workgroup = SAMBA
    security = user

    passdb backend = tdbsam

    netbios name = ""

    printing = cups
    printcap name = cups
    load printers = yes
    cups options = raw

[homes]
    comment = Home Directories
    valid users = %S, %D%w%S
    browseable = No
    read only = No
    inherit acls = Yes

[printers]
    comment = All Printers
    path = /var/tmp
    printable = Yes
    create mask = 0600
    browseable = No

[print$]
    comment = Printer Drivers
    path = /var/lib/samba/drivers
    write list = @printadmin root
    force group = @printadmin
    create mask = 0664
    directory mask = 0775

[share]
        comment = My Share
        path = /home/robert/share
        writeable = yes
        browseable = yes
        public = yes
        create mask = 0644
        directory mask = 0755
        write list = user

The name can be resolved on the local server, but it cannot be resolved on the client. From my point of view, setting an empty NetBIOS name is a user mistake. What's more, no double quotes are used to define the netbios name in smb.conf.

dougg3 commented 2 years ago

I agree. Yes, it's a weird setup, and it was probably a bigger problem 5 years ago than it is now, but it's still conceivable that I could plug my computer into an old Samba network and Caja would crash. I'm sorry that the test setup is so convoluted, but Samba networking in general is complicated. If it helps at all, I have attached videos showing Caja's behavior before and after this fix.

Before: https://user-images.githubusercontent.com/1565758/152090612-5322fd81-5208-4235-bd64-bed773eef5ed.mp4

After: https://user-images.githubusercontent.com/1565758/152090618-9062c73f-d526-4717-b40e-bb8e59f8596e.mp4

dougg3 commented 2 years ago

I'm not really sure what else to say or do at this point. If I'm on the same Windows network as someone who has a broken Samba server with an empty name, Caja will crash when I browse the workgroup because the name ends up being NULL instead of an empty string. In my opinion, crashes are bugs that should be fixed. I've provided detailed instructions for how to simulate such a broken Windows network, and a video of the crash in action.

I can understand concerns about introducing regressions, but the change I'm proposing is essentially just guarding against NULL pointers in two places.

dougg3 commented 2 years ago

Closing because the crash that this pull request solves is no longer reproducible on modern Samba networks (see discussion in #689).