From 65d608cdac701ab10f84bd8dc4a6c0be7eef26f1 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 5 Aug 2016 14:02:12 -0400
Subject: MAINT-6584: Clarify LLKDUMessageError::flush() throwing exception.
---
indra/llkdu/llimagej2ckdu.cpp | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp
index 9347e51b85..2adb90320c 100644
--- a/indra/llkdu/llimagej2ckdu.cpp
+++ b/indra/llkdu/llimagej2ckdu.cpp
@@ -165,6 +165,12 @@ 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";
--
cgit v1.2.3
From a5ce63eb3c4a3f3ea57ca2a353954a2afd43fc72 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 5 Aug 2016 17:44:13 -0400
Subject: MAINT-6584: Add explanatory comments to LLImageJ2CKDU implementation.
These comments are inherently fragile, in that they enumerate all present
callers of certain methods. Adding, removing or relocating calls would
invalidate these comments. However, the LLImageJ2CKDU implementation is
probably pretty stable by now.
---
indra/llkdu/llimagej2ckdu.cpp | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp
index 2adb90320c..1201170f6d 100644
--- a/indra/llkdu/llimagej2ckdu.cpp
+++ b/indra/llkdu/llimagej2ckdu.cpp
@@ -202,6 +202,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();
@@ -212,6 +216,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
@@ -301,6 +311,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);
@@ -333,6 +346,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();
--
cgit v1.2.3
From 80adc9b6aa9814c74fd0c67c5de9d1765a089925 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 5 Aug 2016 17:57:24 -0400
Subject: MAINT-6584: Introduce KDUError exception from other viewer project.
Specifically, manually apply changesets b4db8a8 and b98371d from
nat_linden/viewer-mac-mainloop. We need to throw from a new place, but if we
threw const char* (current convention), the new throw wouldn't be patched when
we merge to new exception convention.
---
indra/llkdu/llimagej2ckdu.cpp | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp
index 1201170f6d..ba1560ec56 100644
--- a/indra/llkdu/llimagej2ckdu.cpp
+++ b/indra/llkdu/llimagej2ckdu.cpp
@@ -34,6 +34,15 @@
#include "kdu_block_coding.h"
+#include <stdexcept>
+
+namespace {
+struct KDUError: public std::runtime_error
+{
+ KDUError(const std::string& msg): std::runtime_error(msg) {}
+};
+} // anonymous namespace
+
class kdc_flow_control {
public:
@@ -173,7 +182,7 @@ struct LLKDUMessageError : public LLKDUMessage
// shutdown will NOT engage the behavior described above.
if (end_of_message)
{
- throw "KDU throwing an exception";
+ throw KDUError("LLKDUMessageError::flush()");
}
}
};
@@ -406,9 +415,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 (...)
@@ -502,9 +511,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
@@ -695,9 +704,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( ... )
@@ -719,9 +728,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 (...)
--
cgit v1.2.3
From 1773f44b6d9fffa7f5d0b2449184b2998e99be5a Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Sat, 6 Aug 2016 11:39:11 -0400
Subject: MAINT-6584: Don't crash on inconsistent dims in a JPEG-2000 image.
Previous code would crump with LL_ERRS. But a bad image file should fail only
the image load -- not crash the viewer.
While at it, validate all components present, not just 0, 1, 2.
While at it, make the failure message report which component and what the
mismatched dimensions are, not just "Components don't have matching
dimensions!"
---
indra/llkdu/llimagej2ckdu.cpp | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/indra/llkdu/llimagej2ckdu.cpp b/indra/llkdu/llimagej2ckdu.cpp
index ba1560ec56..025c77b85e 100644
--- a/indra/llkdu/llimagej2ckdu.cpp
+++ b/indra/llkdu/llimagej2ckdu.cpp
@@ -31,18 +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) {}
+ 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:
@@ -287,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 << "!"));
}
}
--
cgit v1.2.3
From d4b1db277ca538781661bf474b733296156e5728 Mon Sep 17 00:00:00 2001
From: AndreyL ProductEngine <alihatskiy@productengine.com>
Date: Tue, 9 Aug 2016 05:07:39 +0300
Subject: MAINT-6618 Fixed the crash in LLImageRaw::scale() + some additional
checks
---
indra/llimage/llimage.cpp | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/indra/llimage/llimage.cpp b/indra/llimage/llimage.cpp
index 91fa8c6ad1..c7f02a6b1a 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++)
@@ -1136,7 +1142,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();
@@ -1193,6 +1198,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() )
{
@@ -1403,6 +1414,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();
--
cgit v1.2.3
From 5e02d304f03dd6adcb99f422e3a9a67eb262b2a1 Mon Sep 17 00:00:00 2001
From: AndreyL ProductEngine <alihatskiy@productengine.com>
Date: Tue, 9 Aug 2016 19:26:30 +0300
Subject: MAINT-6618 More checks
---
indra/llimage/llimage.cpp | 41 +++++++++++++++++++++++++++++++++++++----
indra/llimage/llimage.h | 3 +++
2 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/indra/llimage/llimage.cpp b/indra/llimage/llimage.cpp
index c7f02a6b1a..feb97ec2ab 100644
--- a/indra/llimage/llimage.cpp
+++ b/indra/llimage/llimage.cpp
@@ -1075,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());
@@ -1176,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()) );
@@ -1242,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
@@ -1376,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() );
@@ -1709,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.
--
cgit v1.2.3