Strange processing of "$" in NetworkBrowser

[prl]

In AutoMount.doCheckMountPoint(), and doing an 'enigma2' or 'old_enigma2' style mount, the share name (the name of the share being mounted) is processed as:

Code: Select all

					tmpsharedir = data['sharedir'].replace(" ", "\\ ")
					if tmpsharedir[-1:] == "$":
						tmpdir = tmpsharedir.replace("$", "\\$")
						tmpsharedir = tmpdir

tmpsharedir is then pasted into the mount command string, which will then be executed by a shell. The replacement of spaces by "\\ " is simple enough, but the replacement of "$" (shell variable reference) is just strange: it only replaces "$" in the string if the string ends in a "$", but then it replaces "$" everywhere in the string.

I'm asking because it appears that CIFS share names that end in "$" are hidden shares, and I'm wondering if that's why only the last character is being checked. Funnily enough, if the "$" is the last character of an argument (i.e. end of line or followed by whitespace), it doesn't need to be escaped:

Code: Select all

root@beyonwizv2:~# a=yyy
root@beyonwizv2:~# echo xxx$ zzz
xxx$ zzz
root@beyonwizv2:~# echo xxx$a zzz
xxxyyy zzz
root@beyonwizv2:~# 

As well as the odd logic, this is also a clumsy (and incomplete) way of escaping shell special characters (if that's what's intended). The correct way to do that is with
arg = "'" + arg.replace("'", "'\\''") + "'".

[peteru]

Yes, SMB shares with trailing $ are special. They should probably not be offered in the UI and thus should not appear in the config files and therefore should not need special handling (besides being filtered out in the first place).

As far as share names with space in their names - that's very likely to result in issues. Rather than trying to quote those strings and hope for the best, it may be better to detect shares with space in the path and filter them out of the list. For bonus points, you could show a warning to the user that the server has shares with spaces in the name, list the offending names and then remove them from the list. Doing it silently is fine though.

Basically take the "affordances" approach to UI design. Offer the user the things you want them to do and prevent problematic situations from arising by not letting the user encounter the problematic configurations in the first place.

[prl]

Yes, SMB shares with trailing $ are special. They should probably not be offered in the UI and thus should not appear in the config files

The code still needs to consider that they may have been entered by the user either through the UI or by directly editing a config file.

[peteru]

Yes, SMB shares with trailing $ are special. They should probably not be offered in the UI and thus should not appear in the config files

The code still needs to consider that they may have been entered by the user either through the UI or by directly editing a config file.

For the first case, the UI should ideally not allow the entry of invalid names. For the second case, all bets are off. Once the user starts editing system files by hand, it is reasonable to assume that they know their way around and are quite willing to assume all risks associated with going behind the scenes.

There is a balancing act here, between catering to every conceivable possibility and keeping the complexity of the code and the user interface as low as possible.

Realistically, I can't see the need for the user interface to support anything more than autofs mounts with a character set limited to [a-zA-Z0-9_] - even if in practice a lot more flexibility is possible. If the UI completely ignores (and preserves) any system configuration entries that don't conform to this pattern, things should be just fine.

[prl]

The name/path at issue is the exported name of the share, not the local share name. At least for CIFS shares (haven't tested NFS shares yet), spaces in the exported share name currently work for Enigma2 and autofs mounts (they would almost certainly work for Enigma2 old format, but I haven't tested that, because it will probably go away). They don't work for fstab mounts. Spaces could be allowed in exported share names by properly escaping spaces when /etc/fstab is modified by NetworkBrowser. I suspect that most other non-alphanumerics could be made to work, too.

Should the Beyonwiz firmware be restricting what names users create for file system exports from their PCs or NASes? Or even from their Beyonwizes: CIFS exports from Beyonwizes can also be created to contain spaces.

There also seems to be little reason to make the character set in the local share names as restrictive as it is.

[prl]

The rules for CIFS file service share names are:

• A share name must be a valid DNS name.
• Share names must start with a letter or number, and can contain only letters, numbers, and the dash (-) character.
• Every dash (-) character must be immediately preceded and followed by a letter or number; consecutive dashes are not permitted in share names.
• All letters in a share name must be lowercase.
• Share names must be from 3 through 63 characters long.

There don't seem to be any restrictions on the names of NFS exports other than them being valid Unix pathnames.

In both cases, escaping some characters is needed in files like /etc/fstab and /etc/exports.