Merge pull request #1639 from secondlife/nat/stack-overflow

Make LLLeap iterate over child stdout while read data is pending

1 files changed, 108 insertions, 112 deletions

diff --git a/indra/llcommon/llleap.cpp b/indra/llcommon/llleap.cpp
index d0fb586459..e93ba83434 100644
--- a/indra/llcommon/llleap.cpp
+++ b/indra/llcommon/llleap.cpp
@@ -18,9 +18,6 @@
 #include <algorithm>
 // std headers
 // external library headers
-#include <boost/bind.hpp>
-#include <boost/scoped_ptr.hpp>
-#include <boost/tokenizer.hpp>
 // other Linden headers
 #include "llerror.h"
 #include "llstring.h"
@@ -64,7 +61,9 @@ public:
         // Pass it a callback to our connect() method, so it can send events
         // from a particular LLEventPump to the plugin without having to know
         // this class or method name.
-        mListener(new LLLeapListener(boost::bind(&LLLeapImpl::connect, this, _1, _2)))
+        mListener(new LLLeapListener(
+                      [this](LLEventPump& pump, const std::string& listener)
+                      { return connect(pump, listener); }))
     {
         // Rule out unpopulated Params block
         if (! cparams.executable.isProvided())
@@ -93,7 +92,7 @@ public:
         }
 
         // Listen for child "termination" right away to catch launch errors.
-        mDonePump.listen("LLLeap", boost::bind(&LLLeapImpl::bad_launch, this, _1));
+        mDonePump.listen("LLLeap", [this](const LLSD& data){ return bad_launch(data); });
 
         // Okay, launch child.
         // Get a modifiable copy of params block to set files and postend.
@@ -113,7 +112,7 @@ public:
 
         // Okay, launch apparently worked. Change our mDonePump listener.
         mDonePump.stopListening("LLLeap");
-        mDonePump.listen("LLLeap", boost::bind(&LLLeapImpl::done, this, _1));
+        mDonePump.listen("LLLeap", [this](const LLSD& data){ return done(data); });
 
         // Child might pump large volumes of data through either stdout or
         // stderr. Don't bother copying all that data into notification event.
@@ -128,13 +127,9 @@ public:
 
         // Listening on stdout is stateful. In general, we're either waiting
         // for the length prefix or waiting for the specified length of data.
-        // We address that with two different listener methods -- one of which
-        // is blocked at any given time.
+        mReadPrefix = true;
         mStdoutConnection = childout.getPump()
-            .listen("prefix", boost::bind(&LLLeapImpl::rstdout, this, _1));
-        mStdoutDataConnection = childout.getPump()
-            .listen("data",   boost::bind(&LLLeapImpl::rstdoutData, this, _1));
-        mBlocker.reset(new LLEventPump::Blocker(mStdoutDataConnection));
+            .listen("LLLeap", [this](const LLSD& data){ return rstdout(data); });
 
         // Log anything sent up through stderr. When a typical program
         // encounters an error, it writes its error message to stderr and
@@ -142,7 +137,7 @@ public:
         // interpreter behaves that way. More generally, though, a plugin
         // author can log whatever s/he wants to the viewer log using stderr.
         mStderrConnection = childerr.getPump()
-            .listen("LLLeap", boost::bind(&LLLeapImpl::rstderr, this, _1));
+            .listen("LLLeap", [this](const LLSD& data){ return rstderr(data); });
 
         // For our lifespan, intercept any LL_ERRS so we can notify plugin
         mRecorder = LLError::addGenericRecorder(
@@ -255,120 +250,120 @@ public:
         return false;
     }
 
-    // Initial state of stateful listening on child stdout: wait for a length
-    // prefix, followed by ':'.
-    bool rstdout(const LLSD& data)
+    // Stateful listening on child stdout:
+    // wait for a length prefix, followed by ':'.
+    bool rstdout(const LLSD&)
     {
         LLProcess::ReadPipe& childout(mChild->getReadPipe(LLProcess::STDOUT));
-        // It's possible we got notified of a couple digit characters without
-        // seeing the ':' -- unlikely, but still. Until we see ':', keep
-        // waiting.
-        if (childout.contains(':'))
+        while (childout.size())
         {
-            std::istream& childstream(childout.get_istream());
-            // Saw ':', read length prefix and store in mExpect.
-            size_t expect;
-            childstream >> expect;
-            int colon(childstream.get());
-            if (colon != ':')
+            /*----------------- waiting for length prefix ------------------*/
+            if (mReadPrefix)
             {
-                // Protocol failure. Clear out the rest of the pending data in
-                // childout (well, up to a max length) to log what was wrong.
-                LLProcess::ReadPipe::size_type
-                    readlen((std::min)(childout.size(), LLProcess::ReadPipe::size_type(80)));
-                bad_protocol(STRINGIZE(expect << char(colon) << childout.read(readlen)));
+                // It's possible we got notified of a couple digit characters without
+                // seeing the ':' -- unlikely, but still. Until we see ':', keep
+                // waiting.
+                if (! childout.contains(':'))
+                {
+                    if (childout.contains('\n'))
+                    {
+                        // Since this is the initial listening state, this is where we'd
+                        // arrive if the child isn't following protocol at all -- say
+                        // because the user specified 'ls' or some darn thing.
+                        bad_protocol(childout.getline());
+                    }
+                    // Either way, stop looping.
+                    break;
+                }
+
+                // Saw ':', read length prefix and store in mExpect.
+                std::istream& childstream(childout.get_istream());
+                size_t expect;
+                childstream >> expect;
+                int colon(childstream.get());
+                if (colon != ':')
+                {
+                    // Protocol failure. Clear out the rest of the pending data in
+                    // childout (well, up to a max length) to log what was wrong.
+                    LLProcess::ReadPipe::size_type
+                        readlen((std::min)(childout.size(),
+                                           LLProcess::ReadPipe::size_type(80)));
+                    bad_protocol(stringize(expect, char(colon), childout.read(readlen)));
+                    break;
+                }
+                else
+                {
+                    // Saw length prefix, saw colon, life is good. Now wait for
+                    // that length of data to arrive.
+                    mExpect = expect;
+                    LL_DEBUGS("LLLeap") << "got length, waiting for "
+                                        << mExpect << " bytes of data" << LL_ENDL;
+                    // Transition to "read data" mode and loop back to check
+                    // if we've already received all the advertised data.
+                    mReadPrefix = false;
+                    continue;
+                }
             }
+            /*----------------- saw prefix, wait for data ------------------*/
             else
             {
-                // Saw length prefix, saw colon, life is good. Now wait for
-                // that length of data to arrive.
-                mExpect = expect;
-                LL_DEBUGS("LLLeap") << "got length, waiting for "
-                                    << mExpect << " bytes of data" << LL_ENDL;
-                // Block calls to this method; resetting mBlocker unblocks
-                // calls to the other method.
-                mBlocker.reset(new LLEventPump::Blocker(mStdoutConnection));
-                // Go check if we've already received all the advertised data.
-                if (childout.size())
+                // Until we've accumulated the promised length of data, keep waiting.
+                if (childout.size() < mExpect)
                 {
-                    LLSD updata(data);
-                    updata["len"] = LLSD::Integer(childout.size());
-                    rstdoutData(updata);
+                    break;
                 }
-            }
-        }
-        else if (childout.contains('\n'))
-        {
-            // Since this is the initial listening state, this is where we'd
-            // arrive if the child isn't following protocol at all -- say
-            // because the user specified 'ls' or some darn thing.
-            bad_protocol(childout.getline());
-        }
-        return false;
-    }
 
-    // State in which we listen on stdout for the specified length of data to
-    // arrive.
-    bool rstdoutData(const LLSD& data)
-    {
-        LLProcess::ReadPipe& childout(mChild->getReadPipe(LLProcess::STDOUT));
-        // Until we've accumulated the promised length of data, keep waiting.
-        if (childout.size() >= mExpect)
-        {
-            // Ready to rock and roll.
-            LL_DEBUGS("LLLeap") << "needed " << mExpect << " bytes, got "
-                                << childout.size() << ", parsing LLSD" << LL_ENDL;
-            LLSD data;
+                // We have the data we were told to expect! Ready to rock and roll.
+                LL_DEBUGS("LLLeap") << "needed " << mExpect << " bytes, got "
+                                    << childout.size() << ", parsing LLSD" << LL_ENDL;
+                LLSD data;
 #if 1
-            // specifically require notation LLSD from child
-            LLPointer<LLSDParser> parser(new LLSDNotationParser());
-            S32 parse_status(parser->parse(childout.get_istream(), data, mExpect));
-            if (parse_status == LLSDParser::PARSE_FAILURE)
+                // specifically require notation LLSD from child
+                LLPointer<LLSDParser> parser(new LLSDNotationParser());
+                S32 parse_status(parser->parse(childout.get_istream(), data, mExpect));
+                if (parse_status == LLSDParser::PARSE_FAILURE)
 #else
-            // SL-18330: accept any valid LLSD serialization format from child
-            // Unfortunately this runs into trouble we have not yet debugged.
-            bool parse_status(LLSDSerialize::deserialize(data, childout.get_istream(), mExpect));
-            if (! parse_status)
+                // SL-18330: accept any valid LLSD serialization format from child
+                // Unfortunately this runs into trouble we have not yet debugged.
+                bool parse_status(LLSDSerialize::deserialize(data, childout.get_istream(), mExpect));
+                if (! parse_status)
 #endif
-            {
-                bad_protocol("unparseable LLSD data");
-            }
-            else if (! (data.isMap() && data["pump"].isString() && data.has("data")))
-            {
-                // we got an LLSD object, but it lacks required keys
-                bad_protocol("missing 'pump' or 'data'");
-            }
-            else
-            {
-                try
                 {
-                    // The LLSD object we got from our stream contains the
-                    // keys we need.
-                    LLEventPumps::instance().obtain(data["pump"]).post(data["data"]);
+                    bad_protocol("unparseable LLSD data");
+                    break;
                 }
-                catch (const std::exception& err)
+                else if (! (data.isMap() && data["pump"].isString() && data.has("data")))
                 {
-                    // No plugin should be allowed to crash the viewer by
-                    // driving an exception -- intentionally or not.
-                    LOG_UNHANDLED_EXCEPTION(stringize("handling request ", data));
-                    // Whether or not the plugin added a "reply" key to the
-                    // request, send a reply. We happen to know who originated
-                    // this request, and the reply LLEventPump of interest.
-                    // Not our problem if the plugin ignores the reply event.
-                    data["reply"] = mReplyPump.getName();
-                    sendReply(llsd::map("error",
-                                        stringize(LLError::Log::classname(err), ": ", err.what())),
-                              data);
+                    // we got an LLSD object, but it lacks required keys
+                    bad_protocol("missing 'pump' or 'data'");
+                    break;
                 }
-                // Block calls to this method; resetting mBlocker unblocks
-                // calls to the other method.
-                mBlocker.reset(new LLEventPump::Blocker(mStdoutDataConnection));
-                // Go check for any more pending events in the buffer.
-                if (childout.size())
+                else
                 {
-                    LLSD updata(data);
-                    data["len"] = LLSD::Integer(childout.size());
-                    rstdout(updata);
+                    try
+                    {
+                        // The LLSD object we got from our stream contains the
+                        // keys we need.
+                        LLEventPumps::instance().obtain(data["pump"]).post(data["data"]);
+                    }
+                    catch (const std::exception& err)
+                    {
+                        // No plugin should be allowed to crash the viewer by
+                        // driving an exception -- intentionally or not.
+                        LOG_UNHANDLED_EXCEPTION(stringize("handling request ", data));
+                        // Whether or not the plugin added a "reply" key to the
+                        // request, send a reply. We happen to know who originated
+                        // this request, and the reply LLEventPump of interest.
+                        // Not our problem if the plugin ignores the reply event.
+                        data["reply"] = mReplyPump.getName();
+                        sendReply(llsd::map("error",
+                                            stringize(LLError::Log::classname(err), ": ", err.what())),
+                                  data);
+                    }
+                    // Transition to "read prefix" mode and go check for any
+                    // more pending events in the buffer.
+                    mReadPrefix = true;
+                    continue;
                 }
             }
         }
@@ -453,7 +448,8 @@ private:
         // child's stdin, suitably enriched with the pump name on which it was
         // received.
         return pump.listen(listener,
-                           boost::bind(&LLLeapImpl::wstdin, this, pump.getName(), _1));
+                           [this, name=pump.getName()](const LLSD& data)
+                           { return wstdin(name, data); });
     }
 
     std::string mDesc;
@@ -461,11 +457,11 @@ private:
     LLEventStream mReplyPump;
     LLProcessPtr mChild;
     LLTempBoundListener
-        mStdinConnection, mStdoutConnection, mStdoutDataConnection, mStderrConnection;
-    std::unique_ptr<LLEventPump::Blocker> mBlocker;
+        mStdinConnection, mStdoutConnection, mStderrConnection;
     LLProcess::ReadPipe::size_type mExpect;
     LLError::RecorderPtr mRecorder;
     std::unique_ptr<LLLeapListener> mListener;
+    bool mReadPrefix;
 };
 
 // These must follow the declaration of LLLeapImpl, so they may as well be last.