review-request: Fix invalid data-pointer in the msword-filter (2)

[Uzak Matus]

Hi Sebastian,

like many other attributes specifying the structure length starting with the "cb" prefix, this one does not include the padding.  From the [MS-DOC] spec.

2.9.140 LPUpxPapx
The structure is always padded to an even length, but the length in cbUpx MUST NOT include this
padding.

The 2nd version of the patch is correct, the grupxLen attribute includes padding and is there for internal book keeping. Please commit.  And big thanks for removing a task from my TODO list for this weekend. :)

-matus

--
Matus Uzak
Software Designer
Ixonos Slovakia s.r.o.
Sturova 27, 040 01 Kosice, Slovakia
mobile 0421 918 718 958
email: matus.uzak at ixonos.com
http://www.ixonos.com

________________________________________
From: Sebastian Sauer [mail at dipe.org]
Sent: Thursday, June 09, 2011 5:21 PM
To: calligra-devel at kde.org
Cc: Uzak Matus
Subject: Re: review-request: Fix invalid data-pointer in the msword-filter (2)

On Wednesday 08 June 2011 22:37:13 Sebastian Sauer wrote:
> Please find attached a patch that fixes bug 275204. The reason for the bug
> was that we where ending with random values cause what could work but
> sometimes didn't. The values where random cause we dealed with mem past
> the allocated number of U8-bytes.

Seems the previous patch wasn't correct / wasn't enough to fix the problem.
Please find attached an updated patch that tries to make sure that we never
end outside of the UPX-boundaries by not trusting cbUPX to have the correct
value and limit it additionally to the U8* data-size the UPX is in.

In the last 30 minutes of loading the doc attached to bug 275204 I was at
least not able to produce the problem any longer with the attached patch. But
then since it's a rather random problem...

Even after studying the msdoc-binary specs for a longer time I am still not
sure why that is so. Either the producer of that doc attached to bug 275204
just produced garbage or we are missing something. Following sentence from the
specs seems to match;

[quote]
Each UPX stored in a file is not a complete UPX, rather it is a UPX
with all trailing zero bytes lopped off, and preceded by a ushort
length field.  So it is stored like:
                                 Field     Size Comment
                                 cbUPX     2 bytes   size of the
                                 following UPX structure
                                 UPX  (cbUPX)   Nonzero prefix of a
                                 UPX structure
Each UPX begins on an even-byte offset within the STD, even if the
length of the previous UPX (cbUPX) was odd.
[/quote]

So,
1) can "with all trailing zero bytes lopped" maybe mean that cbUPX does not
really define that exact size of bytes in the stream but the size of the
produced UPX structure which can be different thanks to the trailing zero
bytes lopped or
2) are we probably not handling the even-byte case correct and therefore earn
garbage sometimes?