Docset storage read-only check is too restrictive (and perhaps fundamentally flawed?)

[jgottula]

Using the Windows version of Zeal, on Windows 8.1, I attempted to set my docset storage to a directory within a networked drive. This networked drive is served by a Samba server on a Linux system, and the permissions were specifically set up such that my Windows user has always had full access (read, write, everything) to the entire SMB share, with no elevation / administrator privileges required.

Despite the fact that my user can do literally whatever it wants in the networked directory, Zeal has the mistaken impression that the directory isn't writable, and therefore gives me the "Docset storage is read only" message and disables all of my buttons. (Even though if it were to just, like, actually try to write files and directories to that path, it would encounter no issues doing so.)

---

I looked at the source code, and it's clear that DocsetsDialog::m_isStorageReadOnly is being set to true in the DocsetsDialog constructor, due to QFileInfo::isWritable returning false for the docset storage directory.

The QFileInfo::isWritable function effectively just returns QFileInfo::permission(QFile::WriteUser); which is to say, it doesn't bother to check the other write permission bits, i.e. QFile::WriteOwner, QFile::WriteGroup, or QFile::WriteOther.

I attached a debugger to the application, set a breakpoint on that call to QFileInfo::isWritable, and then in the eventual call to QFileSystemEngine::fillPermissions, manipulated the value of argument what so that permission checks would be run for user, owner, group, and other; this allowed me to snoop on what the full set of permissions on the directory were, as determined by Qt. I found them to be 0x7554, i.e. as follows:

• rwx owner
• r-x user
• r-x group
• r-- other

I don't entirely know what to make of the situation; but in any case, there does appear to be some kind of false-positive problem going on with the read-only check. And going so far as to disable buttons in the UI (with no ability to override that) seems a bit harsh.

---

I found this to be potentially relevant / applicable:

http://doc.qt.io/qt-5/qfile.html#platform-specific-issues

File permissions are handled differently on Unix-like systems and Windows. In a non writable directory on Unix-like systems, files cannot be created. This is not always the case on Windows, where, for instance, the 'My Documents' directory usually is not writable, but it is still possible to create files in it.

Also, this (admittedly rather old) post regarding a similar problem (false positives when asking Qt whether a directory is read-only on Windows) might be of interest:

https://stackoverflow.com/questions/1089598

---

Issue #522, which concerned a problem apparently caused by directory read-only-ness, and which also involved a lack of UI feedback regarding the fact that read-only-ness was preventing things from working properly, was addressed by d3c5922, which added the check to (at least in theory) determine whether the docset directory is writable, and then show the "Docset storage is read only" message and disable buttons accordingly.

Issue #946 sounds like another case that may be related in some fashion to mine, where the docset-storage-location-is-readonly code erroneously concluded that the path wasn't writable, when in reality it very likely could have been written to without issue.

[jgottula]

Also, upon hacking my executable with a hex editor such that the isWritable check is skipped and m_isStorageReadOnly is always set false, I can confirm that the application works entirely correctly. I can download, install, use, and remove docsets with no trouble at all.

So this really is a case of the read-only check being too stringent.

[jkozera]

Also, upon hacking my executable with a hex editor such that the isWritable check is skipped

Wow! Thank you for such detailed report, and even making the effort to hack the executable. We really should make Zeal easier to build. :)

I suspect there are multiple possible solutions:

1. Use the workaround from https://stackoverflow.com/a/1094950 — but then I guess it requires careful testing, if it really works in all cases.
2. Show an error message instead of disabling the button, and then allow the user to override the check.
3. Check for writability by actually writing to the location, and then check if the result is present there.

I'd suggest combining 1 + 2, to at least try to fix it, and allow all users to try to work it around, but that's up to @trollixx to decide.

[jgottula]

Wow! Thank you for such detailed report, and even making the effort to hack the executable. We really should make Zeal easier to build. :)

Eh... frankly I already had the disassembler open for the debugging exercise I did, and I do a fair bit of software reverse engineering on a daily basis anyway; so it seemed fastest/easiest to just overwrite a couple of assembly instruction bytes in the .exe rather than go to the trouble of setting up a proper build. (I actually haven't even looked into what would have been required to do a win64 build; was basically just too lazy to bother, heh.)

So not necessarily meant as a comment on the build system being too difficult; more of me just doing what seemed simplest to myself at the time.

[jgottula]

Oh and actually, I just remembered something potentially relevant, which is that the Samba share in question was sort of a hacky thing I set up that was "good enough to work": I never bothered to set up all the complicated NT domain/workgroup user garbage (so that my Windows user would be idmap'd to a particular Linux user and group directly); instead I basically just configured the share the simple way, so that I could log in with a particular arbitrary username and password from the Windows side, and then I used force user = root and force group = root and some other stuff in the Samba config to make it so that I could read/write anything on the Linux side as if I was the superuser. (Not the prettiest setup for sure.)

And quite possibly the way I set it up might mean that Windows thinks I don't have permissions to do things, but that whenever it actually tries to do things, it always works anyway because Samba allows it. Or something like that.

Not entirely sure how all of that permissions stuff over SMB really works, to be honest... Microsoft seemingly made the most complicated system they could come up with for it. But in any case, my setup operates fine for Explorer and virtually all Windows applications, and it's been problem-free for a good few years.

[jgottula]

Here's what the Explorer properties window has to say about the permissions on the network-mounted-drive directory I was attempting to use for Zeal docset storage:

(Note that it says there isn't any inheritance and that the permission entries only apply to "this folder only", but that isn't really the case; the same permission entries are present on all subdirectories as well, but I suppose Samba isn't exposing them as if they're inheritable ACL's or whatever.)

And when I do an Effective Access check, Windows does report that there's no mapping between account names and SID's (which is 100% true):

(The Windows machine is jgvm-win8, the Linux machine is jgpc, and the username of relevance is jgottula on both.)

(I ❤️ NT permissions, especially when it involves SMB shares 🙄)

[trollixx]

What a research! This is definitely a very interesting read for such a seemingly minor issue, thanks!

I think trying to download a docset into a folder, and then failing with an error message should be fine. I wouldn't want to build an overcomplicated logic just to figure out if a folder is writable.

One potential issue I can think of would be docsets installed system wide (covered in #516). There needs to be a way to detect those and not constantly fail trying to update them.

[jkozera]

There needs to be a way to detect those and not constantly fail trying to update them.

Docsets are never auto updated, right? (#307) So if you had a separate section with system-wide docsets, then you could disable updating them unless you enable some settings (by default disabled, or perhaps enabled automatically when write permissions are detected, but then you could always allow overriding it).

[jkozera]

@jgottula Would you be willing to check the behavior of the build from https://ci.appveyor.com/project/zealdocs/zeal/build/job/0vbj9vsqpt409nm8/artifacts?

It includes the changes from jkozera@3e0be42 (and jkozera@1a516c9 but it's less important), which

1. includes the workaround from https://stackoverflow.com/a/1094950 and
2. still allows overriding the check.

So I wonder, if you are still going to get the warning about location being non-writable. If no, then it means the workaround was enough. (But I'd still allow the override in case of other cases where the check doesn't work.)

/cc @trollixx I was actually intending to build this from my own jkozera/zeal repo, as opposed to zealdocs/zeal but noticed too late that I'm pushing to zealdocs/zeal — sorry! It saved me from setting up AppVeyor on my personal repo, though. :P

UPDATE: FWIW, https://stackoverflow.com/a/1094950 doesn't work well on Linux (it always determines the location to be read only, even if it is writable). But if it helps on Windows, maybe it's still worth making it platform-dependent.

[teto]

I haven't investigated much myself but I don't get why having the documentation in a read-only folder should be a problem ? If I just want to consult the docs...