Some security fixes for Skin and Curve deseralization.

Both Curve and Skin have the issue that the format
does not store any kind of length.

So, I serialize a Skin to binary, followed but some other data
in the same binary. Then at runtine I can deserialize the Skin
and call  mySkin.setFromRaw(rawData, validOffset, INVALID_LENGTH)

At which point there the possibility, how ever small, that
the skin deserialization code will read into the next
chunk of binary data in the RawData. Data that does not belong
to it. 

The best solution IMO would be to add a length or count to
the Skin and Curve formats since then, like Buffer, it would
know exactly how much data is expected and if the length
passed in does not match the length the format says it needs
it would fail. 

Unfortunately, that would break all assets out there. 

This fix just makes sure that if we do get any kind of error
the data is not left in the Skin or Curve like it was before.

Should probably do the same with Buffer.


Review URL: http://codereview.chromium.org/155599

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20841 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 24 insertions, 0 deletions

diff --git a/o3d/core/cross/curve.cc b/o3d/core/cross/curve.cc
index 9aff37f..5978f9d 100644
--- a/o3d/core/cross/curve.cc
+++ b/o3d/core/cross/curve.cc
@@ -726,6 +726,7 @@ bool Curve::LoadFromBinaryData(MemoryReadStream *stream) {
     switch (key_type) {
       case CurveKey::TYPE_STEP: {
         if (available_bytes < kStepDataSize) {
+          FreeAll();  // This must be called before O3D_ERROR.
           O3D_ERROR(service_locator()) << "unexpected end of curve data";
           return false;
         }
@@ -738,6 +739,7 @@ bool Curve::LoadFromBinaryData(MemoryReadStream *stream) {
       }
       case CurveKey::TYPE_LINEAR: {
         if (available_bytes < kLinearDataSize) {
+          FreeAll();  // This must be called before O3D_ERROR.
           O3D_ERROR(service_locator()) << "unexpected end of curve data";
           return false;
         }
@@ -750,6 +752,7 @@ bool Curve::LoadFromBinaryData(MemoryReadStream *stream) {
       }
       case CurveKey::TYPE_BEZIER: {
         if (available_bytes < kBezierDataSize) {
+          FreeAll();  // This must be called before O3D_ERROR.
           O3D_ERROR(service_locator()) << "unexpected end of curve data";
           return false;
         }
@@ -765,6 +768,7 @@ bool Curve::LoadFromBinaryData(MemoryReadStream *stream) {
         break;
       }
       default: {
+        FreeAll();  // This must be called before O3D_ERROR.
         O3D_ERROR(service_locator()) << "invalid curve data";
         return false;  // unknown key type
       }
@@ -773,6 +777,12 @@ bool Curve::LoadFromBinaryData(MemoryReadStream *stream) {
   return true;
 }
 
+void Curve::FreeAll() {
+  while (!keys_.empty()) {
+    keys_[0]->Destroy();
+  }
+}
+
 bool Curve::Set(o3d::RawData *raw_data) {
   if (!raw_data) {
     O3D_ERROR(service_locator()) << "data object is null";
diff --git a/o3d/core/cross/curve.h b/o3d/core/cross/curve.h
index 0502dac..855b42c 100644
--- a/o3d/core/cross/curve.h
+++ b/o3d/core/cross/curve.h
@@ -414,6 +414,9 @@ class Curve : public Function {
     }
   }
 
+  // Removes all the keys.
+  void FreeAll();
+
   // What to do for inputs before the first key
   Infinity pre_infinity_;
 
diff --git a/o3d/core/cross/skin.cc b/o3d/core/cross/skin.cc
index 99387ca..7a29109 100644
--- a/o3d/core/cross/skin.cc
+++ b/o3d/core/cross/skin.cc
@@ -578,6 +578,7 @@ bool Skin::LoadFromBinaryData(MemoryReadStream *stream) {
   while (!stream->EndOfStream()) {
     // Make sure stream has a uint32 to read (for num_influences)
     if (stream->GetRemainingByteCount() < sizeof(uint32)) {
+      FreeAll();  // We have to call this before O3D_ERROR.
       O3D_ERROR(service_locator()) << "unexpected end of skin data";
       return false;
     }
@@ -588,6 +589,7 @@ bool Skin::LoadFromBinaryData(MemoryReadStream *stream) {
     const size_t kInfluenceSize = sizeof(uint32) + sizeof(float);
     size_t data_size = num_influences * kInfluenceSize;
     if (stream->GetRemainingByteCount() < data_size) {
+      FreeAll();  // We have to call this before O3D_ERROR.
       O3D_ERROR(service_locator()) << "unexpected end of skin data";
       return false;
     }
@@ -608,6 +610,12 @@ bool Skin::LoadFromBinaryData(MemoryReadStream *stream) {
   return true;
 }
 
+void Skin::FreeAll() {
+  influences_array_.clear();
+  inverse_bind_pose_matrices_.clear();
+  info_valid_ = false;
+}
+
 bool Skin::Set(o3d::RawData *raw_data) {
   if (!raw_data) {
     O3D_ERROR(service_locator()) << "data object is null";
diff --git a/o3d/core/cross/skin.h b/o3d/core/cross/skin.h
index 9e0c33f..70193db 100644
--- a/o3d/core/cross/skin.h
+++ b/o3d/core/cross/skin.h
@@ -152,6 +152,9 @@ class Skin : public NamedObject {
   // Update the highest influences and highest matrix index.
   void UpdateInfo() const;
 
+  // Frees all the influences and matrices.
+  void FreeAll();
+
   // The vertex influences.
   InfluencesArray influences_array_;