CHOP-900: Use new apr_procattr_constrain_handle_set() extension.

Now LLProcess explicitly requests APR to limit the handles passed to any child
process, instead of wantonly passing whatever happens to be lying around the
parent process at the time.
This requires the latest APR build.
Also revert LLUpdateDownloader::Implementation::mDownloadStream to llofstream
(as in rev 1878a57aebd7) instead of apr_file_t*. Using APR for that file was a
Band-Aid -- a single whacked mole -- for the problem more systemically
addressed by apr_procattr_constrain_handle_set().

3 files changed, 34 insertions, 27 deletions

diff --git a/autobuild.xml b/autobuild.xml
index 03e95ee9ab..a2d93a6c3e 100644
--- a/autobuild.xml
+++ b/autobuild.xml
@@ -90,9 +90,9 @@
             <key>archive</key>
             <map>
               <key>hash</key>
-              <string>8f186209cae5c7bf86d6af6a7965bea3</string>
+              <string>847f1b55c0549b7abe6628f1ea242b48</string>
               <key>url</key>
-              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/254439/arch/Darwin/installer/apr_suite-1.4.5-darwin-20120423.tar.bz2</string>
+              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/256201/arch/Darwin/installer/apr_suite-1.4.5-darwin-20120509.tar.bz2</string>
             </map>
             <key>name</key>
             <string>darwin</string>
@@ -102,9 +102,9 @@
             <key>archive</key>
             <map>
               <key>hash</key>
-              <string>f8743e0ccc4f651d5cfcf690e789cbe8</string>
+              <string>998ff5f7a5a9be8c0717e1a0eab05e0c</string>
               <key>url</key>
-              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/254439/arch/Linux/installer/apr_suite-1.4.5-linux-20120423.tar.bz2</string>
+              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/256201/arch/Linux/installer/apr_suite-1.4.5-linux-20120509.tar.bz2</string>
             </map>
             <key>name</key>
             <string>linux</string>
@@ -114,9 +114,9 @@
             <key>archive</key>
             <map>
               <key>hash</key>
-              <string>9799e268830010c519174da9a669dabe</string>
+              <string>44bc19a2491e8bd3ac4424c0b641f069</string>
               <key>url</key>
-              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/254439/arch/CYGWIN/installer/apr_suite-1.4.5-windows-20120423.tar.bz2</string>
+              <string>http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-apr/rev/256201/arch/CYGWIN/installer/apr_suite-1.4.5-windows-20120509.tar.bz2</string>
             </map>
             <key>name</key>
             <string>windows</string>
diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 760be6da9b..9667e4e033 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -539,6 +539,23 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
+	// IQA-490, CHOP-900: On Windows, ask APR to jump through hoops to
+	// constrain the set of handles passed to the child process. Before we
+	// changed to APR, the Windows implementation of LLProcessLauncher called
+	// CreateProcess(bInheritHandles=FALSE), meaning to pass NO open handles
+	// to the child process. Now that we support pipes, though, we must allow
+	// apr_proc_create() to pass bInheritHandles=TRUE. But without taking
+	// special pains, that causes trouble in a number of ways, due to the fact
+	// that the viewer is constantly opening and closing files -- most of
+	// which CreateProcess() passes to every child process!
+#if ! defined(APR_HAS_PROCATTR_CONSTRAIN_HANDLE_SET)
+	// Our special preprocessor symbol isn't even defined -- wrong APR
+	LL_WARNS("LLProcess") << "This version of APR lacks Linden "
+						  << "apr_procattr_constrain_handle_set() extension" << LL_ENDL;
+#else
+	chkapr(apr_procattr_constrain_handle_set(procattr, 1));
+#endif
+
 	// For which of stdin, stdout, stderr should we create a pipe to the
 	// child? In the viewer, there are only a couple viable
 	// apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx
diff --git a/indra/viewer_components/updater/llupdatedownloader.cpp b/indra/viewer_components/updater/llupdatedownloader.cpp
index abe45cc52b..75e455e3f6 100644
--- a/indra/viewer_components/updater/llupdatedownloader.cpp
+++ b/indra/viewer_components/updater/llupdatedownloader.cpp
@@ -40,7 +40,6 @@
 #include "llthread.h"
 #include "llupdaterservice.h"
 #include "llcurl.h"
-#include "llapr.h"
 
 class LLUpdateDownloader::Implementation:
 	public LLThread
@@ -66,7 +65,7 @@ private:
 	LLUpdateDownloader::Client & mClient;
 	CURL * mCurl;
 	LLSD mDownloadData;
-	apr_file_t* mDownloadStream;
+	llofstream mDownloadStream;
 	unsigned char mDownloadPercent;
 	std::string mDownloadRecordPath;
 	curl_slist * mHeaderList;
@@ -189,7 +188,6 @@ LLUpdateDownloader::Implementation::Implementation(LLUpdateDownloader::Client &
 	mCancelled(false),
 	mClient(client),
 	mCurl(0),
-	mDownloadStream(NULL),
 	mDownloadPercent(0),
 	mHeaderList(0)
 {
@@ -346,9 +344,12 @@ size_t LLUpdateDownloader::Implementation::onBody(void * buffer, size_t size)
 	if(mCancelled) return 0; // Forces a write error which will halt curl thread.
 	if((size == 0) || (buffer == 0)) return 0;
 
-	apr_size_t written(size);
-	ll_apr_assert_status(apr_file_write(mDownloadStream, buffer, &written));
-	return written;
+	mDownloadStream.write(static_cast<const char *>(buffer), size);
+	if(mDownloadStream.bad()) {
+		return 0;
+	} else {
+		return size;
+	}
 }
 
 
@@ -379,8 +380,7 @@ int LLUpdateDownloader::Implementation::onProgress(double downloadSize, double b
 void LLUpdateDownloader::Implementation::run(void)
 {
 	CURLcode code = curl_easy_perform(mCurl);
-	ll_apr_assert_status(apr_file_close(mDownloadStream));
-	mDownloadStream = NULL;
+	mDownloadStream.close();
 	if(code == CURLE_OK) {
 		LLFile::remove(mDownloadRecordPath);
 		if(validateDownload()) {
@@ -459,9 +459,8 @@ void LLUpdateDownloader::Implementation::resumeDownloading(size_t startByte)
 	if(mHeaderList == 0) throw DownloadError("cannot add Range header");
 	throwOnCurlError(curl_easy_setopt(mCurl, CURLOPT_HTTPHEADER, mHeaderList));
 
-	ll_apr_assert_status(apr_file_open(&mDownloadStream, mDownloadData["path"].asString().c_str(),
-									   APR_WRITE | APR_APPEND | APR_BINARY | APR_BUFFERED,
-									   APR_OS_DEFAULT, gAPRPoolp));
+	mDownloadStream.open(mDownloadData["path"].asString(),
+						 std::ios_base::out | std::ios_base::binary | std::ios_base::app);
 	start();
 }
 
@@ -484,16 +483,7 @@ void LLUpdateDownloader::Implementation::startDownloading(LLURI const & uri, std
 	llofstream dataStream(mDownloadRecordPath);
 	LLSDSerialize::toPrettyXML(mDownloadData, dataStream);
 
-	ll_apr_assert_status(apr_file_open(&mDownloadStream, filePath.c_str(),
-									   APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BINARY | APR_BUFFERED,
-									   APR_OS_DEFAULT, gAPRPoolp));
-	// IQA-463: Do NOT let this open file be inherited by child processes.
-	// That's why we switched from llofstream to apr_file_t. From
-	// apr_file_open() doc
-	// http://apr.apache.org/docs/apr/1.4/group__apr__file__io.html#gabda14cbf242fb4fe99055434213e5446 :
-	// "By default, the returned file descriptor will not be inherited by
-	// child processes created by apr_proc_create(). This can be changed using
-	// apr_file_inherit_set()."
+	mDownloadStream.open(filePath, std::ios_base::out | std::ios_base::binary);
 	initializeCurlGet(uri.asString(), true);
 	start();
 }