There appears to be an issue with libsmbclient 4.7 that returns nonsensical EEXIST error code when a user has not authenticated themselves to access password-protected shares. This patch attempts to work around the issue by treating EEXIST as another case of "invalid login credentials". The workaround tries to detect broken versions of libsmbclient and enables itself only when such a version is found.
See https://bugzilla.samba.org/show_bug.cgi?id=13050 for upstream bug report.
BUG: 385708
| Lint Skipped |
| Unit Tests Skipped |
The upstream bug that we are working around here (which is still unscreened after a month) is https://bugzilla.samba.org/show_bug.cgi?id=13050. I suspect we'll be left with this issue for quite some time, and this is a pretty bad regression for users of rolling release distros. I think we should do something, if possible. Can you make two changes?
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 474 | I meant, mention the upstream bug report here, in the comment in the actual source file. :) | |
Oh, of course, call it the "Friday evening attention level". Link to upstream report is not a part of the comment in the source.
So am I correct that this patch does the following:
If so, is there any other way to check whether the server address is valid besides getting back EEXIST?
This is pretty much the case. One way how to get the login dialog without the patch is to manually enter a valid path like this: "smb://SERVER/valid_share/valid_directory". In this case libsmbclient returns EPERM instead of EEXIST if the requested directory is password-protected. I believe I descried this in a bit more detail in the Samba bug report.
Ifdefing around this won't do much good because applications should not require a rebuild against updated libsmbclient. Peeking through the header file I discovered const char * smbc_version(void). I'll see if I can use this to restrict this hack only to the troublesome libsmbclient versions. Stay tuned.
Detect versions of libsmbclient libraries that are considered broken and apply the workaround only for those.
| smb/kio_smb.cpp | ||
|---|---|---|
| 65–67 | It kind of violates my "change only as much as is needed" but the whole kio-smb source could probably use thorough reformatting anyway. Will do. | |
| smb/kio_smb_browse.cpp | ||
| 521 | Well, this method actually applies the workaround. The m_enableEEXISTWorkaround is set to true when the an affected libsmbclient library is detected. | |
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 521 | Don't we set m_enableEEXISTWorkaround in the constructor? | |
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 521 | Yes. The idea is this: When the slave loads, it checks what libsmbclient lib is available and sets the m_enableEEXISTWorkaround if a broken version is detected. Then iff the return code from smbc_opendir() is EEXIST and the m_enableEEXISTWorkaround is true EEXIST is treated as EACCES or EPERM. This should hopefully prevent any weird behavior when EEXIST might be a valid return code. Do you still think that the method that does this "apply the workaround" logic could use a better name? | |
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 521 | I see, basically checking and applying the workaround are the same thing in this case. | |
| smb/kio_smb.cpp | ||
|---|---|---|
| 49–59 | I like this approach. But I would consider every version >= 4.7.0 as broken. You don't know yet whether 4.7.1 will fix it. If 4.7.1 doesn't fix it, we need to patch this again or it would reintroduce the regression. So I would consider everything broken till the release comes out and then go for something like: if (version >= QVersionNumber(4,7,0) && version < QVersionNumber(x, y, z)) { broken()};and as you can see in this suggestion: have a look at QVersionNumber. It should make this code easier. | |
Do you have commit access? If not, I'll be happy to commit this once some more of the folks who have made comments have also signed off on a final version.
No I don't. elvisangelaccio already commited a few patches on my behalf, can you please make sure you use the same e-mail address as before to commit this? Thanks.
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 521 | While it does no harm, you do not need 'const' for plain types, such as 'int'. | |
| smb/kio_smb_browse.cpp | ||
|---|---|---|
| 521 | Don't worry, I'm well aware. I however like to use consts wherever possible to make it clear that changing a value of such variable makes no logical sense: It's a way of telling "You shouldn't ever have to change this and if you do, perhaps you should think again about what you're trying to do." | |
What is the status on upstreaming this? Is there anything else you need from me to finish this off?
Michal, I don't know how familiar you are with the Samba codebase, but it looks like another user has a proposal for fixing the issue in https://bugzilla.samba.org/show_bug.cgi?id=13050. Are you able to test that and submit a patch to them?
IMHO we should commit this though. It might be a long time before any such patch is accepted and makes into a released version.
Would this fix be eligible for 'Applications/17.08' branch? If yes, please backport/commit in the next 48 hours.