diff options
| -rw-r--r-- | lib/backupstore/BackupStoreDirectory.cpp | 9 | ||||
| -rw-r--r-- | lib/backupstore/BackupStoreDirectory.h | 5 | ||||
| -rw-r--r-- | lib/common/Archive.h | 59 | ||||
| -rw-r--r-- | test/backupstore/testbackupstore.cpp | 94 |
4 files changed, 129 insertions, 38 deletions
diff --git a/lib/backupstore/BackupStoreDirectory.cpp b/lib/backupstore/BackupStoreDirectory.cpp index 332eca7a..d158cdd4 100644 --- a/lib/backupstore/BackupStoreDirectory.cpp +++ b/lib/backupstore/BackupStoreDirectory.cpp @@ -1,7 +1,7 @@ // -------------------------------------------------------------------------- // // File -// Name: BackupStoreDirectory.h +// Name: BackupStoreDirectory.cpp // Purpose: Representation of a backup directory // Created: 2003/08/26 // @@ -36,11 +36,6 @@ typedef struct // Then a StreamableMemBlock for attributes } dir_StreamFormat; -typedef enum -{ - Option_DependencyInfoPresent = 1 -} dir_StreamFormatOptions; - typedef struct { uint64_t mModificationTime; @@ -172,7 +167,7 @@ void BackupStoreDirectory::ReadFromStream(IOStream &rStream, int Timeout) int count = ntohl(hdr.mNumEntries); // Clear existing list - for(std::vector<Entry*>::iterator i = mEntries.begin(); + for(std::vector<Entry*>::iterator i = mEntries.begin(); i != mEntries.end(); i++) { delete (*i); diff --git a/lib/backupstore/BackupStoreDirectory.h b/lib/backupstore/BackupStoreDirectory.h index 5bbb0b35..788a3ad0 100644 --- a/lib/backupstore/BackupStoreDirectory.h +++ b/lib/backupstore/BackupStoreDirectory.h @@ -47,6 +47,11 @@ public: } #endif + typedef enum + { + Option_DependencyInfoPresent = 1 + } dir_StreamFormatOptions; + BackupStoreDirectory(); BackupStoreDirectory(int64_t ObjectID, int64_t ContainerID); // Convenience constructor from a stream diff --git a/lib/common/Archive.h b/lib/common/Archive.h index 76b069a0..2b27b303 100644 --- a/lib/common/Archive.h +++ b/lib/common/Archive.h @@ -47,6 +47,9 @@ public: Write((int) Item); } void WriteExact(uint32_t Item) { Write((int)Item); } + // TODO FIXME: use of "int" here is dangerous and deprecated. It can lead to + // incompatible serialisation on non-32-bit machines. Passing anything other + // than one of the specifically supported fixed size types should be forbidden. void Write(int Item) { int32_t privItem = htonl(Item); @@ -57,6 +60,11 @@ public: int64_t privItem = box_hton64(Item); mrStream.Write(&privItem, sizeof(privItem), mTimeout); } + void WriteInt16(uint16_t Item) + { + uint16_t privItem = htons(Item); + mrStream.Write(&privItem, sizeof(privItem), mTimeout); + } void WriteExact(uint64_t Item) { Write(Item); } void Write(uint64_t Item) { @@ -109,6 +117,15 @@ public: } rItemOut = ntohl(privItem); } + void ReadFullBuffer(void* Buffer, size_t Size) + { + if(!mrStream.ReadFullBuffer(Buffer, Size, + 0 /* not interested in bytes read if this fails */, + mTimeout)) + { + THROW_EXCEPTION(CommonException, ArchiveBlockIncompleteRead); + } + } void ReadIfPresent(int &rItemOut, int ValueIfNotPresent) { int32_t privItem; @@ -132,26 +149,22 @@ public: void Read(int64_t &rItemOut) { int64_t privItem; - if(!mrStream.ReadFullBuffer(&privItem, sizeof(privItem), - 0 /* not interested in bytes read if this fails */, - mTimeout)) - { - THROW_EXCEPTION(CommonException, ArchiveBlockIncompleteRead); - } + ReadFullBuffer(&privItem, sizeof(privItem)); rItemOut = box_ntoh64(privItem); } void ReadExact(uint64_t &rItemOut) { Read(rItemOut); } void Read(uint64_t &rItemOut) { uint64_t privItem; - if(!mrStream.ReadFullBuffer(&privItem, sizeof(privItem), - 0 /* not interested in bytes read if this fails */, - mTimeout)) - { - THROW_EXCEPTION(CommonException, ArchiveBlockIncompleteRead); - } + ReadFullBuffer(&privItem, sizeof(privItem)); rItemOut = box_ntoh64(privItem); } + void ReadInt16(uint16_t &rItemOut) + { + uint16_t privItem; + ReadFullBuffer(&privItem, sizeof(privItem)); + rItemOut = ntohs(privItem); + } void Read(uint8_t &rItemOut) { int privItem; @@ -160,14 +173,14 @@ public: } void ReadIfPresent(std::string &rItemOut, const std::string& ValueIfNotPresent) { - Read(rItemOut, &ValueIfNotPresent); + ReadString(rItemOut, &ValueIfNotPresent); } void Read(std::string &rItemOut) { - Read(rItemOut, NULL); + ReadString(rItemOut, NULL); } private: - void Read(std::string &rItemOut, const std::string* pValueIfNotPresent) + void ReadString(std::string &rItemOut, const std::string* pValueIfNotPresent) { int size; int bytesRead; @@ -193,13 +206,7 @@ private: if(size < (int) sizeof(buf)) { // Fetch rest of pPayload, relying on the Protocol to error on stupidly large sizes for us - if(!mrStream.ReadFullBuffer(buf, size, - 0 /* not interested in bytes read if this fails */, - mTimeout)) - { - THROW_EXCEPTION(CommonException, - ArchiveBlockIncompleteRead); - } + ReadFullBuffer(buf, size); // assign to this string, storing the header and the extra payload rItemOut.assign(buf, size); } @@ -210,13 +217,7 @@ private: char *ppayload = dataB; // Fetch rest of pPayload, relying on the Protocol to error on stupidly large sizes for us - if(!mrStream.ReadFullBuffer(ppayload, size, - 0 /* not interested in bytes read if this fails */, - mTimeout)) - { - THROW_EXCEPTION(CommonException, - ArchiveBlockIncompleteRead); - } + ReadFullBuffer(ppayload, size); // assign to this string, storing the header and the extra pPayload rItemOut.assign(ppayload, size); } diff --git a/test/backupstore/testbackupstore.cpp b/test/backupstore/testbackupstore.cpp index c3798bf2..c7320bbf 100644 --- a/test/backupstore/testbackupstore.cpp +++ b/test/backupstore/testbackupstore.cpp @@ -22,11 +22,12 @@ #include "BackupStoreConstants.h" #include "BackupStoreDirectory.h" #include "BackupStoreException.h" -#include "BackupStoreInfo.h" +#include "BackupStoreFile.h" #include "BackupStoreFilenameClear.h" #include "BackupStoreFileEncodeStream.h" +#include "BackupStoreInfo.h" +#include "BackupStoreObjectMagic.h" #include "BackupStoreRefCountDatabase.h" -#include "BackupStoreFile.h" #include "BoxPortsAndFiles.h" #include "CollectInBufferStream.h" #include "Configuration.h" @@ -3121,6 +3122,94 @@ bool test_read_old_backupstoreinfo_files() TEARDOWN_TEST_BACKUPSTORE(); } +// Test that attributes can be correctly read from and written to the standard +// format, for compatibility with other servers and clients. See +// http://mailman.uk.freebsd.org/pipermail../public/boxbackup/2010-November/005818.html and +// http://lists.boxbackup.org/pipermail/boxbackup/2011-February/005978.html for +// details of the problems with packed structs. +bool test_read_write_attr_streamformat() +{ + SETUP_TEST_BACKUPSTORE(); + + // Construct a minimal valid directory with 1 entry in memory using Archive, and + // try to read it back. + CollectInBufferStream buf; + Archive darc(buf, IOStream::TimeOutInfinite); + + // Write a dir_StreamFormat structure + darc.Write((int32_t)OBJECTMAGIC_DIR_MAGIC_VALUE); // mMagicValue + darc.Write((int32_t)1); // mNumEntries + darc.Write((int64_t)0x0123456789abcdef); // mObjectID + darc.Write((int64_t)0x0000000000000001); // mContainerID + darc.Write((uint64_t)0x23456789abcdef01); // mAttributesModTime + darc.Write((int32_t)BackupStoreDirectory::Option_DependencyInfoPresent); + // mOptionsPresent + // 36 bytes to here + + // Write fake attributes to make it valid. + StreamableMemBlock attr; + attr.WriteToStream(buf); + // 40 bytes to here + + // Write a single entry in an en_StreamFormat structure + darc.Write((uint64_t)0x3456789012345678); // mModificationTime + darc.Write((int64_t)0x0000000000000002); // mObjectID + darc.Write((int64_t)0x0000000000000003); // mSizeInBlocks + darc.Write((uint64_t)0x0000000000000004); // mAttributesHash + darc.WriteInt16((int16_t)0x3141); // mFlags + // 74 bytes to here + + // Then a BackupStoreFilename + BackupStoreFilename fn; + fn.SetAsClearFilename("hello"); + fn.WriteToStream(buf); + // 81 bytes to here + + // Then a StreamableMemBlock for attributes + attr.WriteToStream(buf); + // 85 bytes to here + + // Then an en_StreamFormatDepends for dependency info. + darc.Write((uint64_t)0x0000000000000005); // mDependsNewer + darc.Write((uint64_t)0x0000000000000006); // mDependsOlder + // 101 bytes to here + + // Make sure none of the fields was expanded in transit by Archive. + TEST_EQUAL(101, buf.GetSize()); + + buf.SetForReading(); + BackupStoreDirectory dir(buf); + + TEST_EQUAL(1, dir.GetNumberOfEntries()); + TEST_EQUAL(0x0123456789abcdef, dir.GetObjectID()); + TEST_EQUAL(0x0000000000000001, dir.GetContainerID()); + TEST_EQUAL(0x23456789abcdef01, dir.GetAttributesModTime()); + + BackupStoreDirectory::Iterator i(dir); + BackupStoreDirectory::Entry* pen = i.Next(); + TEST_THAT_OR(pen != NULL, FAIL); + TEST_EQUAL(0x3456789012345678, pen->GetModificationTime()); + TEST_EQUAL(0x0000000000000002, pen->GetObjectID()); + TEST_EQUAL(0x0000000000000003, pen->GetSizeInBlocks()); + TEST_EQUAL(0x0000000000000004, pen->GetAttributesHash()); + TEST_EQUAL(0x0000000000000005, pen->GetDependsNewer()); + TEST_EQUAL(0x0000000000000006, pen->GetDependsOlder()); + TEST_EQUAL(0x3141, pen->GetFlags()); + + CollectInBufferStream buf2; + dir.WriteToStream(buf2); + buf2.SetForReading(); + buf.Seek(0, IOStream::SeekType_Absolute); + TEST_EQUAL(101, buf2.GetSize()); + TEST_EQUAL(buf.GetSize(), buf2.GetSize()); + + // Test that the stream written out for the Directory is exactly the same as the + // one we hand-crafted earlier. + TEST_EQUAL(0, memcmp(buf.GetBuffer(), buf2.GetBuffer(), buf.GetSize())); + + TEARDOWN_TEST_BACKUPSTORE(); +} + int test(int argc, const char *argv[]) { TEST_THAT(test_open_files_with_limited_win32_permissions()); @@ -3187,6 +3276,7 @@ int test(int argc, const char *argv[]) TEST_THAT(test_account_limits_respected()); TEST_THAT(test_multiple_uploads()); TEST_THAT(test_housekeeping_deletes_files()); + TEST_THAT(test_read_write_attr_streamformat()); return finish_test_suite(); } |