On Monday 20 January 2020 12:31:16 Jan Kara wrote:
> On Sun 19-01-20 13:49:44, Pali Rohár wrote:
> > Hello! I looked at udf code which converts linux time to UDF time and I
> > found out that conversion of timezone is incorrect.
> > 
> > Relevant code from udf_time_to_disk_stamp() function:
> > 
> > 	int16_t offset;
> > 
> > 	offset = -sys_tz.tz_minuteswest;
> > 
> > 	dest->typeAndTimezone = cpu_to_le16(0x1000 | (offset & 0x0FFF));
> > 
> > UDF 2.60 2.1.4.1 Uint16 TypeAndTimezone; says:
> > 
> >   For the following descriptions Type refers to the most significant 4 bits of this
> >   field, and TimeZone refers to the least significant 12 bits of this field, which is
> >   interpreted as a signed 12-bit number in two’s complement form.
> > 
> >   TimeZone ... If this field contains -2047 then the time zone has not been specified.
> > 
> > As offset is of signed 16bit integer, (offset & 0x0FFF) result always
> > clears sign bit and therefore timezone is stored to UDF fs incorrectly.
> 
> I don't think the code is actually wrong. Two's complement has a nice
> properly that changing type width can be done just by masking - as an
> example -21 in s32 is 0xffffffeb, if you reduce to just 12-bits, you get
> 0xfeb which is still proper two's complement encoding of -21 for 12-bit wide
> numbers.

Ou right, so it is really OK. Thank you for clarification.

> > This needs to be fixed, sign bit from tz_minuteswest needs to be
> > propagated to 12th bit in typeAndTimezone member.
> > 
> > Also tz_minuteswest is of int type, so conversion to int16_t (or more
> > precisely int12_t) can be truncated. So this needs to be handled too.
> 
> tz_minuteswest is limited to +-15 hours (i.e., 900 minutes) so we shouldn't
> need to handle truncating explicitely.

Fine, then code is correct.

-- 
Pali Rohár
pali.rohar@gmail.com