Skip to content

unix/extmod: Fix bugs with timestamps and Epoch #6390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

dpgeorge
Copy link
Member

This PR fixes the following bugs, all related to the unix port:

  • 32-bit unix builds would overflow the atime/ctime/mtime entries in os.stat due to small-int overflow (fixed by using big ints where needed)
  • FAT FS timestamps were localtime not UTC (fixed by using gmtime)
  • VfsFat and VfsLfs on unix dev and coverage variants would have timestamps reported with a 30 year difference due to the Epoch being wrong (fixed by adjusting by 30 years on unix ports)

Also adds some tests to test the second and third points.

@davehylands
Copy link
Contributor

It's my understanding that VFAT timestamps are in localtime (and that's what windows will store). So if you move an sdcard from a windows machine to a micropython machine then the timestamps will all be off if you're treating them as UTC.

@dpgeorge
Copy link
Member Author

dpgeorge commented Sep 1, 2020

It's my understanding that VFAT timestamps are in localtime

Ok. But in our VFS FAT layer the retrieval of timestamps does not take this into account, it just assumes UTC when converting the Y/M/D/H/M/S stored with the file to seconds-since-the-Epoch. So there may be a problem there as well.

So if you move an sdcard from a windows machine to a micropython machine then the timestamps will all be off if you're treating them as UTC.

The fix here targets only the unix port, when making timestamps for FAT files on a locally mounted filesystem using the VFS layer (not something that is done often, it has nothing to do with usual file access through the VfsPosix class). And the aim is to make it so that, after creating a file on a VfsFat filesystem, the timestamp of that file is equal to the value returned by time.time() (which could indeed be fixed by adjusting the value returned via os.stat() rather than changing what is stored on disk).

@dpgeorge
Copy link
Member Author

dpgeorge commented Sep 1, 2020

Doing some tests with FAT on my Arch Linux machine: it seems that Linux stores FAT timestamps on disk in UTC format. So looking at the file on a Windows machine shows that the timestamp is out by the timezone offset. And vice versa: creating a file on Windows and then stat'ing it on Linux gives a timestamp which is out by the timezone in the opposite direction.

Linux has a mount option for vfat - tz=UTC - which controls this behaviour, and that seems to be enabled by default on my machine.

At this stage I'd prefer not to go down the path of trying to fix timezone behaviour/support, and just apply the patch here to make the unix port have the same behaviour as bare-metal for VfsFat filesystems.

On ports like unix where the Epoch is 1970/1/1 and atime/mtime/ctime are in
seconds since the Epoch, this value will overflow a small-int on 32-bit
systems.  So far this is only an issue on 32-bit unix builds that use the
VFS layer (eg dev and coverage unix variants) but the fix (using
mp_obj_new_int_from_uint instead of MP_OBJ_NEW_SMALL_INT) is there for all
ports so as to not complicate the code, and because they will need the
range one day.

Also apply a similar fix to other fields in VfsPosix.stat because they may
also be large.

Signed-off-by: Damien George <damien@micropython.org>
On 32-bit builds these stat fields will overflow a small-int, so use
mp_obj_new_int_from_uint to construct the int object.

Signed-off-by: Damien George <damien@micropython.org>
By setting MICROPY_EPOCH_IS_1970 a port can opt to use 1970/1/1 as the
Epoch for timestamps returned by stat().  And this setting is enabled on
the unix and windows ports because that's what they use.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
Copy link
Member Author

dpgeorge commented Sep 1, 2020

At this stage I'd prefer not to go down the path of trying to fix timezone behaviour/support, and just apply the patch here to make the unix port have the same behaviour as bare-metal for VfsFat filesystems.

On second thoughts, it's probably best to follow how FAT was designed and how Windows does it and store localtime in the timestamps. Then at least that side of things is correct, it's creating a valid and correct FAT filesystem. os.stat() will give values which are incorrect but that can be fixed later on.

Note: this only affects unix dev and coverage variants, when using uos.VfsFat. It has no impact on anything else.

@dpgeorge dpgeorge force-pushed the unix-vfs-stat-epoch branch from ffe0b04 to 04c60d1 Compare September 1, 2020 04:37
Signed-off-by: Damien George <damien@micropython.org>
Verifies mtime timestamps on files match the value returned by time.time().

Also update vfs_fat_ramdisk.py so it doesn't check FAT timestamp of the
root, because that may change across runs/ports.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the unix-vfs-stat-epoch branch from 04c60d1 to 0e6ef40 Compare September 1, 2020 14:20
@dpgeorge dpgeorge merged commit 0e6ef40 into micropython:master Sep 1, 2020
@dpgeorge dpgeorge deleted the unix-vfs-stat-epoch branch September 1, 2020 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants