smb2: fix timeout/busyloop with some SMBv1 servers

Question: What timeout value should we use to detect that a server is responding?

changed milestone to %4.0

Question: What timeout value should we use to detect that a server is responding?

On tear-down, that's easy: 0 seconds. The TCP/IP stack will do the clean up asynchronously however fit, as with HTTP, FTP, etc.

That's kind of the whole point of vlc_poll_i11e

HTTP, FTP, etc.

With RTSP being the notable exception, where you actually need to explicitly tear down so the server stops sending UDP. And if there is no timely response, you are screwed.

But AFAIK, SMB does not have that problem. The payload go over TCP, not UDP.

added MRStatus::Reviewable label

added MRStatus::InReview label and removed MRStatus::Reviewable label

added 8 commits

• d56dab09 - access: cache: don't call free_cb from entry_Delete
• 7e45dbf4 - smb2: don't use sys->error_status while opening
• e2ee8583 - smb2: rework error handling
• 51651ed8 - smb2: destroy the context in case of error
• 0fa55fb9 - smb2: remove teardown handling
• 25b986cb - smb2: always use smb2 timeout
• a346805b - smb2: timeout on first connection
• d67f34bb - access: cache: interrupt when the cache is destroyed

Compare with previous version

Lot of rewriting to remove the teardown because it was hiding a serious issue.

mentioned in merge request !1606 (closed)

It seems that you could return -err directly...

What do you mean ? smb2_lseek return 0 or a negative errno code.

Here, VLC_SMB2_SET_ERROR is kind of useless, but it display an error log, that's why I kept it.

I mean that EGENERIC is not a decent error code, especially not when there is a decent error code next.

Ah yes, I forgot the recent changes, no more conflicts between errno and vlc error codes.

Not sure what you base this on. If the device is in deep sleep state, the time between the TCP SYN/ACK and useful data may well be several seconds, or the time to the SYN/ACK for that matter.

I could increase to 2 seconds, but in the worst case scenario, the user will always have to wait 2 seconds when parsing or player a file, unless !1399 (closed) is merged. In that case, the extra wait happen only once (per vlc instance).

2 seconds is not sufficient. VLC uses 5 seconds normally and even that's occasionally insufficient.

You can't break slow but correct/up-to-date implementations to support broken/obsolete ones, IMO.

For the record, the samba project use a delay of 2seconds before trying the SMBv1 port but they don't kill the SMBv2 connection attempt like we must do (because smb2 and dsm are different modules).

I already made a mistake in the past, half breaking SMBv1 for android users, we got ton of complaints. People are not ready to get rid of this shitty and dangerous protocol (but I'm personal).

Uh FWIW Samba disabled SMB1 2.5 years ago. But my point is not that SMB1 should or shouldn't be dropped.

My point is that you can't downgrade because that's a security vulnerability, and in this particular case, an actively exploited one. In other words you MUST NOT use SMB1 with a server that supports SMB2, even if the server accepts SMB1-only clients.

I agree. It should not be automatic. The user can still force smb v1 with some advanced option.

Maybe we could pop up a dialog after 5 seconds telling the user that this might be a smbv1 server (after checking the port is responding).

In this particular case, frighteningasking the user is probably the only way short of dropping support entirely.

mentioned in merge request !1622 (merged)

Depends on !1622 (merged)

added 76 commits

• a346805b...c8570567 - 69 commits from branch videolan:master
• e126c4d5 - access: cache: don't call free_cb from entry_Delete
• e6bda87f - smb2: don't use sys->error_status while opening
• fe28dbcc - smb2: rework error handling
• 606f7c09 - smb2: destroy the context in case of error
• 05d68967 - smb2: remove teardown handling
• bd4e92f9 - smb2: always use smb2 timeout
• 4a4e42fc - smb2: timeout on first connection

Compare with previous version

added MRStatus::NotCompliant label and removed MRStatus::InReview label

mentioned in issue #26825 (closed)

closed