C++ fix for ICE-4930, invalid 1.0 marhsaling of pending objects

9 files changed, 209 insertions, 97 deletions

diff --git a/cpp/include/Ice/BasicStream.h b/cpp/include/Ice/BasicStream.h
index b376d4b5b39..f81448087d8 100644
--- a/cpp/include/Ice/BasicStream.h
+++ b/cpp/include/Ice/BasicStream.h
@@ -369,20 +369,8 @@ public:
         _currentReadEncaps->decoder->skipSlice();
     }
 
-    void readPendingObjects()
-    {
-        if(_currentReadEncaps && _currentReadEncaps->decoder)
-        {
-            _currentReadEncaps->decoder->readPendingObjects();
-        }
-    }
-    void writePendingObjects()
-    {
-        if(_currentWriteEncaps && _currentWriteEncaps->encoder)
-        {
-            _currentWriteEncaps->encoder->writePendingObjects();
-        }
-    }
+    void readPendingObjects();
+    void writePendingObjects();
 
     void writeSize(Ice::Int v) // Inlined for performance reasons.
     {
@@ -802,16 +790,8 @@ public:
     void writeEnum(Ice::Int, Ice::Int);
 
     // Exception
-    void writeException(const Ice::UserException& e)
-    {
-        initWriteEncaps();
-        _currentWriteEncaps->encoder->write(e);
-    }
-    void throwException(const UserExceptionFactoryPtr& factory = 0)
-    {
-        initReadEncaps();
-        _currentReadEncaps->decoder->throwException(factory);
-    }
+    void writeException(const Ice::UserException&);
+    void throwException(const UserExceptionFactoryPtr& = 0);
 
     void sliceObjects(bool);
 
@@ -909,7 +889,7 @@ private:
     public:
         EncapsDecoder(BasicStream* stream, ReadEncaps* encaps, bool sliceObjects) :
             _stream(stream), _encaps(encaps), _sliceObjects(sliceObjects), _traceSlicing(-1), _sliceType(NoSlice),
-            _usesClasses(false), _typeIdIndex(0)
+            _typeIdIndex(0)
         {
         } 
 
@@ -960,7 +940,6 @@ private:
         bool _skipFirstSlice;
         Ice::SliceInfoSeq _slices;          // Preserved slices.
         IndexListList _indirectionTables;   // Indirection tables for the preserved slices.
-        bool _usesClasses;
 
         // Slice attributes
         Ice::Byte _sliceFlags;
@@ -979,7 +958,7 @@ private:
     {
     public:
         EncapsEncoder(BasicStream* stream, WriteEncaps* encaps) : 
-            _stream(stream), _encaps(encaps), _sliceType(NoSlice), _usesClasses(false), _objectIdIndex(0), 
+            _stream(stream), _encaps(encaps), _sliceType(NoSlice), _objectIdIndex(0), 
             _typeIdIndex(0)
         {
         }
@@ -1030,8 +1009,6 @@ private:
         // Object/exception attributes
         SliceType _sliceType;
         bool _firstSlice;
-        bool _usesClasses;
-        Container::size_type _usesClassesPos;
 
         // Slice attributes
         Ice::Byte _sliceFlags;
diff --git a/cpp/include/Ice/Exception.h b/cpp/include/Ice/Exception.h
index 413bdd34bba..1a568c44e9b 100644
--- a/cpp/include/Ice/Exception.h
+++ b/cpp/include/Ice/Exception.h
@@ -64,7 +64,6 @@ public:
     virtual void __read(const InputStreamPtr&);
   
     virtual bool __usesClasses() const;
-    virtual void __usesClasses(bool);
 
 protected:
 
diff --git a/cpp/src/Ice/BasicStream.cpp b/cpp/src/Ice/BasicStream.cpp
index a86352dc009..1f4e126a879 100644
--- a/cpp/src/Ice/BasicStream.cpp
+++ b/cpp/src/Ice/BasicStream.cpp
@@ -256,6 +256,55 @@ IceInternal::BasicStream::skipEncaps()
     return encoding;
 }
 
+void
+IceInternal::BasicStream::readPendingObjects()
+{
+    if(_currentReadEncaps && _currentReadEncaps->decoder)
+    {
+        _currentReadEncaps->decoder->readPendingObjects();
+        delete _currentReadEncaps->decoder;
+        _currentReadEncaps->decoder = 0;
+    }
+    else if(getReadEncoding() == Ice::Encoding_1_0)
+    {
+        //
+        // If using the 1.0 encoding and no objects were read, we
+        // still read an empty sequence of pending objects if
+        // requested (i.e.: if this is called).
+        //
+        // This is required by the 1.0 encoding, even if no objects
+        // are written we do marshal an empty sequence if marshaled
+        // data types use classes.
+        //
+        skipSize();
+    }
+}
+
+void
+IceInternal::BasicStream::writePendingObjects()
+{
+    if(_currentWriteEncaps && _currentWriteEncaps->encoder)
+    {
+        _currentWriteEncaps->encoder->writePendingObjects();
+        delete _currentWriteEncaps->encoder;
+        _currentWriteEncaps->encoder = 0;
+    }
+    else if(getWriteEncoding() == Ice::Encoding_1_0)
+    {
+        //
+        // If using the 1.0 encoding and no objects were written, we
+        // still write an empty sequence for pending objects if
+        // requested (i.e.: if this is called).
+        // 
+        // This is required by the 1.0 encoding, even if no objects
+        // are written we do marshal an empty sequence if marshaled
+        // data types use classes.
+        //
+        writeSize(0);
+    }
+}
+
+
 Int
 IceInternal::BasicStream::readAndCheckSeqSize(int minSize)
 {
@@ -1565,12 +1614,45 @@ IceInternal::BasicStream::writeEnum(Int v, Int maxValue)
 }
 
 void
+IceInternal::BasicStream::writeException(const Ice::UserException& e)
+{
+    initWriteEncaps();
+    _currentWriteEncaps->encoder->write(e);
+
+    //
+    // Reset the encoder, the writing of the exception wrote
+    // pending objects if any.
+    //
+    delete _currentWriteEncaps->encoder;
+    _currentWriteEncaps->encoder = 0;
+}
+
+void
+IceInternal::BasicStream::throwException(const UserExceptionFactoryPtr& factory)
+{
+    initReadEncaps();
+    try
+    {
+        _currentReadEncaps->decoder->throwException(factory);
+    }
+    catch(const Ice::UserException&)
+    {
+        //
+        // Reset the decoder, the reading of the exception wrote
+        // pending objects if any.
+        //
+        delete _currentReadEncaps->decoder;
+        _currentReadEncaps->decoder = 0;
+        throw;
+    }
+}
+
+void
 IceInternal::BasicStream::sliceObjects(bool doSlice)
 {
     _sliceObjects = doSlice;
 }
 
-
 bool
 IceInternal::BasicStream::readOptImpl(Int readTag, OptionalFormat expectedFormat)
 {
@@ -1787,7 +1869,6 @@ IceInternal::BasicStream::EncapsEncoder::write(const ObjectPtr& v)
             // Object references are encoded as a negative integer in 1.0.
             //
             _stream->write(-index);
-            _usesClasses = true;
         }
         else if(_sliceType != NoSlice && _encaps->format == SlicedFormat)
         {
@@ -1828,7 +1909,6 @@ IceInternal::BasicStream::EncapsEncoder::write(const ObjectPtr& v)
         if(_encaps->encoding == Encoding_1_0)
         {
             _stream->write(0);
-            _usesClasses = true;
         }
         else
         {
@@ -1840,8 +1920,31 @@ IceInternal::BasicStream::EncapsEncoder::write(const ObjectPtr& v)
 void
 IceInternal::BasicStream::EncapsEncoder::write(const UserException& v)
 {
+    //
+    // User exception with the 1.0 encoding start with a boolean
+    // flag that indicates whether or not the exception uses
+    // classes. 
+    //
+    // This allows reading the pending objects even if some part of
+    // the exception was sliced. With encoding > 1.0, we don't need
+    // this, each slice indirect patch table indicates the presence of
+    // objects.
+    //
+    bool usesClasses;
+    if(_encaps->encoding == Encoding_1_0)
+    {
+        usesClasses = v.__usesClasses();
+        _stream->write(usesClasses);
+    }
+    else
+    {
+        usesClasses = true; // Always call writePendingObjects
+    }
     v.__write(_stream);
-    writePendingObjects();
+    if(usesClasses)
+    {
+        writePendingObjects();
+    }
 }
 
 void
@@ -1875,11 +1978,6 @@ IceInternal::BasicStream::EncapsEncoder::startException(const SlicedDataPtr& dat
 {
     _sliceType = ExceptionSlice;
     _firstSlice = true;
-    if(_encaps->encoding == Encoding_1_0)
-    {
-        _usesClassesPos = _stream->b.size();
-        _stream->write(false); // Placeholder for usesClasses boolean
-    }
     if(data)
     {
         writeSlicedData(data);
@@ -1889,11 +1987,6 @@ IceInternal::BasicStream::EncapsEncoder::startException(const SlicedDataPtr& dat
 void 
 IceInternal::BasicStream::EncapsEncoder::endException()
 {
-    if(_encaps->encoding == Encoding_1_0)
-    {
-        Byte* dest = &(*(_stream->b.begin() + _usesClassesPos));
-        *dest = static_cast<Byte>(_usesClasses);
-    }
     _sliceType = NoSlice;
 }
 
@@ -2038,29 +2131,24 @@ void
 IceInternal::BasicStream::EncapsEncoder::writePendingObjects()
 {
     //
-    // With the 1.0 encoding, write pending objects if nil or non-nil
-    // references were written (_usesClasses = true). Otherwise, write
-    // pending objects only if some non-nil references were written.
+    // With the 1.0 encoding, write pending objects if the marshalled
+    // data uses classes. Otherwise with encoding > 1.0, only write
+    // pending objects if some non-nil references were written.
     //
-    if(_encaps->encoding == Encoding_1_0)
+    if(_encaps->encoding != Encoding_1_0)
     {
-        if(!_usesClasses)
+        if(_toBeMarshaledMap.empty())
         {
             return;
         }
-        _usesClasses = false;
-    }
-    else if(_toBeMarshaledMap.empty())
-    {
-        return;
-    }
-    else 
-    {
-        //
-        // Write end marker for encapsulation optionals before encoding 
-        // the pending objects.
-        //
-        _stream->write(static_cast<Byte>(OptionalFormatEndMarker));
+        else 
+        {
+            //
+            // Write end marker for encapsulation optionals before encoding 
+            // the pending objects.
+            //
+            _stream->write(static_cast<Byte>(OptionalFormatEndMarker));
+        }
     }
 
     while(!_toBeMarshaledMap.empty())
@@ -2198,7 +2286,6 @@ IceInternal::BasicStream::EncapsDecoder::read(PatchFunc patchFunc, void* patchAd
         // Object references are encoded as a negative integer in 1.0.
         //
         _stream->read(index);
-        _usesClasses = true;
         if(index > 0)
         {
             throw MarshalException(__FILE__, __LINE__, "invalid object id");
@@ -2250,19 +2337,23 @@ IceInternal::BasicStream::EncapsDecoder::throwException(const UserExceptionFacto
 {
     assert(_sliceType == NoSlice);
 
+    //
+    // User exception with the 1.0 encoding start with a boolean flag
+    // that indicates whether or not the exception has classes.
+    //
+    // This allows reading the pending objects even if some part of
+    // the exception was sliced. With encoding > 1.0, we don't need
+    // this, each slice indirect patch table indicates the presence of
+    // objects.
+    //
+    bool usesClasses;
     if(_encaps->encoding == Encoding_1_0)
     {
-        //
-        // User exception with the 1.0 encoding start with a boolean
-        // flag that indicates whether or not the exception has
-        // classes. This allows reading the pending objects even if
-        // some the exception was sliced. With encoding > 1.0, we
-        // don't need this, each slice indirect patch table indicates
-        // the presence of objects.
-        //
-        bool usesClasses;
         _stream->read(usesClasses);
-        _usesClasses |= usesClasses;
+    }
+    else
+    {
+        usesClasses = true; // Always call readPendingObjects.
     }
 
     _sliceType = ExceptionSlice;
@@ -2301,7 +2392,10 @@ IceInternal::BasicStream::EncapsDecoder::throwException(const UserExceptionFacto
             catch(UserException& ex)
             {
                 ex.__read(_stream);
-                readPendingObjects();
+                if(usesClasses)
+                {
+                    readPendingObjects();
+                }
                 throw;
         
                 // Never reached.
@@ -2576,29 +2670,24 @@ void
 IceInternal::BasicStream::EncapsDecoder::readPendingObjects()
 {
     //
-    // With the 1.0 encoding, read pending objects if nil or non-nil
-    // references were read (_usesClasses == true). Otherwise, read
-    // pending objects only if some non-nil references were read.
+    // With the 1.0 encoding, we read pending objects if the marshaled
+    // data uses classes. Otherwise, only read pending objects if some
+    // non-nil references were read.
     //
-    if(_encaps->encoding == Encoding_1_0)
+    if(_encaps->encoding != Encoding_1_0)
     {        
-        if(!_usesClasses)
+        if(_patchMap.empty())
         {
             return;
         }
-        _usesClasses = false;
-    }
-    else if(_patchMap.empty())
-    {
-        return;
-    }
-    else
-    {
-        //
-        // Read unread encapsulation optionals before reading the
-        // pending objects.
-        //
-        _stream->skipOpts();
+        else
+        {
+            //
+            // Read unread encapsulation optionals before reading the
+            // pending objects.
+            //
+            _stream->skipOpts();
+        }
     }
 
     Int num;
diff --git a/cpp/src/Ice/Exception.cpp b/cpp/src/Ice/Exception.cpp
index 91b1377230b..177eb58e8a0 100644
--- a/cpp/src/Ice/Exception.cpp
+++ b/cpp/src/Ice/Exception.cpp
@@ -137,11 +137,6 @@ Ice::UserException::__usesClasses() const
     return false;
 }
 
-void
-Ice::UserException::__usesClasses(bool)
-{
-}
-
 Ice::LocalException::LocalException(const char* file, int line) :
     Exception(file, line)
 {
diff --git a/cpp/src/slice2cpp/Gen.cpp b/cpp/src/slice2cpp/Gen.cpp
index 6ba84c8e6b4..ac7058fc64b 100644
--- a/cpp/src/slice2cpp/Gen.cpp
+++ b/cpp/src/slice2cpp/Gen.cpp
@@ -877,6 +877,20 @@ Slice::Gen::TypesVisitor::visitExceptionStart(const ExceptionPtr& p)
     C << nl << "throw *this;";
     C << eb;
 
+    if(!p->isLocal() && p->usesClasses())
+    {
+        if(!base || (base && !base->usesClasses()))
+        {
+            H << sp << nl << "virtual bool __usesClasses() const;";
+
+            C << sp << nl << "bool";
+            C << nl << scoped.substr(2) << "::__usesClasses() const";
+            C << sb;
+            C << nl << "return true;";
+            C << eb;
+        }
+    }
+
     if(!dataMembers.empty())
     {
         H << sp;
@@ -2156,6 +2170,10 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
     {
         C << nl << "::IceInternal::BasicStream* __os = __result->__startWriteParams(" << opFormatTypeToString(p) <<");";
         writeMarshalCode(C, inParams, 0, TypeContextInParam);
+        if(p->sendsClasses())
+        {
+            C << nl << "__os->writePendingObjects();";
+        }
         C << nl << "__result->__endWriteParams();";
     }
     C << nl << "__result->__send(true);";
@@ -2703,6 +2721,10 @@ Slice::Gen::DelegateMVisitor::visitOperation(const OperationPtr& p)
         C << sb;
         C << nl<< "::IceInternal::BasicStream* __os = __og.startWriteParams(" << opFormatTypeToString(p) << ");";
         writeMarshalCode(C, inParams, 0, TypeContextInParam);
+        if(p->sendsClasses())
+        {
+            C << nl << "__os->writePendingObjects();";
+        }
         C << nl << "__og.endWriteParams();";
         C << eb;
         C << nl << "catch(const ::Ice::LocalException& __ex)";
@@ -4262,6 +4284,10 @@ Slice::Gen::ObjectVisitor::visitOperation(const OperationPtr& p)
                 C << nl << "::IceInternal::BasicStream* __os = __inS.__startWriteParams("
                   << opFormatTypeToString(p) << ");";
                 writeMarshalCode(C, outParams, p);
+                if(p->returnsClasses())
+                {
+                    C << nl << "__os->writePendingObjects();";
+                }
                 C << nl << "__inS.__endWriteParams(true);";
             }
             else
@@ -5883,6 +5909,10 @@ Slice::Gen::AsyncImplVisitor::visitOperation(const OperationPtr& p)
         C << sb;
         C << nl << "::IceInternal::BasicStream* __os = __startWriteParams(" << opFormatTypeToString(p) << ");";
         writeMarshalCode(C, outParams, p, TypeContextInParam);
+        if(p->returnsClasses())
+        {
+            C << nl << "__os->writePendingObjects();";
+        }
         C << nl << "__endWriteParams(true);";
         C << eb;
         C << nl << "catch(const ::Ice::Exception& __ex)";
diff --git a/cpp/test/Ice/objects/AllTests.cpp b/cpp/test/Ice/objects/AllTests.cpp
index 0acf1cc340d..c23a6974bc0 100644
--- a/cpp/test/Ice/objects/AllTests.cpp
+++ b/cpp/test/Ice/objects/AllTests.cpp
@@ -215,6 +215,15 @@ allTests(const Ice::CommunicatorPtr& communicator, bool collocated)
     initial->setI(h);
     cout << "ok" << endl;
 
+    cout << "testing sequences..." << flush;
+    BaseSeq inS, outS, retS;
+    retS = initial->opBaseSeq(inS, outS);
+
+    inS.resize(1);
+    inS[0] = new Base();
+    retS = initial->opBaseSeq(inS, outS);
+    cout << "ok" << endl;
+
     if(!collocated)
     {
         cout << "testing UnexpectedObjectException... " << flush;
diff --git a/cpp/test/Ice/objects/Test.ice b/cpp/test/Ice/objects/Test.ice
index 54e7df966f1..5768ef51606 100644
--- a/cpp/test/Ice/objects/Test.ice
+++ b/cpp/test/Ice/objects/Test.ice
@@ -91,6 +91,8 @@ class H implements I
 {
 };
 
+sequence<Base> BaseSeq;
+
 class Initial
 {
     void shutdown();
@@ -108,6 +110,8 @@ class Initial
     I getH();
 
     void setI(I theI);
+
+    BaseSeq opBaseSeq(BaseSeq inSeq, out BaseSeq outSeq);
 };
 
 class Empty
diff --git a/cpp/test/Ice/objects/TestI.cpp b/cpp/test/Ice/objects/TestI.cpp
index 70c325475bd..246864b2019 100644
--- a/cpp/test/Ice/objects/TestI.cpp
+++ b/cpp/test/Ice/objects/TestI.cpp
@@ -211,6 +211,13 @@ InitialI::setI(const IPtr&, const Ice::Current&)
 }
 
 
+BaseSeq
+InitialI::opBaseSeq(const BaseSeq& inSeq, BaseSeq& outSeq, const Ice::Current&)
+{
+    outSeq = inSeq;
+    return inSeq;
+}
+
 IPtr
 InitialI::getJ(const Ice::Current&)
 {
diff --git a/cpp/test/Ice/objects/TestI.h b/cpp/test/Ice/objects/TestI.h
index 70e0ebc8904..f91ff29e381 100644
--- a/cpp/test/Ice/objects/TestI.h
+++ b/cpp/test/Ice/objects/TestI.h
@@ -111,6 +111,8 @@ public:
 
     virtual void setI(const Test::IPtr&, const Ice::Current&);
 
+    virtual Test::BaseSeq opBaseSeq(const Test::BaseSeq&, Test::BaseSeq&, const Ice::Current&);
+
 private:
 
     Ice::ObjectAdapterPtr _adapter;