From ca226e7bb019308db4bfc818d7e04422d9d28106 Mon Sep 17 00:00:00 2001 From: Tobias Markmann Date: Wed, 10 Feb 2016 20:25:21 +0100 Subject: Fix crash when saving a received file to non-writable location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WriteBytestream::write(…) now returns a boolean indicating its success state (false in case of an error). Adjusted FileWriteBytestream accordingly. The QtWebKitChatView will test if the file path selected by the user is writable before accepting it and starting the transfer. If it is not writable a red warning message will be added to the file-transfer element in the chat view. Test-Information: Added an integration test that tests the new behavior for the FileWriteBytestream class. Tested two file transfers on OS X 10.11.3, one to a write protected location and another to /tmp. The first is not accepted by the UI, and without the UI sanity check it results in a file-transfer error. The second succeeds as expected. Change-Id: I5aa0c617423073feb371365a23a294c149c88036 diff --git a/Swift/QtUI/QtWebKitChatView.cpp b/Swift/QtUI/QtWebKitChatView.cpp index 260da8a..b543e34 100644 --- a/Swift/QtUI/QtWebKitChatView.cpp +++ b/Swift/QtUI/QtWebKitChatView.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2015 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -747,6 +748,50 @@ void QtWebKitChatView::setWhiteboardSessionStatus(const std::string& id, const C setWhiteboardSessionStatus(P2QSTRING(id), state); } +static bool isFilePathWritable(const QString& path) { + QFileInfo fileInfo = QFileInfo(path); + if (fileInfo.exists()) { + return fileInfo.isWritable(); + } + else { + bool writable = false; + QFile writeTestFile(path); + if (writeTestFile.open(QIODevice::ReadWrite)) { + writeTestFile.write("test"); + if (writeTestFile.error() == QFileDevice::NoError) { + writable = true; + } + } + writeTestFile.close(); + writeTestFile.remove(); + return writable; + } +} + +void QtWebKitChatView::setFileTransferWarning(QString id, QString warningText) { + QWebElement ftElement = findElementWithID(document_, "div", id); + if (ftElement.isNull()) { + SWIFT_LOG(debug) << "Tried to access FT UI via invalid id! id = " << Q2PSTRING(id) << std::endl; + return; + } + + removeFileTransferWarning(id); + ftElement.appendInside(QString("

%1
").arg(QtUtilities::htmlEscape(warningText))); +} + +void QtWebKitChatView::removeFileTransferWarning(QString id) { + QWebElement ftElement = findElementWithID(document_, "div", id); + if (ftElement.isNull()) { + SWIFT_LOG(debug) << "Tried to access FT UI via invalid id! id = " << Q2PSTRING(id) << std::endl; + return; + } + + QWebElement warningElement = ftElement.findFirst(".ft_warning"); + if (!warningElement.isNull()) { + warningElement.removeFromDocument(); + } +} + void QtWebKitChatView::handleHTMLButtonClicked(QString id, QString encodedArgument1, QString encodedArgument2, QString encodedArgument3, QString encodedArgument4, QString encodedArgument5) { QString arg1 = decodeButtonArgument(encodedArgument1); QString arg2 = decodeButtonArgument(encodedArgument2); @@ -777,9 +822,13 @@ void QtWebKitChatView::handleHTMLButtonClicked(QString id, QString encodedArgume QString filename = arg2; QString path = QFileDialog::getSaveFileName(this, tr("Save File"), filename); - if (!path.isEmpty()) { + if (!path.isEmpty() && isFilePathWritable(path)) { filePaths_[ft_id] = path; window_->onFileTransferAccept(Q2PSTRING(ft_id), Q2PSTRING(path)); + removeFileTransferWarning(ft_id); + } + else { + setFileTransferWarning(ft_id, tr("The chosen save location is not writable! Click the 'Accept' button to select a different save location.")); } } else if (id.startsWith(ButtonFileTransferOpenFile)) { diff --git a/Swift/QtUI/QtWebKitChatView.h b/Swift/QtUI/QtWebKitChatView.h index 99375f7..173a05b 100644 --- a/Swift/QtUI/QtWebKitChatView.h +++ b/Swift/QtUI/QtWebKitChatView.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2015 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -95,6 +95,8 @@ namespace Swift { void addToJSEnvironment(const QString&, QObject*); void setFileTransferProgress(QString id, const int percentageDone); void setFileTransferStatus(QString id, const ChatWindow::FileTransferState state, const QString& msg); + void setFileTransferWarning(QString id, QString warningText); + void removeFileTransferWarning(QString id); void setWhiteboardSessionStatus(QString id, const ChatWindow::WhiteboardSessionState state); void setMUCInvitationJoined(QString id); void askDesktopToOpenFile(const QString& filename); diff --git a/Swiften/FileTransfer/ByteArrayWriteBytestream.h b/Swiften/FileTransfer/ByteArrayWriteBytestream.h index 938b1d4..08c4d4b 100644 --- a/Swiften/FileTransfer/ByteArrayWriteBytestream.h +++ b/Swiften/FileTransfer/ByteArrayWriteBytestream.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2015 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -15,9 +15,10 @@ namespace Swift { ByteArrayWriteBytestream() { } - virtual void write(const std::vector& bytes) { + virtual bool write(const std::vector& bytes) { data.insert(data.end(), bytes.begin(), bytes.end()); onWrite(bytes); + return true; } const std::vector& getData() const { diff --git a/Swiften/FileTransfer/FileTransferError.h b/Swiften/FileTransfer/FileTransferError.h index 67e32f2..eff8ca9 100644 --- a/Swiften/FileTransfer/FileTransferError.h +++ b/Swiften/FileTransfer/FileTransferError.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2015 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -15,6 +15,7 @@ namespace Swift { UnknownError, PeerError, ReadError, + WriteError, ClosedError }; diff --git a/Swiften/FileTransfer/FileWriteBytestream.cpp b/Swiften/FileTransfer/FileWriteBytestream.cpp index bbf3d51..c39d63a 100644 --- a/Swiften/FileTransfer/FileWriteBytestream.cpp +++ b/Swiften/FileTransfer/FileWriteBytestream.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2015 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -24,16 +24,21 @@ FileWriteBytestream::~FileWriteBytestream() { } } -void FileWriteBytestream::write(const std::vector& data) { +bool FileWriteBytestream::write(const std::vector& data) { if (data.empty()) { - return; + return true; } if (!stream) { stream = new boost::filesystem::ofstream(file, std::ios_base::out|std::ios_base::binary); } - assert(stream->good()); - stream->write(reinterpret_cast(&data[0]), boost::numeric_cast(data.size())); - onWrite(data); + if (stream->good()) { + stream->write(reinterpret_cast(&data[0]), boost::numeric_cast(data.size())); + if (stream->good()) { + onWrite(data); + return true; + } + } + return false; } void FileWriteBytestream::close() { diff --git a/Swiften/FileTransfer/FileWriteBytestream.h b/Swiften/FileTransfer/FileWriteBytestream.h index b2d3347..02e1b46 100644 --- a/Swiften/FileTransfer/FileWriteBytestream.h +++ b/Swiften/FileTransfer/FileWriteBytestream.h @@ -18,7 +18,7 @@ namespace Swift { FileWriteBytestream(const boost::filesystem::path& file); virtual ~FileWriteBytestream(); - virtual void write(const std::vector&); + virtual bool write(const std::vector&); void close(); private: diff --git a/Swiften/FileTransfer/SOCKS5BytestreamClientSession.cpp b/Swiften/FileTransfer/SOCKS5BytestreamClientSession.cpp index a38501b..4fc0246 100644 --- a/Swiften/FileTransfer/SOCKS5BytestreamClientSession.cpp +++ b/Swiften/FileTransfer/SOCKS5BytestreamClientSession.cpp @@ -5,7 +5,7 @@ */ /* - * Copyright (c) 2013-2015 Isode Limited. + * Copyright (c) 2013-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -166,7 +166,6 @@ void SOCKS5BytestreamClientSession::startReceiving(boost::shared_ptrwrite(unprocessedData); - //onBytesReceived(unprocessedData.size()); unprocessedData.clear(); } else { SWIFT_LOG(debug) << "Session isn't ready for transfer yet!" << std::endl; diff --git a/Swiften/FileTransfer/SOCKS5BytestreamServerSession.cpp b/Swiften/FileTransfer/SOCKS5BytestreamServerSession.cpp index 7838dd1..0e1eb6b 100644 --- a/Swiften/FileTransfer/SOCKS5BytestreamServerSession.cpp +++ b/Swiften/FileTransfer/SOCKS5BytestreamServerSession.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2013 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -8,16 +8,15 @@ #include #include -#include -#include -#include #include +#include #include #include -#include -#include +#include #include +#include +#include namespace Swift { @@ -34,8 +33,8 @@ SOCKS5BytestreamServerSession::SOCKS5BytestreamServerSession( SOCKS5BytestreamServerSession::~SOCKS5BytestreamServerSession() { if (state != Finished && state != Initial) { - std::cerr << "Warning: SOCKS5BytestreamServerSession unfinished" << std::endl; - finish(false); + SWIFT_LOG(warning) << "SOCKS5BytestreamServerSession unfinished" << std::endl; + finish(); } } @@ -47,7 +46,7 @@ void SOCKS5BytestreamServerSession::start() { } void SOCKS5BytestreamServerSession::stop() { - finish(false); + finish(); } void SOCKS5BytestreamServerSession::startSending(boost::shared_ptr stream) { @@ -81,8 +80,9 @@ void SOCKS5BytestreamServerSession::handleDataRead(boost::shared_ptrwrite(createByteArray(vecptr(*data), data->size())); - // onBytesReceived(data->size()); + if (!writeBytestream->write(createByteArray(vecptr(*data), data->size()))) { + finish(boost::optional(FileTransferError::WriteError)); + } } } @@ -94,7 +94,7 @@ void SOCKS5BytestreamServerSession::handleDataAvailable() { void SOCKS5BytestreamServerSession::handleDisconnected(const boost::optional& error) { SWIFT_LOG(debug) << (error ? (error == Connection::ReadError ? "Read Error" : "Write Error") : "No Error") << std::endl; - finish(error ? true : false); + finish(error ? boost::optional(FileTransferError::PeerError) : boost::optional()); } void SOCKS5BytestreamServerSession::process() { @@ -143,7 +143,7 @@ void SOCKS5BytestreamServerSession::process() { if (!hasBytestream) { SWIFT_LOG(debug) << "Readstream or Wrtiestream with ID " << streamID << " not found!" << std::endl; connection->write(result); - finish(true); + finish(boost::optional(FileTransferError::PeerError)); } else { SWIFT_LOG(debug) << "Found stream. Sent OK." << std::endl; @@ -169,16 +169,16 @@ void SOCKS5BytestreamServerSession::sendData() { } } catch (const BytestreamException&) { - finish(true); + finish(boost::optional(FileTransferError::PeerError)); } } else { - finish(false); + finish(); } } -void SOCKS5BytestreamServerSession::finish(bool error) { - SWIFT_LOG(debug) << error << " " << state << std::endl; +void SOCKS5BytestreamServerSession::finish(const boost::optional& error) { + SWIFT_LOG(debug) << "state: " << state << std::endl; if (state == Finished) { return; } @@ -189,11 +189,7 @@ void SOCKS5BytestreamServerSession::finish(bool error) { dataAvailableConnection.disconnect(); readBytestream.reset(); state = Finished; - if (error) { - onFinished(boost::optional(FileTransferError::PeerError)); - } else { - onFinished(boost::optional()); - } + onFinished(error); } } diff --git a/Swiften/FileTransfer/SOCKS5BytestreamServerSession.h b/Swiften/FileTransfer/SOCKS5BytestreamServerSession.h index d8eea34..ed5272f 100644 --- a/Swiften/FileTransfer/SOCKS5BytestreamServerSession.h +++ b/Swiften/FileTransfer/SOCKS5BytestreamServerSession.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -10,10 +10,10 @@ #include #include -#include +#include #include #include -#include +#include namespace Swift { class SOCKS5BytestreamRegistry; @@ -50,14 +50,13 @@ namespace Swift { boost::signal)> onFinished; boost::signal onBytesSent; - // boost::signal onBytesReceived; const std::string& getStreamID() const { return streamID; } private: - void finish(bool error); + void finish(const boost::optional& error = boost::optional()); void process(); void handleDataRead(boost::shared_ptr); void handleDisconnected(const boost::optional&); diff --git a/Swiften/FileTransfer/WriteBytestream.h b/Swiften/FileTransfer/WriteBytestream.h index 3299620..76237c4 100644 --- a/Swiften/FileTransfer/WriteBytestream.h +++ b/Swiften/FileTransfer/WriteBytestream.h @@ -1,14 +1,15 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ #pragma once -#include #include +#include + #include #include @@ -19,7 +20,12 @@ namespace Swift { virtual ~WriteBytestream(); - virtual void write(const std::vector&) = 0; + /** + * Write data from vector argument to bytestream. + * + * On success true is returned and \ref onWrite is called. On failure false is returned. + */ + virtual bool write(const std::vector&) = 0; boost::signal&)> onWrite; }; diff --git a/Swiften/QA/SConscript b/Swiften/QA/SConscript index 2135ef4..2c588e5 100644 --- a/Swiften/QA/SConscript +++ b/Swiften/QA/SConscript @@ -5,7 +5,7 @@ SConscript(dirs = [ # "ReconnectTest", "ClientTest", # "DNSSDTest", -# "StorageTest", + "StorageTest", "TLSTest", "ScriptedTests", "ProxyProviderTest", diff --git a/Swiften/QA/StorageTest/FileReadBytestreamTest.cpp b/Swiften/QA/StorageTest/FileReadBytestreamTest.cpp index c36420a..d70d9c9 100644 --- a/Swiften/QA/StorageTest/FileReadBytestreamTest.cpp +++ b/Swiften/QA/StorageTest/FileReadBytestreamTest.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010 Isode Limited. + * Copyright (c) 2010-2016 Isode Limited. * All rights reserved. * See the COPYING file for more information. */ @@ -9,7 +9,8 @@ #include #include -#include "SwifTools/Application/PlatformApplicationPathProvider.h" + +#include using namespace Swift; @@ -33,18 +34,18 @@ class FileReadBytestreamTest : public CppUnit::TestFixture { void testRead() { boost::shared_ptr testling(createTestling()); - std::vector result = testling->read(10); + boost::shared_ptr< std::vector > result = testling->read(10); - CPPUNIT_ASSERT(ByteArray::create("/*\n * Copy") == result); + CPPUNIT_ASSERT(createByteArray("/*\n * Copy") == *result.get()); } void testRead_Twice() { boost::shared_ptr testling(createTestling()); testling->read(10); - ByteArray result(testling->read(10)); + boost::shared_ptr< std::vector > result = testling->read(10); - CPPUNIT_ASSERT_EQUAL(std::string("right (c) "), result.toString()); + CPPUNIT_ASSERT_EQUAL(std::string("right (c) "), byteArrayToString(*result)); } void testIsFinished_NotFinished() { diff --git a/Swiften/QA/StorageTest/FileWriteBytestreamTest.cpp b/Swiften/QA/StorageTest/FileWriteBytestreamTest.cpp new file mode 100644 index 0000000..3686cf9 --- /dev/null +++ b/Swiften/QA/StorageTest/FileWriteBytestreamTest.cpp @@ -0,0 +1,58 @@ +/* + * Copyright (c) 2016 Isode Limited. + * All rights reserved. + * See the COPYING file for more information. + */ + +#include +#include + +#include +#include + +#include +#include +#include + +using namespace Swift; + +class FileWriteBytestreamTest : public CppUnit::TestFixture { + CPPUNIT_TEST_SUITE(FileWriteBytestreamTest); + CPPUNIT_TEST(testSuccessfulWrite); + CPPUNIT_TEST(testFailingWrite); + CPPUNIT_TEST_SUITE_END(); + + public: + void setUp() { + onWriteWasCalled = false; + } + + void testSuccessfulWrite() { + boost::filesystem::path filename = boost::filesystem::unique_path("write_file_bytestream_test_%%%%%%%%%%%%%%%%.bin"); + boost::shared_ptr writeBytestream = boost::make_shared(filename.string()); + writeBytestream->onWrite.connect(boost::bind(&FileWriteBytestreamTest::handleOnWrite, this, _1)); + + CPPUNIT_ASSERT_EQUAL(true, writeBytestream->write(createByteArray("Some data."))); + CPPUNIT_ASSERT_EQUAL(true, onWriteWasCalled); + + boost::filesystem::remove(filename); + } + + void testFailingWrite() { + boost::shared_ptr writeBytestream = boost::make_shared(""); + writeBytestream->onWrite.connect(boost::bind(&FileWriteBytestreamTest::handleOnWrite, this, _1)); + + CPPUNIT_ASSERT_EQUAL(false, writeBytestream->write(createByteArray("Some data."))); + CPPUNIT_ASSERT_EQUAL(false, onWriteWasCalled); + } + + + void handleOnWrite(const std::vector& /*data*/) { + onWriteWasCalled = true; + } + + private: + bool onWriteWasCalled; +}; + +CPPUNIT_TEST_SUITE_REGISTRATION(FileWriteBytestreamTest); diff --git a/Swiften/QA/StorageTest/SConscript b/Swiften/QA/StorageTest/SConscript index 6d65b30..fff2a2b 100644 --- a/Swiften/QA/StorageTest/SConscript +++ b/Swiften/QA/StorageTest/SConscript @@ -15,7 +15,8 @@ if env["TEST"] : myenv.MergeFlags(myenv["PLATFORM_FLAGS"]) tester = myenv.Program("StorageTest", [ - "VCardFileStorageTest.cpp", + #"VCardFileStorageTest.cpp", "FileReadBytestreamTest.cpp", + "FileWriteBytestreamTest.cpp", ]) myenv.Test(tester, "system", is_checker = True) -- cgit v0.10.2-6-g49f6