Re: [PATCH 1/2] isofs: validate directory records in isofs_read_level3_size()

[Jan Kara]

On Wed 23-09-26 23:32:17, Matthias Goergens wrote:
> Thanks for looking at it.
> 
> I don't see how it can trigger there, but I'm probably missing
> something, so here is my reasoning.  With 1/2 alone, offset is always
> below bufsize at the top of the loop: a record that ends exactly at the
> end of the block takes the existing "offset >= bufsize" branch further
> down, which leaves offset & (bufsize - 1) == 0 and bumps block, so the
> next iteration starts at offset 0 of the next block.  And
> isofs_dir_record_valid() accepts a record that ends exactly at bufsize:
> its test "len > bufsize - offset" is false when len equals the room
> left.

Doh, you're right. I got confused by the flow in the loop. Your patches are
correct.

> I also tried it, running fs/isofs in a userspace harness on level 3
> images where a record of a multi-extent file ends exactly at the end of
> a block, including xorrisofs -iso-level 3 output with a sparse 8 GiB
> file.  Neither 1/2 alone nor 1/2+2/2 rejects the directory, and the file
> sizes come out right; a record whose length runs past the block is
> rejected.
> 
> If you have a reproducer, or just a hint at the case you have in mind,
> I'd be really happy to look at it.
> 
> Even if the code might be technically correct, it's confusing.  1/2 only
> works because of a branch further down that 2/2 then deletes, so I'll
> send a v2 that squashes the two: move on to the next block first, then
> check the record, as you suggest, with the straddle code gone.  The end
> result is the same code as after 2/2.

Yeah, I was also wondering if squashing the two commits won't be less
confusing.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR