nathanhi / pyfatfs

Python based FAT12/FAT16/FAT32 implementation with VFAT support
https://pypi.org/project/pyfatfs/
MIT License
29 stars 14 forks source link

The DosDateTime.deserialize_time function produce ValueError: second must be in 0..59 #34

Closed beckerben closed 11 months ago

beckerben commented 1 year ago

I'm finding at the top of every minute, it is possible to get the value error in the deserialize_time function.

You can produce the error with:

#Function from DasDateTime
def deserialize_time(tm: int) -> time:
        """Convert a DOS time format to a Python object."""
        second = (tm & (1 << 5) - 1) * 2
        minute = (tm >> 5) & ((1 << 6) - 1)
        hour = (tm >> 11) & ((1 << 5) - 1)
        return time(hour, minute, second)

#Code to show the out of range second error
python_time = deserialize_time(0x001E)
print(python_time)
nathanhi commented 11 months ago

Interesting! According to the FAT specification having a second value >29 (in this case: 30) is invalid:

image

Do you mind sharing what created the file and/or the filesystem? Maybe even an example filesystem image? At least in PyFatFS there's a boundary check in place that verifies the seconds range:

>>> from pyfatfs.DosDateTime import DosDateTime
>>> DosDateTime(1980, 1, 1, 0, 0, 60)
Traceback (most recent call last):
  File "/app/extra/plugins/python/helpers/pydev/pydevconsole.py", line 364, in runcode
    coro = func()
           ^^^^^^
  File "<input>", line 1, in <module>
ValueError: second must be in 0..59

Serialization of the boundary is part of the test suite and seems to work as intended:

>>> from pyfatfs.DosDateTime import DosDateTime
>>> hex(DosDateTime(1980, 1, 1, 0, 0, 58).serialize_time())
'0x1d'
>>> hex(DosDateTime(1980, 1, 1, 0, 0, 59).serialize_time())
'0x1d'
beckerben commented 11 months ago

It was a turbidity meter (Hach TL2350) and we believe that there is actually an issue with the instrument with regard to how it is writing invalid timestamps. We worked around it by handling the invalid value to force it to return 0 rather than triggering the exception.

nathanhi commented 11 months ago

Thank you for the heads-up! I've updated you pull request and added additional information to the commit message.