Changeset View
Standalone View
sftp/kio_sftp.cpp
Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Line(s) | |||||
50 | #else | 50 | #else | ||
51 | #include <utime.h> | 51 | #include <utime.h> | ||
52 | #endif | 52 | #endif | ||
53 | 53 | | |||
54 | #define KIO_SFTP_SPECIAL_TIMEOUT 30 | 54 | #define KIO_SFTP_SPECIAL_TIMEOUT 30 | ||
55 | 55 | | |||
56 | // How big should each data packet be? Definitely not bigger than 64kb or | 56 | // How big should each data packet be? Definitely not bigger than 64kb or | ||
57 | // you will overflow the 2 byte size variable in a sftp packet. | 57 | // you will overflow the 2 byte size variable in a sftp packet. | ||
58 | // TODO: investigate what size we should have and consider changing. | ||||
59 | // this seems too large... | ||||
60 | // from the RFC: | ||||
61 | // The maximum size of a packet is in practise determined by the client | ||||
62 | // (the maximum size of read or write requests that it sends, plus a few | ||||
63 | // bytes of packet overhead). All servers SHOULD support packets of at | ||||
64 | // least 34000 bytes (where the packet size refers to the full length, | ||||
65 | // including the header above). This should allow for reads and writes of | ||||
66 | // at most 32768 bytes. | ||||
67 | // In practise that means we can assume that the server supports 32kb, | ||||
68 | // it may be more or it could be less. Since there's not really a system in place to | ||||
69 | // figure out the maximum (and at least openssh arbitrarily resets the entire | ||||
70 | // session if it finds a packet that is too large | ||||
71 | // [https://bugs.kde.org/show_bug.cgi?id=404890]) we ought to be more conservative! | ||||
72 | // At the same time there's no bug reports about the 60k requests being too large so | ||||
73 | // perhaps all popular servers effectively support at least 64k. | ||||
58 | #define MAX_XFER_BUF_SIZE (60 * 1024) | 74 | #define MAX_XFER_BUF_SIZE (60 * 1024) | ||
meven: Why not change it now to 32 * 1024 then ?
I guess you tested this value works at least with… | |||||
The reason I am not changing it is because it is out of scope for the bug fix and requires research. I'm also kinda leaning towards keeping it until there's a bug report for a server that doesn't support it. The way the RFC is written reads incredibly open ended to the point where there may be no maximum or it may be even smaller than 32kb. sitter: The reason I am not changing it is because it is out of scope for the bug fix and requires… | |||||
59 | 75 | | |||
60 | #define KSFTP_ISDIR(sb) (sb->type == SSH_FILEXFER_TYPE_DIRECTORY) | 76 | #define KSFTP_ISDIR(sb) (sb->type == SSH_FILEXFER_TYPE_DIRECTORY) | ||
61 | 77 | | |||
62 | using namespace KIO; | 78 | using namespace KIO; | ||
63 | extern "C" | 79 | extern "C" | ||
64 | { | 80 | { | ||
65 | int Q_DECL_EXPORT kdemain( int argc, char **argv ) | 81 | int Q_DECL_EXPORT kdemain( int argc, char **argv ) | ||
66 | { | 82 | { | ||
▲ Show 20 Lines • Show All 1364 Lines • ▼ Show 20 Line(s) | |||||
1431 | } | 1447 | } | ||
1432 | 1448 | | |||
1433 | Result SFTPInternal::write(const QByteArray &data) | 1449 | Result SFTPInternal::write(const QByteArray &data) | ||
1434 | { | 1450 | { | ||
1435 | qCDebug(KIO_SFTP_LOG) << "write, offset = " << openOffset << ", bytes = " << data.size(); | 1451 | qCDebug(KIO_SFTP_LOG) << "write, offset = " << openOffset << ", bytes = " << data.size(); | ||
1436 | 1452 | | |||
1437 | Q_ASSERT(mOpenFile != nullptr); | 1453 | Q_ASSERT(mOpenFile != nullptr); | ||
1438 | 1454 | | |||
1439 | ssize_t bytesWritten = sftp_write(mOpenFile, data.data(), data.size()); | 1455 | if (!sftpWrite(mOpenFile, data, nullptr)) { | ||
1440 | if (bytesWritten < 0) { | | |||
1441 | qCDebug(KIO_SFTP_LOG) << "Could not write to " << mOpenUrl; | 1456 | qCDebug(KIO_SFTP_LOG) << "Could not write to " << mOpenUrl; | ||
1442 | close(); | 1457 | close(); | ||
1443 | return Result::fail(KIO::ERR_CANNOT_WRITE, mOpenUrl.toDisplayString()); | 1458 | return Result::fail(KIO::ERR_CANNOT_WRITE, mOpenUrl.toDisplayString()); | ||
1444 | } | 1459 | } | ||
1445 | 1460 | | |||
1446 | q->written(bytesWritten); | 1461 | q->written(data.size()); | ||
1447 | 1462 | | |||
1448 | return Result::pass(); | 1463 | return Result::pass(); | ||
1449 | } | 1464 | } | ||
1450 | 1465 | | |||
1451 | Result SFTPInternal::seek(KIO::filesize_t offset) | 1466 | Result SFTPInternal::seek(KIO::filesize_t offset) | ||
1452 | { | 1467 | { | ||
1453 | qCDebug(KIO_SFTP_LOG) << "seek, offset = " << offset; | 1468 | qCDebug(KIO_SFTP_LOG) << "seek, offset = " << offset; | ||
1454 | 1469 | | |||
▲ Show 20 Lines • Show All 345 Lines • ▼ Show 20 Line(s) | 1812 | if (result == 0) { | |||
1800 | // http://bugs.proftpd.org/show_bug.cgi?id=4398 | 1815 | // http://bugs.proftpd.org/show_bug.cgi?id=4398 | ||
1801 | // At this point we'll have opened the file and thus created it. | 1816 | // At this point we'll have opened the file and thus created it. | ||
1802 | // It's safe to break here as even in the ideal scenario that the server | 1817 | // It's safe to break here as even in the ideal scenario that the server | ||
1803 | // doesn't fall over, the write code is pointless because zero size writes | 1818 | // doesn't fall over, the write code is pointless because zero size writes | ||
1804 | // do absolutely nothing. | 1819 | // do absolutely nothing. | ||
1805 | break; | 1820 | break; | ||
1806 | } | 1821 | } | ||
1807 | 1822 | | |||
1808 | ssize_t bytesWritten = sftp_write(file, buffer.data(), buffer.size()); | 1823 | bool success = sftpWrite(file, buffer, [this,&totalBytesSent](int bytes) { | ||
1809 | if (bytesWritten < 0) { | 1824 | totalBytesSent += bytes; | ||
1810 | qCDebug(KIO_SFTP_LOG) << "Failed to sftp_write" << buffer.size() << "bytes." | 1825 | q->processedSize(totalBytesSent); | ||
1811 | << "- Already written: " << totalBytesSent | 1826 | }); | ||
1812 | << "- SFTP error:" << sftp_get_error(mSftp) | 1827 | if (!success) { | ||
1813 | << "- SSH error:" << ssh_get_error_code(mSession); | 1828 | qCDebug(KIO_SFTP_LOG) << "totalBytesSent at error:" << totalBytesSent; | ||
1814 | errorCode = KIO::ERR_CANNOT_WRITE; | 1829 | errorCode = KIO::ERR_CANNOT_WRITE; | ||
1815 | result = -1; | 1830 | result = -1; | ||
1816 | } else { | 1831 | break; | ||
1817 | totalBytesSent += bytesWritten; | | |||
1818 | q->processedSize(totalBytesSent); | | |||
1819 | } | 1832 | } | ||
1820 | } // result | 1833 | } // result | ||
1821 | } while (result > 0); | 1834 | } while (result > 0); | ||
1822 | sftp_attributes_free(sb); | 1835 | sftp_attributes_free(sb); | ||
1823 | 1836 | | |||
1824 | // An error occurred deal with it. | 1837 | // An error occurred deal with it. | ||
1825 | if (result < 0) { | 1838 | if (result < 0) { | ||
1826 | qCDebug(KIO_SFTP_LOG) << "Error during 'put'. Aborting."; | 1839 | qCDebug(KIO_SFTP_LOG) << "Error during 'put'. Aborting."; | ||
1827 | 1840 | | |||
1828 | if (file != nullptr) { | 1841 | if (file != nullptr) { | ||
meven: `const auto length = qMin<int>(MAX_XFER_BUF_SIZE, buffer.size() - offset);` | |||||
1829 | sftp_close(file); | 1842 | sftp_close(file); | ||
AFAICT the size of the buffer never changes so this will easily cause a buffer overrun if I'm not mistaken? Say for example you have a buffer with buffer.size() == MAX_XFER_BUF_SIZE + 1. Then on the second iteration of the while loop (assuming bytesWritten == MAX_XFER_BUF_SIZE) you'll do a sftp_write() pointing to a char buffer of size 1, but which incorrectly states that the size is MAX_XFER_BUF_SIZE. feverfew: AFAICT the size of the buffer never changes so this will easily cause a buffer overrun if I'm… | |||||
Maybe we can deduce the server buffer size based on the bytesWritten value : at init serv_buffer_size =MAX_XFER_BUF_SIZE; and then if (length > bytesWritten) { serv_buffer_size = bytesWritten } and use serv_buffer_size instead of MAX_XFER_BUF_SIZE in the loop. meven: Maybe we can deduce the server buffer size based on the `bytesWritten` value : at init… | |||||
I'm not sure if that would help. MAX_XFER_BUF_SIZE would be the upper bound of this approach, and if the server buffer size is less I imagine we'll crash anyway (as detailed in the bug report). If we simply write less then yes, we can use this to "calibrate" but seeming as it crashes instead then unfortunately I don't think this will work. feverfew: I'm not sure if that would help. `MAX_XFER_BUF_SIZE` would be the upper bound of this approach… | |||||
What Alex said. I guess we could try to infer if a write failed because of buffer size but it seems a waste of time in the grand scheme of things (and also has lots of pits to fall into since we'd have to hook onto a callback and then carry out a mid-flight session reset). In the long run we aren't loosing much by going with a static size and request queuing like we do for read() already. From what I have seen in the past the small requests of sftp become largely irrelevant with queuing and efficient (threaded) encryption. sitter: What Alex said.
I guess we could try to infer if a write failed because of buffer size but it… | |||||
1830 | 1843 | | |||
1831 | sftp_attributes attr = sftp_stat(mSftp, dest.constData()); | 1844 | sftp_attributes attr = sftp_stat(mSftp, dest.constData()); | ||
1832 | if (bMarkPartial && attr != nullptr) { | 1845 | if (bMarkPartial && attr != nullptr) { | ||
1833 | size_t size = q->configValue(QStringLiteral("MinimumKeepSize"), DEFAULT_MINIMUM_KEEP_SIZE); | 1846 | size_t size = q->configValue(QStringLiteral("MinimumKeepSize"), DEFAULT_MINIMUM_KEEP_SIZE); | ||
1834 | if (attr->size < size) { | 1847 | if (attr->size < size) { | ||
1835 | sftp_unlink(mSftp, dest.constData()); | 1848 | sftp_unlink(mSftp, dest.constData()); | ||
1836 | } | 1849 | } | ||
1837 | } | 1850 | } | ||
▲ Show 20 Lines • Show All 66 Lines • ▼ Show 20 Line(s) | 1907 | if (attr != nullptr) { | |||
1904 | sftp_attributes_free(attr); | 1917 | sftp_attributes_free(attr); | ||
1905 | } | 1918 | } | ||
1906 | } | 1919 | } | ||
1907 | } | 1920 | } | ||
1908 | 1921 | | |||
1909 | return Result::pass(); | 1922 | return Result::pass(); | ||
1910 | } | 1923 | } | ||
1911 | 1924 | | |||
1925 | bool SFTPInternal::sftpWrite(sftp_file file, | ||||
1926 | const QByteArray &buffer, | ||||
1927 | const std::function<void(int bytes)> &onWritten) | ||||
1928 | { | ||||
1929 | // TODO: enqueue requests. | ||||
1930 | // Similarly to reading we may enqueue a number of requests to mitigate the | ||||
1931 | // network overhead and speed up things. Serial iteration gives super poor | ||||
1932 | // performance. | ||||
1933 | | ||||
1934 | // Break up into multiple requests in case a single request would be too large. | ||||
1935 | // Servers can impose an arbitrary size limit that we don't want to hit. | ||||
1936 | // https://bugs.kde.org/show_bug.cgi?id=404890 | ||||
1937 | off_t offset = 0; | ||||
1938 | while (offset < buffer.size()) { | ||||
1939 | const auto length = qMin<int>(MAX_XFER_BUF_SIZE, buffer.size() - offset); | ||||
1940 | ssize_t bytesWritten = sftp_write(file, buffer.data() + offset, length); | ||||
1941 | if (bytesWritten < 0) { | ||||
1942 | qCDebug(KIO_SFTP_LOG) << "Failed to sftp_write" << length << "bytes." | ||||
1943 | << "- Already written (for this call):" << offset | ||||
1944 | << "- Return of sftp_write:" << bytesWritten | ||||
1945 | << "- SFTP error:" << sftp_get_error(mSftp) | ||||
1946 | << "- SSH error:" << ssh_get_error_code(mSession) | ||||
1947 | << "- SSH errorString:" << ssh_get_error(mSession); | ||||
1948 | return false; | ||||
1949 | } else if (onWritten) { | ||||
1950 | onWritten(bytesWritten); | ||||
1951 | } | ||||
1952 | offset += bytesWritten; | ||||
1953 | } | ||||
1954 | return true; | ||||
1955 | } | ||||
1956 | | ||||
1912 | Result SFTPInternal::copy(const QUrl &src, const QUrl &dest, int permissions, KIO::JobFlags flags) | 1957 | Result SFTPInternal::copy(const QUrl &src, const QUrl &dest, int permissions, KIO::JobFlags flags) | ||
1913 | { | 1958 | { | ||
1914 | qCDebug(KIO_SFTP_LOG) << src << " -> " << dest << " , permissions = " << QString::number(permissions) | 1959 | qCDebug(KIO_SFTP_LOG) << src << " -> " << dest << " , permissions = " << QString::number(permissions) | ||
1915 | << ", overwrite = " << (flags & KIO::Overwrite) | 1960 | << ", overwrite = " << (flags & KIO::Overwrite) | ||
1916 | << ", resume = " << (flags & KIO::Resume); | 1961 | << ", resume = " << (flags & KIO::Resume); | ||
1917 | 1962 | | |||
1918 | const bool isSourceLocal = src.isLocalFile(); | 1963 | const bool isSourceLocal = src.isLocalFile(); | ||
1919 | const bool isDestinationLocal = dest.isLocalFile(); | 1964 | const bool isDestinationLocal = dest.isLocalFile(); | ||
▲ Show 20 Lines • Show All 846 Lines • Show Last 20 Lines |
Why not change it now to 32 * 1024 then ?
I guess you tested this value works at least with openssh.
I guess the best solution would be to ask/figure out the server buffer size.
What does gvfs, or other libs ?