#2302 Cannot unpack squashfs images created with squashfs-tools-ng

[Goliath]

Hi,

I'm the principal author of squashfs-tools-ng, a new tooling for squashfs archives, created from scratch after reverse engineering the underlying format.

The images generated by squashfs-tools-ng can be mounted by Linux just fine and can be uncompacked with the existing squashfs-tools as well.

To my understanding, 7zip is supposed to support unpacking squashfs archives, but it doesn't seem to like any of the images generated by squashfs-tools-ng. Some initial investigation into the issue is also in the ticket here:

https://github.com/AgentD/squashfs-tools-ng/issues/85

[Igor Pavlov]

please create small example of unsupported squashfs archive, and attach it here.

[Goliath]

The attached test.sqfs was generated from the squashfs-tools-ng doc directory using mksquashfs and works.

The second file, test2.sqfs was generated by running it through squashfs-tools-ng tar2sqfs like this: sqfs2tar test.sqfs | tar2sqfs test2.sqfs. It does not appear to work on my end using 7-zip 16.02.

[Igor Pavlov]

There are lines in 7-Zip that lead to that problem.
SquashfsHandler.cpp
CHandler::OpenDir():

  // for some squashfs files: fileSize = rem + 3  !!!
  if (_h.Major >= 3)
  {
    if (fileSize < 3)
      return S_FALSE;
    fileSize -= 3;
  }

if I remove these lines, 7-Zip can open test2.sqfs, but it can't open test.sqfs.
Please write me here, if you know what are these 3 additional bytes in node.FileSize.

[Goliath]

Is this perhaps derived from the readdir() code in the squashfs kernel module? The kernel code at one point adds 3 to the directory size and then in readdir checks if the offset is < 3 to return artificial . and .. entries.

Other than that I don't know of any padding inode or file padding.

[Igor Pavlov]

I don't keep squashfs details in mind.
And I don't know any source code lines for these 3 bytes in squashfs code.
Please show exact lines in squashfs code for these 3 bytes.
Also try to create smaller examples (maybe with just one small file inside) to show exact difference in headers.

[Goliath]

The code I was referring to, is this one:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/squashfs/dir.c#n50

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/squashfs/dir.c#n115

The size is padded by 3 byte and when reading at an offset < 3, "." and ".." are returned. My guess was, that this might be where this difference comes from.

The attached archives contain exactly 1 file each (under the root directory). Inodes and directories are uncompressed.

According to vbindiff, there are 5 bytes different:

• The flags in the super block.
• The permissions, link count and parent inode number of the root inode.
• mksquashfs sets the root inode size (i.e. the size of the directory listing) to be off by 3.

They have an identical listing. The size that mksquashfs sets is 3 bytes larger than it actually is.

I did some digging in mksquashfs code and found that this is intentionally added: https://github.com/plougher/squashfs-tools/blob/master/squashfs-tools/mksquashfs.c#L1287

If I modify the size value to be 3 bytes larger, 7-zip can read the archive. However, neither the Linux kernel nor unsquashfs have any trouble reading both archives.

[Igor Pavlov]

It's still difficult for me.
If original 3 bytes scheme is good for original squashfs, why new squash doesn't follow it?
What is good implementation from them?
Original squashfs or new?
We need full description of 3-bytes poroblem for understanding:

"original" squashfs does ..., because ..., it's good/bad.
"new" squashf does another way, because ...

Any inconsistency in fields creates such compatibility problems.
So we need some specification that describes what is allowed and what is not allowed.

[Goliath]

There is now "new squashfs", there are new tools, that try to be compatible with the Linux kernel code. This now looks like a compatibility problem on our end.

7-zip uses the size stored in the inode, but the Linux kernel SquashFS does not, it uses the size stored in the directory header, so it doesn't matter if the length is off by 3. This is why the problem didn't show up sooner.

This is the closest I have to a specification from reverse engineering the kernel side of the code, but it (right now) has the same bug:

https://github.com/AgentD/squashfs-tools-ng/blob/master/doc/format.txt

I will have to fix this on my end, but thanks for the quick help!

[Igor Pavlov]

What are "old squashfs" and "new squashfs"?
What code is used to produce most of squashfs files now?

I still don't understand about 3 bytes.
Why old squashfs has that 3 bytes difference in most squashfs files?
why it's 3 bytes, but not 1,2,3,4 bytes?
Is it some bug of original (old) code squashfs?

Actually I don't understand my code:

if (_h.Major >= 3)
  {
    if (fileSize < 3)
      return S_FALSE;
    fileSize -= 3;
  }

It looks illogical, when we reduce fileSize for 3 bytes here.
Why we do it only for MajorVersion >= 3?
It looks like workaround for some bug of encoder squashfs code.

If it's bug of squashfs encoder, we need some description, like this:
"encoder squashfs (what exact versions ?) has bug, that writes incorrect value to header. The most squashfs decoders do not use that value, so they do not see that bug."

Now I don't understand squashfs code (and all implementations), so it's diffucult for me to analyse all aspects of that problem.

[Phillip Lougher]

What are "old squashfs" and "new squashfs"?

There is no "new" Squashfs. Squashfs-tools-ng is just a third-party implementation, like there are implementations for Python, Squashfs FUSE etc. Do not mistake it for some new tooling that replaces my Squashfs-tools, because it isn't.

I still don't understand about 3 bytes.
Why old squashfs has that 3 bytes difference in most squashfs files?
why it's 3 bytes, but not 1,2,3,4 bytes?
Is it some bug of original (old) code squashfs?

No. the extra three bytes added is deliberate. Squashfs does not store directory entries for ".", and "..". It instead invents these entries when called by the kernel code.

So "." is returned with directory offset of 0. ".." is returned with directory offset of 1. The next directory entry (which is the first real entry) has an offset of 3.

This is explained in the kernel code.

https://github.com/torvalds/linux/blob/master/fs/squashfs/dir.c#L51

[Phillip Lougher]

Also https://github.com/torvalds/linux/blob/master/fs/squashfs/dir.c#L116

[Phillip Lougher]

Actually I don't understand my code:

if (_h.Major >= 3)
{
if (fileSize < 3)
return S_FALSE;
fileSize -= 3;
}
It looks illogical, when we reduce fileSize for 3 bytes here.
Why we do it only for MajorVersion >= 3?
It looks like workaround for some bug of encoder squashfs code.

No.

Squashfs in versions 1 & 2 did not store ".", or ".." entries in directories.

More importantly, when returning the list of files (directory contents) to the kernel it didn't return "." or ".." entries.

That was OK. The Linux kernel VFS doesn't need them.

But when I tried to mainline Squashfs back in 2005, the lack of "." and ".." entries being returned was complained about.

So, in version 3 I updated the code to return "." and ".." entries.

Hence the difference between V1/V2 and later versions.

[Igor Pavlov]

I still want to see some explanation for the number "3".
Why it's 3 and not 2, for example?
I write only squashfs decoder, and I don't care about linux kernel related problems and things.
But I still need the code:

if (_h.Major >= 3)
  {
    if (fileSize < 3)
      return S_FALSE;
    fileSize -= 3;
  }

So the question is - why I need that illogical code?
Why container format contains non-consistent fields, where size value is not real size, and we must modify the value in decoder?
It was impossible to write consistent values to headers?

[Zachary Dremann]

The + 3 is 1 byte for ".", and 2 bytes for "..". These 3 bytes are not present in the on-disk file, but are part of the "logical size" of the directory. Arguably, since that data isn't actually in the file itself, the kernel code could have just added 3 to the size of the inode it uses for directory inode types, but that is what the format is, and I don't think it's changeable at this point.

[Phillip Lougher]

The + 3 is 1 byte for ".", and 2 bytes for "..". These 3 bytes are not present in the on-disk >file, but are part of the "logical size" of the directory. Arguably, since that data isn't >actually in the file itself, the kernel code could have just added 3 to the size of the inode >it uses for directory inode types, but that is what the format is, and I don't think it's >changeable at this point.

Think about those 3 bytes as part of the compression, or removal of unnecessary data. Just because they're not there, it doesn't mean the inode size should be decreased by 3.

In the same way, if 100 bytes compresses to 20 bytes (80 bytes isn't actually there), you still store 100 as the size, not 20,

The inode size is the logical size, before removal of redundant data and compression.

[Igor Pavlov]

Actually, the question is about "good implementation" of decoder.
I don't understand all aspects and tricks in these headers.
So maybe my decoder in 7-Zip is not good as possible.
For example, another headers in "-ng" work ok with another decoder implemenations, and do not work with my decoder implementation.
So is classic decoder is not strict enough?
Or does my implementation work with these 3 bytes in wrong way?
If it's wrong way now in 7-Zip, what is correct way to work with these 3 bytes?

[Phillip Lougher]

. is one character

.. is two characters.

1 + 2 == 3.

It is the length of "." plus the length of "..".

Surely, that is easy to understand.

I didn't write your code, if you find it illogical that's not my problem.

But, think of it this way.

V3 directories are always 3 byes or larger in size. That's because they have "." and ".." entries. An empty directory will always have "." and "..".

V2 and before did not have "." or "..". So an empty directory is ZERO bytes in size.

There is no inconsistency here. V2 and V3 are different formats. You're trying to deal with both in the same code. It's a hack. But, not my problem.

[Igor Pavlov]

V3 directories are always 3 byes or larger in size. That's because they have "." and ".." entries. An empty directory will always have "." and "..".

When you write "V3 directories" are always 3 bytes or larger in size, you mean some virtual directory (linux directory), but not real directory data that is stored in sqs-file?

But I don't care about these virtual directories (that are for linux). So why I still see these 3 bytes in my code?
If you do transformation real_headers -> virtual_headers_for_linux, why didn't you hide these 3 bytes in that transformation, if you need it for linux.
So if I don't need that linux specific transformation in 7-Zip code, I could work without that "illogical" code for these 3 bytes.

[Phillip Lougher]

Just forget about those 3 bytes OK. They're completely unimportant for you.

Clear enough?

[Igor Pavlov]

They are important, if we can't solve the problem.
I try to use strict decoding and check most of header fields.
So I must include the code to work with these 3 bytes.
And that code doesn't work for sqsh archives, created by "-ng", But another decoders work with these files.
So some of these decoders is not correct or is not strict enough.
So I want to get correct way in 7-Zip for strict decoding of such archives.

[Zachary Dremann]

The format of v3/4 squashfs archives sets the file_size of directories to the total size of all headers + entries + 3. That is the format squashfs-tools generates, and the kernel reads. "-ng" was generating invalid archives, which don't follow this format correctly, but happened to be read without issue in other decoders. Per https://github.com/AgentD/squashfs-tools-ng/issues/85#issuecomment-883161894: "This will be fixed on the squashfs-tools-ng side, and I will make an effort in patching existing documentation". The 7-zip code is correct as is, other decoders failed to validate that all data related to the directory headers + entries fit within dir.file_size - 3.

[Zachary Dremann]

I belive this issue can be closed