Fix reading pascal string #42

gyk · 2023-03-19T05:17:27Z

I encountered a PSD file that caused psd crate to panic. The problems are:

read_pascal_string seems to have a bug. Pascal string is padded to make the size even, so the extra byte only exists when data length + 1 (header byte) is odd. Related to DescriptorStructure can occur inside an ImageResourcesBlock #40. Ref implementation: https://github.com/ImageMagick/ImageMagick/blob/7.1.1-3/coders/psd.c#L798-L799
It contains non-UTF-8 strings.
The last channel (blue) has a trailing zero byte. Not sure whether it is unnecessary padding, but otherwise the image data itself is intact. It would be nice to make the crate more tolerant of possibly malformed files.

This PR attempts to resolve above issues.

Update: Also fixes panicking when the Layer and mask information section is empty (Krita can produce such PSDs).

chinedufn · 2023-03-19T18:02:09Z

Thanks!

We'll need a test case that has a minimal PSD file where

Before this commit, we could not read back some string
As of this commit, we're able to read back the string

The test can live somewhere in here https://github.com/chinedufn/psd/tree/master/tests

After that we can get this merged.

gyk · 2023-03-25T11:14:59Z

I've added a 1x1 PSD that

Contains a non-UTF-8 Pascal string of 10 bytes in an image resource block, which is directly copied from a real world PSD file.
Has zero-length layer and mask information section.

chinedufn

Thanks a lot for adding a tiny test PSD file. Looks great.

The String::from_utf8_lossy changes look good. (thank you!)

My only concern is that some of the parsing changes aren't tested.

I left questions on the changes that don't appear to be tested.

Thanks for working to put this together!

chinedufn · 2023-03-26T02:05:58Z

src/psd_channel.rs

+                if cursor.position() + bytes_to_read as u64 > len {
+                    break;
+                }


Tests are passing without this change.
Is this check solving for a real PSD that you came across?
If so can we add a test fixture?
If not, what led you to want to check here to make sure that we aren't reading passed the length? Can we remove this check, or do we need it?

Yes, it is a real PSD file (and its owner has explicitly stated that it is confidential). It can be opened in ImageMagick and Krita, but FFmpeg also complains the file is malformed.
From its XMP metadata I can tell it had been edited by
Photoshop CC 2017 and then Photoshop 23.0 on Windows.

When the code hits this IF branch, cursor.position() and len are both 5964830, and bytes_to_read is 1. The 0 is at the end of the blue channel. I really have no idea why there is an extra byte.

Got it, thanks for explaining.

I think we can unit test this then.

Can add something like this to the bottom of this file:

#[cfg(test)] mod tests { use crate::PsdLayer; use super::*; /// Verify that when when inserting an RLE channel's bytes into an RGBA byte vec we do not /// attempt to read beyond the channel's length. #[test] fn does_not_read_beyond_rle_channels_bytes() { let layer = PsdLayer { channels: todo!("Set up some failing data"), layer_properties: todo!("Set up some failing data"), }; let rgba = todo!(""); let channel_bytes = todo!(""); layer.insert_channel_bytes(&mut rgba, PsdChannelKind::Red, &channel_bytes); assert_eq!(rgba, vec![...]); } }

Then just craft a PsdLayer and rgba and channel_bytes that would fail without your fix.

Can make PsdLayer.channels pub(crate) so that it is visible from here.

Can also add a second test for the other fix below.

Both tests have been added, please take another look.

chinedufn · 2023-03-26T02:06:49Z

src/psd_channel.rs

+                if cursor.position() + 1 > len {
+                    break;
+                }


Tests are passing without this change.
Is this check solving for a real PSD that you came across?
If so can we add a test fixture?
If not, what led you to want to check here to make sure that we aren't reading passed the length? Can we remove this check, or do we need it?

chinedufn · 2023-03-26T02:12:03Z

src/sections/mod.rs

-        let result = String::from_utf8(data.to_vec()).unwrap();
+        let result = String::from_utf8_lossy(data).into_owned();
+
+        if len % 2 == 0 {


Tests still pass without this len % 2 == 0

Good catch! I forgot the problem actually occurred in another image resource block, and the original data contains a string of odd length (13) so the size field following it was incorrectly decoded (0x0005AC00 instead of 0x000005AC):

38 42 49 4D 0B B8 0D 4F 72 69 67 69 6E 44 61 74 8BIM.¸.OriginDat 61 49 52 42 00 00 05 AC 00 00 00 10 00 00 00 01 aIRB...¬........ ...

How about removing one byte of space (0x20) from the non-UTF-8 string in the test fixture?

Want to add a second fixture that is dedicated to this odd length pascal string case?

This way we don't stuff too many different cases into a single fixture.

If you can easily do that then that would be preferred.

If not, we can add a test to the bottom of this file that handles this case (feel free to do both if you want).

Thanks!

chinedufn · 2023-03-26T02:12:50Z

src/sections/layer_and_mask_information_section/mod.rs

-        cursor.read_4();
+        let len = cursor.read_u32();
+
+        if len == 0 {


This is the only change where tests start failing if we comment it out.

chinedufn

Thanks a lot for explaining the changes.

Everything that you explained makes sense. I left an idea for a way that we can get this tested.
Once we have some coverage we can merge this and I can deploy a new version.

chinedufn · 2023-03-26T18:39:51Z

src/psd_channel.rs

+                if cursor.position() + bytes_to_read as u64 > len {
+                    break;
+                }


Got it, thanks for explaining.

I think we can unit test this then.

Can add something like this to the bottom of this file:

#[cfg(test)] mod tests { use crate::PsdLayer; use super::*; /// Verify that when when inserting an RLE channel's bytes into an RGBA byte vec we do not /// attempt to read beyond the channel's length. #[test] fn does_not_read_beyond_rle_channels_bytes() { let layer = PsdLayer { channels: todo!("Set up some failing data"), layer_properties: todo!("Set up some failing data"), }; let rgba = todo!(""); let channel_bytes = todo!(""); layer.insert_channel_bytes(&mut rgba, PsdChannelKind::Red, &channel_bytes); assert_eq!(rgba, vec![...]); } }

Then just craft a PsdLayer and rgba and channel_bytes that would fail without your fix.

Can make PsdLayer.channels pub(crate) so that it is visible from here.

Can also add a second test for the other fix below.

chinedufn · 2023-03-26T18:41:41Z

src/sections/mod.rs

-        let result = String::from_utf8(data.to_vec()).unwrap();
+        let result = String::from_utf8_lossy(data).into_owned();
+
+        if len % 2 == 0 {


Want to add a second fixture that is dedicated to this odd length pascal string case?

This way we don't stuff too many different cases into a single fixture.

If you can easily do that then that would be preferred.

If not, we can add a test to the bottom of this file that handles this case (feel free to do both if you want).

Thanks!

chinedufn · 2023-04-02T19:12:37Z

Thanks for getting this all tested!

gyk added 3 commits March 19, 2023 12:20

Do not panic when reading non-UTF-8 strings

21ef3af

Fix read_pascal_string

3f9981a

Do not read past the end of buffer

f4b2c9a

gyk added 2 commits March 23, 2023 12:26

Layer and mask information section can be empty

dfad805

Add a test for Pascal string

aa36033

chinedufn reviewed Mar 26, 2023

View reviewed changes

Add more tests

17bab2a

chinedufn merged commit 5276ab7 into chinedufn:master Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reading pascal string #42

Fix reading pascal string #42

gyk commented Mar 19, 2023 •

edited

Loading

chinedufn commented Mar 19, 2023

gyk commented Mar 25, 2023

chinedufn left a comment •

edited

Loading

chinedufn Mar 26, 2023 •

edited

Loading

gyk Mar 26, 2023

chinedufn Mar 26, 2023 •

edited

Loading

gyk Apr 1, 2023

chinedufn Mar 26, 2023 •

edited

Loading

chinedufn Mar 26, 2023

gyk Mar 26, 2023

chinedufn Mar 26, 2023

chinedufn Mar 26, 2023

chinedufn left a comment

chinedufn Mar 26, 2023 •

edited

Loading

chinedufn Mar 26, 2023

chinedufn commented Apr 2, 2023

Fix reading pascal string #42

Fix reading pascal string #42

Conversation

gyk commented Mar 19, 2023 • edited Loading

chinedufn commented Mar 19, 2023

gyk commented Mar 25, 2023

chinedufn left a comment • edited Loading

Choose a reason for hiding this comment

chinedufn Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

gyk Mar 26, 2023

Choose a reason for hiding this comment

chinedufn Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

gyk Apr 1, 2023

Choose a reason for hiding this comment

chinedufn Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Mar 26, 2023

Choose a reason for hiding this comment

gyk Mar 26, 2023

Choose a reason for hiding this comment

chinedufn Mar 26, 2023

Choose a reason for hiding this comment

chinedufn Mar 26, 2023

Choose a reason for hiding this comment

chinedufn left a comment

Choose a reason for hiding this comment

chinedufn Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

chinedufn Mar 26, 2023

Choose a reason for hiding this comment

chinedufn commented Apr 2, 2023

gyk commented Mar 19, 2023 •

edited

Loading

chinedufn left a comment •

edited

Loading

chinedufn Mar 26, 2023 •

edited

Loading

chinedufn Mar 26, 2023 •

edited

Loading

chinedufn Mar 26, 2023 •

edited

Loading

chinedufn Mar 26, 2023 •

edited

Loading