[Box Backup-commit] COMMIT r3301 - in box/trunk: lib/backupstore lib/common lib/server test/backupstore

subversion at boxbackup.org subversion at boxbackup.org
Sun Mar 2 08:59:39 GMT 2014


Author: chris
Date: 2014-03-02 08:59:39 +0000 (Sun, 02 Mar 2014)
New Revision: 3301

Modified:
   box/trunk/lib/backupstore/BackupCommands.cpp
   box/trunk/lib/common/SelfFlushingStream.h
   box/trunk/lib/server/makeprotocol.pl.in
   box/trunk/test/backupstore/testbackupstore.cpp
Log:
Always flush any incoming stream on server side.

Otherwise the protocol might be broken and can't be used any more, even if
we made an effort to return an Error reply instead of throwing an exception.

This used to not be a problem because an Error reply would terminate the
connection anyway, but it no longer does. So if the client also didn't
terminate, but tried to handle the exception and keep using the connection,
then it might find that its next command fails because the protocol is broken.

Modified: box/trunk/lib/backupstore/BackupCommands.cpp
===================================================================
--- box/trunk/lib/backupstore/BackupCommands.cpp	2014-03-02 08:59:22 UTC (rev 3300)
+++ box/trunk/lib/backupstore/BackupCommands.cpp	2014-03-02 08:59:39 UTC (rev 3301)
@@ -227,7 +227,9 @@
 //		Created: 2003/09/02
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<BackupProtocolMessage> BackupProtocolStoreFile::DoCommand(BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext) const
+std::auto_ptr<BackupProtocolMessage> BackupProtocolStoreFile::DoCommand(
+	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext,
+	IOStream& rDataStream) const
 {
 	CHECK_PHASE(Phase_Commands)
 	CHECK_WRITEABLE_SESSION
@@ -249,14 +251,11 @@
 		}
 	}
 	
-	// A stream follows, which contains the file
-	std::auto_ptr<IOStream> filestream(rProtocol.ReceiveStream());
-	
 	// Ask the context to store it
 	int64_t id = 0;
 	try
 	{
-		id = rContext.AddFile(*filestream, mDirectoryObjectID,
+		id = rContext.AddFile(rDataStream, mDirectoryObjectID,
 			mModificationTime, mAttributesHash, mDiffFromFileID,
 			mFilename,
 			true /* mark files with same name as old versions */);
@@ -469,11 +468,12 @@
 //
 // --------------------------------------------------------------------------
 std::auto_ptr<BackupProtocolMessage> BackupProtocolCreateDirectory::DoCommand(
-	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext) const
+	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext,
+	IOStream& rDataStream) const
 {
 	return BackupProtocolCreateDirectory2(mContainingDirectoryID,
 		mAttributesModTime, 0 /* ModificationTime */,
-		mDirectoryName).DoCommand(rProtocol, rContext);
+		mDirectoryName).DoCommand(rProtocol, rContext, rDataStream);
 }
 
 
@@ -488,17 +488,16 @@
 //
 // --------------------------------------------------------------------------
 std::auto_ptr<BackupProtocolMessage> BackupProtocolCreateDirectory2::DoCommand(
-	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext) const
+	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext,
+	IOStream& rDataStream) const
 {
 	CHECK_PHASE(Phase_Commands)
 	CHECK_WRITEABLE_SESSION
 	
-	// Get the stream containing the attributes
-	std::auto_ptr<IOStream> attrstream(rProtocol.ReceiveStream());
 	// Collect the attributes -- do this now so no matter what the outcome, 
 	// the data has been absorbed.
 	StreamableMemBlock attr;
-	attr.Set(*attrstream, rProtocol.GetTimeout());
+	attr.Set(rDataStream, rProtocol.GetTimeout());
 	
 	// Check to see if the hard limit has been exceeded
 	if(rContext.HardLimitExceeded())
@@ -547,17 +546,17 @@
 //		Created: 2003/09/06
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<BackupProtocolMessage> BackupProtocolChangeDirAttributes::DoCommand(BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext) const
+std::auto_ptr<BackupProtocolMessage> BackupProtocolChangeDirAttributes::DoCommand(
+	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext,
+	IOStream& rDataStream) const
 {
 	CHECK_PHASE(Phase_Commands)
 	CHECK_WRITEABLE_SESSION
 
-	// Get the stream containing the attributes
-	std::auto_ptr<IOStream> attrstream(rProtocol.ReceiveStream());
 	// Collect the attributes -- do this now so no matter what the outcome, 
 	// the data has been absorbed.
 	StreamableMemBlock attr;
-	attr.Set(*attrstream, rProtocol.GetTimeout());
+	attr.Set(rDataStream, rProtocol.GetTimeout());
 
 	// Get the context to do it's magic
 	rContext.ChangeDirAttributes(mObjectID, attr, mAttributesModTime);
@@ -575,17 +574,18 @@
 //		Created: 2003/09/06
 //
 // --------------------------------------------------------------------------
-std::auto_ptr<BackupProtocolMessage> BackupProtocolSetReplacementFileAttributes::DoCommand(BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext) const
+std::auto_ptr<BackupProtocolMessage>
+BackupProtocolSetReplacementFileAttributes::DoCommand(
+	BackupProtocolReplyable &rProtocol, BackupStoreContext &rContext,
+	IOStream& rDataStream) const
 {
 	CHECK_PHASE(Phase_Commands)
 	CHECK_WRITEABLE_SESSION
 
-	// Get the stream containing the attributes
-	std::auto_ptr<IOStream> attrstream(rProtocol.ReceiveStream());
 	// Collect the attributes -- do this now so no matter what the outcome, 
 	// the data has been absorbed.
 	StreamableMemBlock attr;
-	attr.Set(*attrstream, rProtocol.GetTimeout());
+	attr.Set(rDataStream, rProtocol.GetTimeout());
 
 	// Get the context to do it's magic
 	int64_t objectID = 0;

Modified: box/trunk/lib/common/SelfFlushingStream.h
===================================================================
--- box/trunk/lib/common/SelfFlushingStream.h	2014-03-02 08:59:22 UTC (rev 3300)
+++ box/trunk/lib/common/SelfFlushingStream.h	2014-03-02 08:59:39 UTC (rev 3301)
@@ -33,6 +33,12 @@
 	
 	~SelfFlushingStream()
 	{
+		if(StreamDataLeft())
+		{
+			BOX_WARNING("Not all data was read from stream, "
+				"discarding the rest");
+		}
+
 		Flush();
 	}
 

Modified: box/trunk/lib/server/makeprotocol.pl.in
===================================================================
--- box/trunk/lib/server/makeprotocol.pl.in	2014-03-02 08:59:22 UTC (rev 3300)
+++ box/trunk/lib/server/makeprotocol.pl.in	2014-03-02 08:59:39 UTC (rev 3301)
@@ -159,8 +159,9 @@
 
 #include "$filename_base.h"
 #include "CollectInBufferStream.h"
+#include "MemBlockStream.h"
+#include "SelfFlushingStream.h"
 #include "SocketStream.h"
-#include "MemBlockStream.h"
 __E
 
 print H <<__E;
@@ -227,6 +228,9 @@
 public:
 	virtual std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol,
 		$context_class &rContext) const;
+	virtual std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol,
+		$context_class &rContext, IOStream& rDataStream) const;
+	virtual bool HasStreamWithCommand() const = 0;
 };
 
 class $reply_base_class
@@ -259,6 +263,12 @@
 {
 	THROW_EXCEPTION(ConnectionException, Conn_Protocol_TriedToExecuteReplyCommand)
 }
+
+std::auto_ptr<$message_base_class> $message_base_class\::DoCommand($replyable_base_class &rProtocol,
+	$context_class &rContext, IOStream& rDataStream) const
+{
+	THROW_EXCEPTION(ConnectionException, Conn_Protocol_TriedToExecuteReplyCommand)
+}
 __E
 
 my %cmd_classes;
@@ -317,14 +327,39 @@
 		print H "\tstd::string GetMessage() const;\n";
 	}
 	
-	if(obj_is_type($cmd, 'Command'))
+	my $has_stream = obj_is_type($cmd, 'StreamWithCommand');
+
+	if(obj_is_type($cmd, 'Command') && $has_stream)
 	{
 		print H <<__E;
 	std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol, 
+		$context_class &rContext, IOStream& rDataStream) const; // IMPLEMENT THIS\n
+	std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol,
+		$context_class &rContext) const
+	{
+		THROW_EXCEPTION_MESSAGE(CommonException, Internal,
+			"This command requires a stream parameter");
+	}
+__E
+	}
+	elsif(obj_is_type($cmd, 'Command') && !$has_stream)
+	{
+		print H <<__E;
+	std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol, 
 		$context_class &rContext) const; // IMPLEMENT THIS\n
+	std::auto_ptr<$message_base_class> DoCommand($replyable_base_class &rProtocol,
+		$context_class &rContext, IOStream& rDataStream) const
+	{
+		THROW_EXCEPTION_MESSAGE(CommonException, NotSupported,
+			"This command requires no stream parameter");
+	}
 __E
 	}
 
+	print H <<__E;
+	bool HasStreamWithCommand() const { return $has_stream; }
+__E
+
 	# want to be able to read from streams?
 	print H "\tvoid SetPropertiesFromStreamData(Protocol &rProtocol);\n";
 	
@@ -1002,10 +1037,20 @@
 	{
 		// Get an object from the conversation
 		std::auto_ptr<$message_base_class> pobj = Receive();
+		std::auto_ptr<$message_base_class> preply;
 
 		// Run the command
-		std::auto_ptr<$message_base_class> preply = pobj->DoCommand(*this, rContext);
-		
+		if(pobj->HasStreamWithCommand())
+		{
+			std::auto_ptr<IOStream> apDataStream = ReceiveStream();
+			SelfFlushingStream autoflush(*apDataStream);
+			preply = pobj->DoCommand(*this, rContext, *apDataStream);
+		}
+		else
+		{
+			preply = pobj->DoCommand(*this, rContext);
+		}
+
 		// Send the reply
 		Send(*preply);
 
@@ -1052,7 +1097,7 @@
 				my $reply_class = $cmd_classes{$reply_msg};
 				my $reply_id = $cmd_id{$reply_msg};
 				my $has_stream = obj_is_type($cmd,'StreamWithCommand');
-				my $argextra = $has_stream?', std::auto_ptr<IOStream> apStream':'';
+				my $argextra = $has_stream?', std::auto_ptr<IOStream> apDataStream':'';
 				my $send_stream_extra = '';
 				my $send_stream_method = $writing_client ? "SendStream"
 					: "SendStreamAfterCommand";
@@ -1068,7 +1113,7 @@
 					{
 						$send_stream_extra = <<__E;
 	// Send stream after the command
-	SendStream(*apStream);
+	SendStream(*apDataStream);
 __E
 					}
 					
@@ -1078,7 +1123,7 @@
 $send_stream_extra
 	
 	// Wait for the reply
-	std::auto_ptr<$message_base_class> preply = Receive();
+	std::auto_ptr<$message_base_class> apReply = Receive();
 __E
 				}
 				elsif($writing_local)
@@ -1085,30 +1130,22 @@
 				{
 					if($has_stream)
 					{
-						$send_stream_extra = <<__E;
-	// Send stream after the command
-	SendStreamAfterCommand(apStream);
+						print CPP <<__E;
+	std::auto_ptr<$message_base_class> apReply = rQuery.DoCommand(*this,
+		mrContext, *apDataStream);
 __E
 					}
-
-					print CPP <<__E;
-	// Push streams to send, if any, into queue for retrieval by DoCommand.
-	$send_stream_extra
-
-	// Execute the command and get the reply message
-	std::auto_ptr<$message_base_class> preply = rQuery.DoCommand(*this, mrContext);
-	
-	if(!mStreamsToSend.empty())
-	{
-		THROW_EXCEPTION_MESSAGE(ConnectionException,
-			Protocol_StreamsNotConsumed, rQuery.ToString());
-	}
+					else
+					{
+						print CPP <<__E;
+	std::auto_ptr<$message_base_class> apReply = rQuery.DoCommand(*this, mrContext);
 __E
+					}
 				}
 
 				# Common to both client and local
 				print CPP <<__E;
-	CheckReply("$cmd", rQuery, *preply, $reply_id);
+	CheckReply("$cmd", rQuery, *apReply, $reply_id);
 
 	// Correct response, if no exception thrown by CheckReply
 	return std::auto_ptr<$reply_class>(

Modified: box/trunk/test/backupstore/testbackupstore.cpp
===================================================================
--- box/trunk/test/backupstore/testbackupstore.cpp	2014-03-02 08:59:22 UTC (rev 3300)
+++ box/trunk/test/backupstore/testbackupstore.cpp	2014-03-02 08:59:39 UTC (rev 3301)
@@ -45,6 +45,7 @@
 #include "StoreTestUtils.h"
 #include "TLSContext.h"
 #include "Test.h"
+#include "ZeroStream.h"
 
 #include "MemLeakFindOn.h"
 
@@ -1077,6 +1078,18 @@
 	TEST_THAT(check_num_files(0, 0, 0, 1));
 	TEST_THAT(check_num_blocks(protocol, 0, 0, 0, root_dir_blocks, root_dir_blocks));
 
+	// Used to not consume the stream
+	std::auto_ptr<IOStream> upload(new ZeroStream(1000));
+	TEST_COMMAND_RETURNS_ERROR(protocol.QueryStoreFile(
+			BACKUPSTORE_ROOT_DIRECTORY_ID,
+			0,
+			0, /* use for attr hash too */
+			99999, /* diff from ID */
+			uploads[0].name,
+			upload),
+		Err_DiffFromFileDoesNotExist);
+	// TODO FIXME replace all other TEST_CHECK_THROWS with TEST_COMMAND_RETURNS_ERROR
+
 	// TODO FIXME These tests should not be here, but in
 	// test_server_commands. But make sure you use a network protocol,
 	// not a local one, when you move them.




More information about the Boxbackup-commit mailing list