Skip to content
This repository was archived by the owner on Sep 16, 2024. It is now read-only.

Integer overflow in filesystem stat implementation #409

Closed
amotl opened this issue Feb 24, 2020 · 4 comments
Closed

Integer overflow in filesystem stat implementation #409

amotl opened this issue Feb 24, 2020 · 4 comments

Comments

@amotl
Copy link
Contributor

amotl commented Feb 24, 2020

Dear Pycom team,

while investigating an issue why rshell would upload our files over and over again despite its rsync implementation should compensate for that (dhylands/rshell#124), we just found an issue when looking at mtime timestamps of the files created on the device.

We found that there might be an integer overflow within the virtual filesystem implementation keeping track of filesystem metadata.

This example says a thousand words:

>>> import time; f = open('/flash/test', 'w'); f.write('hello'); f.close(); time.time() - os.stat('/flash/test')[7]
2147483648

It looks like only newer firmware releases are affected. The bug apparently started appearing in 1.20.0.rc0 and is still visible in 1.20.2rc3-pybytes. However, it is not present in 1.18.2.r7 and 1.18.3-pybytes. It looks like it is independent of FatFS vs. LittleFS.

2147483648 is just 2^31 or INT_MAX + 1 or abs(INT_MIN). The correct outcome should be zero (0), or sometimes 1 - dependent on how fast the oneliner is executing.

Thanks already for looking into this.

With kind regards,
Andreas.

cc @husigeza

@robert-hh
Copy link
Contributor

Bug Fix:
File esp32/extmod/vfs_fat.c, Zeilen 368-370
File esp32/littlefs/vfs_littlefs.c, Zeilen 689-691
Replace MP_OBJ_NEW_SMALL_INT by mp_obj_new_int_from_uint

P.S.: I'm not going to send a PR for that, since these seem to end at the office in charge for that at Alpha Centauri.

@amotl
Copy link
Contributor Author

amotl commented Feb 24, 2020

That was quick. I will submit a PR later. Thank you so much, Robert!

@amotl amotl changed the title Integer overflow in filesystem stat implementation? Integer overflow in filesystem stat implementation Feb 26, 2020
amotl added a commit to daq-tools/pycom-micropython that referenced this issue Feb 26, 2020
@amotl
Copy link
Contributor Author

amotl commented Feb 26, 2020

We just verified through dhylands/rshell#124 (comment) that changing these bits brings much of joy, especially when accompanied by dhylands/rshell#125.

Thanks again @robert-hh!

amotl added a commit to daq-tools/pycom-micropython that referenced this issue Feb 27, 2020
amotl added a commit to daq-tools/pycom-micropython that referenced this issue Mar 6, 2020
amotl added a commit to daq-tools/pycom-micropython that referenced this issue Mar 6, 2020
Using small ints for storing st_atime, st_mtime and st_ctime values
is prone to integer overflows, effectively yielding wrong timestamps.

This resolves pycom#409. Contributed by @robert-hh.
@Xykon Xykon closed this as completed in b74db42 Jun 10, 2020
@amotl
Copy link
Contributor Author

amotl commented Jun 10, 2020

Thanks for merging this!

X-Ryl669 pushed a commit to X-Ryl669/pycom-micropython-sigfox that referenced this issue May 12, 2023
…#409. (pycom#129)

Co-authored-by: Andreas Motl <andreas@hiveeyes.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants