Fix crash when saving a received file to non-writable location

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
author: Tobias Markmann <tm@ayena.de> 2016-02-10 19:25:21 (GMT)
committer: Kevin Smith <kevin.smith@isode.com> 2016-02-15 12:31:10 (GMT)
commit: ca226e7bb019308db4bfc818d7e04422d9d28106 (patch)
tree: 9f7ef065f20db3e04eb08560674eed16d10e95e5 /Swiften/FileTransfer
parent: 431eb62386101dc8fc1e7d346c49bd0d81fda70e (diff)
download: swift-ca226e7bb019308db4bfc818d7e04422d9d28106.zip
swift-ca226e7bb019308db4bfc818d7e04422d9d28106.tar.bz2
8 files changed, 49 insertions, 42 deletions
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<unsigned char>& bytes) {
+			virtual bool write(const std::vector<unsigned char>& bytes) {
 				data.insert(data.end(), bytes.begin(), bytes.end());
 				onWrite(bytes);
+				return true;
 			}
 
 			const std::vector<unsigned char>& 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<unsigned char>& data) {
+bool FileWriteBytestream::write(const std::vector<unsigned char>& 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<const char*>(&data[0]), boost::numeric_cast<std::streamsize>(data.size()));
-	onWrite(data);
+	if (stream->good()) {
+		stream->write(reinterpret_cast<const char*>(&data[0]), boost::numeric_cast<std::streamsize>(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<unsigned char>&);
+			virtual bool write(const std::vector<unsigned char>&);
 			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_ptr<WriteBytest
 		state = Reading;
 		writeBytestream = writeStream;
 		writeBytestream->write(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 <boost/bind.hpp>
 #include <boost/numeric/conversion/cast.hpp>
-#include <iostream>
 
-#include <Swiften/Base/ByteArray.h>
-#include <Swiften/Base/SafeByteArray.h>
 #include <Swiften/Base/Algorithm.h>
+#include <Swiften/Base/ByteArray.h>
 #include <Swiften/Base/Concat.h>
 #include <Swiften/Base/Log.h>
-#include <Swiften/Network/HostAddressPort.h>
-#include <Swiften/FileTransfer/SOCKS5BytestreamRegistry.h>
+#include <Swiften/Base/SafeByteArray.h>
 #include <Swiften/FileTransfer/BytestreamException.h>
+#include <Swiften/FileTransfer/SOCKS5BytestreamRegistry.h>
+#include <Swiften/Network/HostAddressPort.h>
 
 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<ReadBytestream> stream) {
@@ -81,8 +80,9 @@ void SOCKS5BytestreamServerSession::handleDataRead(boost::shared_ptr<SafeByteArr
 		append(unprocessedData, *data);
 		process();
 	} else {
-		writeBytestream->write(createByteArray(vecptr(*data), data->size()));
-		// onBytesReceived(data->size());
+		if (!writeBytestream->write(createByteArray(vecptr(*data), data->size()))) {
+			finish(boost::optional<FileTransferError>(FileTransferError::WriteError));
+		}
 	}
 }
 
@@ -94,7 +94,7 @@ void SOCKS5BytestreamServerSession::handleDataAvailable() {
 
 void SOCKS5BytestreamServerSession::handleDisconnected(const boost::optional<Connection::Error>& 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>(FileTransferError::PeerError) : boost::optional<FileTransferError>());
 }
 
 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>(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>(FileTransferError::PeerError));
 		}
 	}
 	else {
-		finish(false);
+		finish();
 	}
 }
 
-void SOCKS5BytestreamServerSession::finish(bool error) {
-	SWIFT_LOG(debug) << error << " " << state << std::endl;
+void SOCKS5BytestreamServerSession::finish(const boost::optional<FileTransferError>& 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>(FileTransferError::PeerError));
-	} else {
-		onFinished(boost::optional<FileTransferError>());
-	}
+	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 <Swiften/Base/API.h>
 #include <Swiften/Base/boost_bsignals.h>
-#include <Swiften/Network/Connection.h>
+#include <Swiften/FileTransfer/FileTransferError.h>
 #include <Swiften/FileTransfer/ReadBytestream.h>
 #include <Swiften/FileTransfer/WriteBytestream.h>
-#include <Swiften/FileTransfer/FileTransferError.h>
+#include <Swiften/Network/Connection.h>
 
 namespace Swift {
 	class SOCKS5BytestreamRegistry;
@@ -50,14 +50,13 @@ namespace Swift {
 
 			boost::signal<void (boost::optional<FileTransferError>)> onFinished;
 			boost::signal<void (unsigned long long)> onBytesSent;
-			// boost::signal<void (unsigned long long)> onBytesReceived;
 
 			const std::string& getStreamID() const {
 				return streamID;
 			}
 
 		private:
-			void finish(bool error);
+			void finish(const boost::optional<FileTransferError>& error = boost::optional<FileTransferError>());
 			void process();
 			void handleDataRead(boost::shared_ptr<SafeByteArray>);
 			void handleDisconnected(const boost::optional<Connection::Error>&);
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 <boost/shared_ptr.hpp>
 #include <vector>
 
+#include <boost/shared_ptr.hpp>
+
 #include <Swiften/Base/API.h>
 #include <Swiften/Base/boost_bsignals.h>
 
@@ -19,7 +20,12 @@ namespace Swift {
 
 			virtual ~WriteBytestream();
 
-			virtual void write(const std::vector<unsigned char>&) = 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<unsigned char>&) = 0;
 
 			boost::signal<void (const std::vector<unsigned char>&)> onWrite;
 	};
author	Tobias Markmann <tm@ayena.de>	2016-02-10 19:25:21 (GMT)
committer	Kevin Smith <kevin.smith@isode.com>	2016-02-15 12:31:10 (GMT)
commit	ca226e7bb019308db4bfc818d7e04422d9d28106 (patch)
tree	9f7ef065f20db3e04eb08560674eed16d10e95e5 /Swiften/FileTransfer
parent	431eb62386101dc8fc1e7d346c49bd0d81fda70e (diff)
download	swift-ca226e7bb019308db4bfc818d7e04422d9d28106.zip swift-ca226e7bb019308db4bfc818d7e04422d9d28106.tar.bz2