Merge "AAPT2: Ignore trailing data after IEND chunk in PNG" into oc-dev
diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp
index c192d69..5adf04a 100644
--- a/tools/aapt2/cmd/Compile.cpp
+++ b/tools/aapt2/cmd/Compile.cpp
@@ -372,6 +372,8 @@
if (file_type == file::FileType::kDirectory) {
context->GetDiagnostics()->Error(DiagMessage(input_path)
<< "resource file cannot be a directory");
+ } else if (file_type == file::FileType::kNonexistant) {
+ context->GetDiagnostics()->Error(DiagMessage(input_path) << "file not found");
} else {
context->GetDiagnostics()->Error(DiagMessage(input_path)
<< "not a valid resource file");
@@ -488,7 +490,7 @@
// Ensure that we only keep the chunks we care about if we end up
// using the original PNG instead of the crunched one.
PngChunkFilter png_chunk_filter(content);
- std::unique_ptr<Image> image = ReadPng(context, &png_chunk_filter);
+ std::unique_ptr<Image> image = ReadPng(context, path_data.source, &png_chunk_filter);
if (!image) {
return false;
}
diff --git a/tools/aapt2/compile/Png.h b/tools/aapt2/compile/Png.h
index e4255e7..7ca1f0e 100644
--- a/tools/aapt2/compile/Png.h
+++ b/tools/aapt2/compile/Png.h
@@ -69,7 +69,12 @@
bool Rewind() override;
size_t ByteCount() const override { return window_start_; }
- bool HadError() const override { return error_; }
+ bool HadError() const override {
+ return !error_msg_.empty();
+ }
+ std::string GetError() const override {
+ return error_msg_;
+ }
private:
DISALLOW_COPY_AND_ASSIGN(PngChunkFilter);
@@ -79,13 +84,13 @@
android::StringPiece data_;
size_t window_start_ = 0;
size_t window_end_ = 0;
- bool error_ = false;
+ std::string error_msg_;
};
/**
* Reads a PNG from the InputStream into memory as an RGBA Image.
*/
-std::unique_ptr<Image> ReadPng(IAaptContext* context, io::InputStream* in);
+std::unique_ptr<Image> ReadPng(IAaptContext* context, const Source& source, io::InputStream* in);
/**
* Writes the RGBA Image, with optional 9-patch meta-data, into the OutputStream
diff --git a/tools/aapt2/compile/PngChunkFilter.cpp b/tools/aapt2/compile/PngChunkFilter.cpp
index f9043b5..7073c13 100644
--- a/tools/aapt2/compile/PngChunkFilter.cpp
+++ b/tools/aapt2/compile/PngChunkFilter.cpp
@@ -16,11 +16,13 @@
#include "compile/Png.h"
+#include "android-base/stringprintf.h"
#include "androidfw/StringPiece.h"
#include "io/Io.h"
using android::StringPiece;
+using android::base::StringPrintf;
namespace aapt {
@@ -73,7 +75,7 @@
window_start_ = 0;
window_end_ = kPngSignatureSize;
} else {
- error_ = true;
+ error_msg_ = "PNG does not start with PNG signature";
}
}
@@ -90,7 +92,7 @@
}
bool PngChunkFilter::Next(const void** buffer, size_t* len) {
- if (error_) {
+ if (HadError()) {
return false;
}
@@ -106,16 +108,21 @@
const size_t kMinChunkHeaderSize = 3 * sizeof(uint32_t);
// Is there enough room for a chunk header?
- if (data_.size() - window_start_ < kMinChunkHeaderSize) {
- error_ = true;
+ if (data_.size() - window_end_ < kMinChunkHeaderSize) {
+ error_msg_ = StringPrintf("Not enough space for a PNG chunk @ byte %zu/%zu", window_end_,
+ data_.size());
return false;
}
// Verify the chunk length.
const uint32_t chunk_len = Peek32LE(data_.data() + window_end_);
- if (((uint64_t)chunk_len) + ((uint64_t)window_end_) + sizeof(uint32_t) > data_.size()) {
+ if ((size_t)chunk_len > data_.size() - window_end_ - kMinChunkHeaderSize) {
// Overflow.
- error_ = true;
+ const uint32_t chunk_type = Peek32LE(data_.data() + window_end_ + sizeof(uint32_t));
+ error_msg_ = StringPrintf(
+ "PNG chunk type %08x is too large: chunk length is %zu but chunk "
+ "starts at byte %zu/%zu",
+ chunk_type, (size_t)chunk_len, window_end_ + kMinChunkHeaderSize, data_.size());
return false;
}
@@ -124,6 +131,16 @@
if (IsPngChunkWhitelisted(chunk_type)) {
// Advance the window to include this chunk.
window_end_ += kMinChunkHeaderSize + chunk_len;
+
+ // Special case the IEND chunk, which MUST appear last and libpng stops parsing once it hits
+ // such a chunk (let's do the same).
+ if (chunk_type == kPngChunkIEND) {
+ // Truncate the data to the end of this chunk. There may be garbage trailing after
+ // (b/38169876)
+ data_ = data_.substr(0, window_end_);
+ break;
+ }
+
} else {
// We want to strip this chunk. If we accumulated a window,
// we must return the window now.
@@ -145,14 +162,14 @@
}
void PngChunkFilter::BackUp(size_t count) {
- if (error_) {
+ if (HadError()) {
return;
}
window_start_ -= count;
}
bool PngChunkFilter::Rewind() {
- if (error_) {
+ if (HadError()) {
return false;
}
window_start_ = 0;
diff --git a/tools/aapt2/compile/PngCrunch.cpp b/tools/aapt2/compile/PngCrunch.cpp
index ae98afc..42443d8 100644
--- a/tools/aapt2/compile/PngCrunch.cpp
+++ b/tools/aapt2/compile/PngCrunch.cpp
@@ -73,6 +73,11 @@
static void LogError(png_structp png_ptr, png_const_charp error_msg) {
IDiagnostics* diag = (IDiagnostics*)png_get_error_ptr(png_ptr);
diag->Error(DiagMessage() << error_msg);
+
+ // Causes libpng to longjmp to the spot where setjmp was set. This is how libpng does
+ // error handling. If this custom error handler method were to return, libpng would, by
+ // default, print the error message to stdout and call the same png_longjmp method.
+ png_longjmp(png_ptr, 1);
}
static void ReadDataFromStream(png_structp png_ptr, png_bytep buffer, png_size_t len) {
@@ -82,7 +87,12 @@
size_t in_len;
if (!in->Next(&in_buffer, &in_len)) {
if (in->HadError()) {
- std::string err = in->GetError();
+ std::stringstream error_msg_builder;
+ error_msg_builder << "failed reading from input";
+ if (!in->GetError().empty()) {
+ error_msg_builder << ": " << in->GetError();
+ }
+ std::string err = error_msg_builder.str();
png_error(png_ptr, err.c_str());
}
return;
@@ -103,6 +113,11 @@
while (len > 0) {
if (!out->Next(&out_buffer, &out_len)) {
if (out->HadError()) {
+ std::stringstream err_msg_builder;
+ err_msg_builder << "failed writing to output";
+ if (!out->GetError().empty()) {
+ err_msg_builder << ": " << out->GetError();
+ }
std::string err = out->GetError();
png_error(png_ptr, err.c_str());
}
@@ -126,7 +141,7 @@
}
}
-std::unique_ptr<Image> ReadPng(IAaptContext* context, io::InputStream* in) {
+std::unique_ptr<Image> ReadPng(IAaptContext* context, const Source& source, io::InputStream* in) {
// Read the first 8 bytes of the file looking for the PNG signature.
// Bail early if it does not match.
const png_byte* signature;
@@ -163,6 +178,9 @@
return {};
}
+ // Create a diagnostics that has the source information encoded.
+ SourcePathDiagnostics source_diag(source, context->GetDiagnostics());
+
// Automatically release PNG resources at end of scope.
PngReadStructDeleter png_read_deleter(read_ptr, info_ptr);
@@ -174,7 +192,7 @@
}
// Handle warnings ourselves via IDiagnostics.
- png_set_error_fn(read_ptr, (png_voidp)context->GetDiagnostics(), LogError, LogWarning);
+ png_set_error_fn(read_ptr, (png_voidp)&source_diag, LogError, LogWarning);
// Set up the read functions which read from our custom data sources.
png_set_read_fn(read_ptr, (png_voidp)in, ReadDataFromStream);
@@ -231,8 +249,8 @@
// something
// that can always be represented by 9-patch.
if (width > std::numeric_limits<int32_t>::max() || height > std::numeric_limits<int32_t>::max()) {
- context->GetDiagnostics()->Error(
- DiagMessage() << "PNG image dimensions are too large: " << width << "x" << height);
+ source_diag.Error(DiagMessage()
+ << "PNG image dimensions are too large: " << width << "x" << height);
return {};
}