Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix 1343 png 0.27 #1351

Merged
merged 3 commits into from
Oct 11, 2020
Merged

Fix 1343 png 0.27 #1351

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
806ffac
Adding test files and bash script/reference-output
clanmills Oct 9, 2020
834dca3
Code changes
clanmills Oct 9, 2020
9fa6d39
C++ changes requested by @piponazo. Fix python png_test() recommende…
clanmills Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions src/pngimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ namespace Exiv2 {
bool zTXt = std::strcmp(chType,"zTXt")== 0;
bool iCCP = std::strcmp(chType,"iCCP")== 0;
bool iTXt = std::strcmp(chType,"iTXt")== 0;
bool eXIf = std::strcmp(chType,"eXIf")== 0;

// for XMP, ICC etc: read and format data
bool bXMP = option == kpsXMP && findi(dataString,xmpKey)==0;
Expand All @@ -313,7 +314,7 @@ namespace Exiv2 {
bool bSoft = option == kpsRecursive && findi(dataString,softKey)==0;
bool bComm = option == kpsRecursive && findi(dataString,commKey)==0;
bool bDesc = option == kpsRecursive && findi(dataString,descKey)==0;
bool bDump = bXMP || bICC || bExif || bIptc || bSoft || bComm || bDesc ;
bool bDump = bXMP || bICC || bExif || bIptc || bSoft || bComm || bDesc || eXIf ;

if( bDump ) {
DataBuf dataBuf;
Expand All @@ -339,6 +340,9 @@ namespace Exiv2 {
if ( iTXt ) {
bGood = (start+3) < dataOffset ; // good if not a nul chunk
}
if ( eXIf ) {
bGood = true ;// eXIf requires no pre-processing)
}

// format is content dependent
if ( bGood ) {
Expand Down Expand Up @@ -383,6 +387,11 @@ namespace Exiv2 {
out.write((const char*)decoded.pData_,decoded.size_);
bLF = true;
}
if ( eXIf && option == kpsRecursive ) {
// create memio object with the data, then print the structure
BasicIo::AutoPtr p = BasicIo::AutoPtr(new MemIo(data,dataOffset));
printTiffStructure(*p,out,option,depth);
}

if ( bLF ) out << std::endl;
}
Expand Down Expand Up @@ -448,8 +457,11 @@ namespace Exiv2 {

/// \todo analyse remaining chunks of the standard
// Perform a chunk triage for item that we need.
if (chunkType == "IEND" || chunkType == "IHDR" || chunkType == "tEXt" || chunkType == "zTXt" ||
chunkType == "iTXt" || chunkType == "iCCP") {
if(chunkType == "IEND" || chunkType == "IHDR"
|| chunkType == "tEXt" || chunkType == "zTXt"
|| chunkType == "eXIf"
|| chunkType == "iTXt" || chunkType == "iCCP"
){
DataBuf chunkData(chunkLength);
readChunk(chunkData, *io_); // Extract chunk data.

Expand All @@ -463,6 +475,13 @@ namespace Exiv2 {
PngChunk::decodeTXTChunk(this, chunkData, PngChunk::zTXt_Chunk);
} else if (chunkType == "iTXt") {
PngChunk::decodeTXTChunk(this, chunkData, PngChunk::iTXt_Chunk);
} else if (chunkType == "eXIf") {
ByteOrder bo = TiffParser::decode(exifData(),
iptcData(),
xmpData(),
chunkData.pData_,
chunkData.size_);
setByteOrder(bo);
} else if (chunkType == "iCCP") {
// The ICC profile name can vary from 1-79 characters.
uint32_t iccOffset = 0;
Expand Down Expand Up @@ -533,16 +552,17 @@ namespace Exiv2 {
// Write PNG Signature.
if (outIo.write(pngSignature, 8) != 8) throw Error(kerImageWriteFailed);

DataBuf cheaderBuf(8); // Chunk header : 4 bytes (data size) + 4 bytes (chunk type).
DataBuf cheaderBuf(9); // Chunk header : 4 bytes (data size) + 4 bytes (chunk type) + nul
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why we need to do this change.

You are changing the size from 8 to 9 but then the last byte is never used.
In line 556 you are setting byte[8] to 0, but that is already done in line 562 where we set the whole byte array to 0.

Could you explain why this is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to use strcmp() instead of memcmp() to inspect different chunks. You can revert that, if you prefer memcmp.()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, I think this change is not needed . We are passing szChunk to the strcmp function and not cheaderBuf. You can still copy 4 bytes from cheaderBuf to szChunk and szChunk can have the 5th byte set to NULL.

cheaderBuf.pData_[8] = 0;

while(!io_->eof())
{
// Read chunk header.

std::memset(cheaderBuf.pData_, 0x00, cheaderBuf.size_);
long bufRead = io_->read(cheaderBuf.pData_, cheaderBuf.size_);
long bufRead = io_->read(cheaderBuf.pData_, 8);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case we additional byte is not needed at the end, I would revert this change and use cheaderBuf.size_ instead of 8 (which could be considered as a magic value)

if (io_->error()) throw Error(kerFailedToReadImageData);
if (bufRead != cheaderBuf.size_) throw Error(kerInputDataReadFailed);
if (bufRead != 8) throw Error(kerInputDataReadFailed);

// Decode chunk data length.

Expand All @@ -551,7 +571,7 @@ namespace Exiv2 {

// Read whole chunk : Chunk header + Chunk data (not fixed size - can be null) + CRC (4 bytes).

DataBuf chunkBuf(8 + dataOffset + 4); // Chunk header (8 bytes) + Chunk data + CRC (4 bytes).
DataBuf chunkBuf(8 + dataOffset + 4); // Chunk header (8 bytes) + Chunk data + CRC (4 bytes).
memcpy(chunkBuf.pData_, cheaderBuf.pData_, 8); // Copy header.
bufRead = io_->read(chunkBuf.pData_ + 8, dataOffset + 4); // Extract chunk data + CRC
if (io_->error()) throw Error(kerFailedToReadImageData);
Expand All @@ -561,16 +581,21 @@ namespace Exiv2 {
memcpy(szChunk,cheaderBuf.pData_ + 4,4);
szChunk[4] = 0;

if (!memcmp(cheaderBuf.pData_ + 4, "IEND", 4))
if ( !strcmp(szChunk,"IEND") )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for replacing the memcmp functions by strcmp

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to revert some of this stuff and update the PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nono, the comment was positive, I think it is more readable to use strcmp for the string comparisons rather than the original memcp 😉

{
// Last chunk found: we write it and done.
#ifdef EXIV2_DEBUG_MESSAGES
std::cout << "Exiv2::PngImage::doWriteMetadata: Write IEND chunk (length: " << dataOffset << ")\n";
#endif
if (outIo.write(chunkBuf.pData_, chunkBuf.size_) != chunkBuf.size_) throw Error(kerImageWriteFailed);
if (outIo.write(chunkBuf.pData_, chunkBuf.size_) != chunkBuf.size_)
throw Error(kerImageWriteFailed);
return;
}
else if (!memcmp(cheaderBuf.pData_ + 4, "IHDR", 4))
else if ( !strcmp(szChunk, "eXIf") ) {
; // do nothing Exif metdata is written following IHDR
; // as zTXt chunk with signature Raw profile type exif__
}
else if ( !strcmp(szChunk, "IHDR") )
{
#ifdef EXIV2_DEBUG_MESSAGES
std::cout << "Exiv2::PngImage::doWriteMetadata: Write IHDR chunk (length: " << dataOffset << ")\n";
Expand Down Expand Up @@ -672,10 +697,10 @@ namespace Exiv2 {
}
}
}
else if (!memcmp(cheaderBuf.pData_ + 4, "tEXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "zTXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "iTXt", 4) ||
!memcmp(cheaderBuf.pData_ + 4, "iCCP", 4))
else if (!strcmp(szChunk, "tEXt") ||
!strcmp(szChunk, "zTXt") ||
!strcmp(szChunk, "iTXt") ||
!strcmp(szChunk, "iCCP"))
{
DataBuf key = PngChunk::keyTXTChunk(chunkBuf, true);
if (compare("Raw profile type exif", key, 21) ||
Expand Down
2 changes: 2 additions & 0 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ TESTS = addmoddel.sh \
iso65k-test.sh \
modify-test.sh \
path-test.sh \
png-test.sh \
preview-test.sh \
stdin-test.sh \
stringto-test.sh \
Expand All @@ -102,6 +103,7 @@ icc-test \
iso65k-test \
modify-test \
path-test \
png-test \
stringto-test \
webp-test \
write2-test \
Expand Down
Loading