Merged in andreyl_productengine/viewer-427

3 files changed, 123 insertions, 20 deletions

diff --git a/indra/llimage/llimage.cpp b/indra/llimage/llimage.cpp
index 91fa8c6ad1..feb97ec2ab 100644
--- a/indra/llimage/llimage.cpp
+++ b/indra/llimage/llimage.cpp
@@ -942,6 +942,12 @@ void LLImageRaw::clear(U8 r, U8 g, U8 b, U8 a)
 {
 	llassert( getComponents() <= 4 );
 	// This is fairly bogus, but it'll do for now.
+	if (isBufferInvalid())
+	{
+		LL_WARNS() << "Invalid image buffer" << LL_ENDL;
+		return;
+	}
+
 	U8 *pos = getData();
 	U32 x, y;
 	for (x = 0; x < getWidth(); x++)
@@ -1069,6 +1075,11 @@ void LLImageRaw::composite( LLImageRaw* src )
 {
 	LLImageRaw* dst = this;  // Just for clarity.
 
+	if (!validateSrcAndDst("LLImageRaw::composite", src, dst))
+	{
+		return;
+	}
+
 	llassert(3 == src->getComponents());
 	llassert(3 == dst->getComponents());
 
@@ -1136,7 +1147,6 @@ void LLImageRaw::compositeUnscaled4onto3( LLImageRaw* src )
 	llassert( (3 == src->getComponents()) || (4 == src->getComponents()) );
 	llassert( (src->getWidth() == dst->getWidth()) && (src->getHeight() == dst->getHeight()) );
 
-
 	U8* src_data = src->getData();
 	U8* dst_data = dst->getData();
 	S32 pixels = getWidth() * getHeight();
@@ -1171,6 +1181,11 @@ void LLImageRaw::copyUnscaledAlphaMask( LLImageRaw* src, const LLColor4U& fill)
 {
 	LLImageRaw* dst = this;  // Just for clarity.
 
+	if (!validateSrcAndDst("LLImageRaw::copyUnscaledAlphaMask", src, dst))
+	{
+		return;
+	}
+
 	llassert( 1 == src->getComponents() );
 	llassert( 4 == dst->getComponents() );
 	llassert( (src->getWidth() == dst->getWidth()) && (src->getHeight() == dst->getHeight()) );
@@ -1193,6 +1208,12 @@ void LLImageRaw::copyUnscaledAlphaMask( LLImageRaw* src, const LLColor4U& fill)
 // Fill the buffer with a constant color
 void LLImageRaw::fill( const LLColor4U& color )
 {
+	if (isBufferInvalid())
+	{
+		LL_WARNS() << "Invalid image buffer" << LL_ENDL;
+		return;
+	}
+
 	S32 pixels = getWidth() * getHeight();
 	if( 4 == getComponents() )
 	{
@@ -1231,14 +1252,13 @@ LLPointer<LLImageRaw> LLImageRaw::duplicate()
 // Src and dst can be any size.  Src and dst can each have 3 or 4 components.
 void LLImageRaw::copy(LLImageRaw* src)
 {
-	if (!src)
+	LLImageRaw* dst = this;  // Just for clarity.
+
+	if (!validateSrcAndDst("LLImageRaw::copy", src, dst))
 	{
-		LL_WARNS() << "LLImageRaw::copy called with a null src pointer" << LL_ENDL;
 		return;
 	}
 
-	LLImageRaw* dst = this;  // Just for clarity.
-
 	if( (src->getWidth() == dst->getWidth()) && (src->getHeight() == dst->getHeight()) )
 	{
 		// No scaling needed
@@ -1365,6 +1385,11 @@ void LLImageRaw::copyScaled( LLImageRaw* src )
 {
 	LLImageRaw* dst = this;  // Just for clarity.
 
+	if (!validateSrcAndDst("LLImageRaw::copyScaled", src, dst))
+	{
+		return;
+	}
+
 	llassert_always( (1 == src->getComponents()) || (3 == src->getComponents()) || (4 == src->getComponents()) );
 	llassert_always( src->getComponents() == dst->getComponents() );
 
@@ -1403,6 +1428,12 @@ bool LLImageRaw::scale( S32 new_width, S32 new_height, bool scale_image_data )
 {
 	llassert((1 == getComponents()) || (3 == getComponents()) || (4 == getComponents()) );
 
+	if (isBufferInvalid())
+	{
+		LL_WARNS() << "Invalid image buffer" << LL_ENDL;
+		return false;
+	}
+
 	S32 old_width = getWidth();
 	S32 old_height = getHeight();
 	
@@ -1692,6 +1723,25 @@ void LLImageRaw::compositeRowScaled4onto3( U8* in, U8* out, S32 in_pixel_len, S3
 	}
 }
 
+bool LLImageRaw::validateSrcAndDst(std::string func, LLImageRaw* src, LLImageRaw* dst)
+{
+	if (!src || !dst || src->isBufferInvalid() || dst->isBufferInvalid())
+	{
+		LL_WARNS() << func << ": Source: ";
+		if (!src) LL_CONT << "Null pointer";
+		else if (src->isBufferInvalid()) LL_CONT << "Invalid buffer";
+		else LL_CONT << "OK";
+
+		LL_CONT << "; Destination: ";
+		if (!dst) LL_CONT << "Null pointer";
+		else if (dst->isBufferInvalid()) LL_CONT << "Invalid buffer";
+		else LL_CONT << "OK";
+		LL_CONT << "." << LL_ENDL;
+
+		return false;
+	}
+	return true;
+}
 
 //----------------------------------------------------------------------------
 
diff --git a/indra/llimage/llimage.h b/indra/llimage/llimage.h
index adc650d360..9cc7431a9c 100644
--- a/indra/llimage/llimage.h
+++ b/indra/llimage/llimage.h
@@ -277,6 +277,9 @@ protected:
 public:
 	static S32 sGlobalRawMemory;
 	static S32 sRawImageCount;
+
+private:
+	bool validateSrcAndDst(std::string func, LLImageRaw* src, LLImageRaw* dst);
 };
 
 // Compressed representation of image.
diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp
index 9347e51b85..025c77b85e 100644
--- a/indra/llkdu/llimagej2ckdu.cpp
+++ b/indra/llkdu/llimagej2ckdu.cpp
@@ -31,9 +31,31 @@
 #include "llpointer.h"
 #include "llmath.h"
 #include "llkdumem.h"
+#include "stringize.h"
 
 #include "kdu_block_coding.h"
 
+#include <stdexcept>
+#include <iostream>
+
+namespace {
+// exception used to keep KDU from terminating entire program -- see comments
+// in LLKDUMessageError::flush()
+struct KDUError: public std::runtime_error
+{
+	KDUError(const std::string& msg): std::runtime_error(msg) {}
+};
+} // anonymous namespace
+
+// stream kdu_dims to std::ostream
+// Turns out this must NOT be in the anonymous namespace!
+inline
+std::ostream& operator<<(std::ostream& out, const kdu_dims& dims)
+{
+	return out << "(" << dims.pos.x << "," << dims.pos.y << "),"
+				  "[" << dims.size.x << "x" << dims.size.y << "]";
+}
+
 class kdc_flow_control {
 	
 public:
@@ -165,9 +187,15 @@ struct LLKDUMessageError : public LLKDUMessage
 		// terminating handler→flush call."
 		// So throwing an exception here isn't arbitrary: we MUST throw an
 		// exception if we want to recover from a KDU error.
+		// Because this confused me: the above quote specifically refers to
+		// the kdu_error class, which is constructed internally within KDU at
+		// the point where a fatal error is discovered and reported. It is NOT
+		// talking about the kdu_message subclass passed to
+		// kdu_customize_errors(). Destroying this static object at program
+		// shutdown will NOT engage the behavior described above.
 		if (end_of_message) 
 		{
-			throw "KDU throwing an exception";
+	 		throw KDUError("LLKDUMessageError::flush()");
 		}
 	}
 };
@@ -196,6 +224,10 @@ LLImageJ2CKDU::~LLImageJ2CKDU()
 // Stuff for new simple decode
 void transfer_bytes(kdu_byte *dest, kdu_line_buf &src, int gap, int precision);
 
+// This is called by the real (private) initDecode() (keep_codestream true)
+// and getMetadata() methods (keep_codestream false). As far as nat can tell,
+// mode is always MODE_FAST. It was called by findDiscardLevelsBoundaries()
+// as well, when that still existed, with keep_codestream true and MODE_FAST.
 void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECodeStreamMode mode)
 {
 	S32 data_size = base.getDataSize();
@@ -206,6 +238,12 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod
 	//
 	mCodeStreamp.reset();
 
+	// It's not clear to nat under what circumstances we would reuse a
+	// pre-existing LLKDUMemSource instance. As of 2016-08-05, it consists of
+	// two U32s and a pointer, so it's not as if it would be a huge overhead
+	// to allocate a new one every time.
+	// Also -- why is base.getData() tested specifically here? If that returns
+	// NULL, shouldn't we bail out of the whole method?
 	if (!mInputp && base.getData())
 	{
 		// The compressed data has been loaded
@@ -262,13 +300,19 @@ void LLImageJ2CKDU::setupCodeStream(LLImageJ2C &base, bool keep_codestream, ECod
 
 	S32 components = mCodeStreamp->get_num_components();
 
-	if (components >= 3)
-	{ // Check that components have consistent dimensions (for PPM file)
-		kdu_dims dims1; mCodeStreamp->get_dims(1,dims1);
-		kdu_dims dims2; mCodeStreamp->get_dims(2,dims2);
-		if ((dims1 != dims) || (dims2 != dims))
+	// Check that components have consistent dimensions (for PPM file)
+	for (int idx = 1; idx < components; ++idx)
+	{
+		kdu_dims other_dims;
+		mCodeStreamp->get_dims(idx, other_dims);
+		if (other_dims != dims)
 		{
-			LL_ERRS() << "Components don't have matching dimensions!" << LL_ENDL;
+			// This method is only called from methods that catch KDUError.
+			// We want to fail the image load, not crash the viewer.
+			throw KDUError(STRINGIZE("Component " << idx << " dimensions "
+									 << other_dims
+									 << " do not match component 0 dimensions "
+									 << dims << "!"));
 		}
 	}
 
@@ -295,6 +339,9 @@ void LLImageJ2CKDU::cleanupCodeStream()
 	mTileIndicesp.reset();
 }
 
+// This is the protected virtual method called by LLImageJ2C::initDecode().
+// However, as far as nat can tell, LLImageJ2C::initDecode() is called only by
+// llimage_libtest.cpp's load_image() function. No detectable production use.
 bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, int discard_level, int* region)
 {
 	return initDecode(base,raw_image,0.0f,MODE_FAST,0,4,discard_level,region);
@@ -327,6 +374,9 @@ bool LLImageJ2CKDU::initEncode(LLImageJ2C &base, LLImageRaw &raw_image, int bloc
 	return true;
 }
 
+// This is the real (private) initDecode() called both by the protected
+// initDecode() method and by decodeImpl(). As far as nat can tell, only the
+// decodeImpl() usage matters for production.
 bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, F32 decode_time, ECodeStreamMode mode, S32 first_channel, S32 max_channel_count, int discard_level, int* region)
 {
 	base.resetLastError();
@@ -384,9 +434,9 @@ bool LLImageJ2CKDU::initDecode(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco
 			mTPosp->x = 0;
 		}
 	}
-	catch (const char* msg)
+	catch (const KDUError& msg)
 	{
-		base.setLastError(ll_safe_string(msg));
+		base.setLastError(msg.what());
 		return false;
 	}
 	catch (...)
@@ -480,9 +530,9 @@ bool LLImageJ2CKDU::decodeImpl(LLImageJ2C &base, LLImageRaw &raw_image, F32 deco
 					return false;
 				}
 			}
-			catch (const char* msg)
+			catch (const KDUError& msg)
 			{
-				base.setLastError(ll_safe_string(msg));
+				base.setLastError(msg.what());
 				base.decodeFailed();
 				cleanupCodeStream();
 				return true; // done
@@ -673,9 +723,9 @@ bool LLImageJ2CKDU::encodeImpl(LLImageJ2C &base, const LLImageRaw &raw_image, co
 		base.updateData(); // set width, height
 		delete[] output_buffer;
 	}
-	catch(const char* msg)
+	catch(const KDUError& msg)
 	{
-		base.setLastError(ll_safe_string(msg));
+		base.setLastError(msg.what());
 		return false;
 	}
 	catch( ... )
@@ -697,9 +747,9 @@ bool LLImageJ2CKDU::getMetadata(LLImageJ2C &base)
 		setupCodeStream(base, false, MODE_FAST);
 		return true;
 	}
-	catch (const char* msg)
+	catch (const KDUError& msg)
 	{
-		base.setLastError(ll_safe_string(msg));
+		base.setLastError(msg.what());
 		return false;
 	}
 	catch (...)