Skip to content
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

Bugfix - prevent Berry path.listfile() crash by limiting to 25 files #18906

Closed
wants to merge 1 commit into from

Conversation

btsimonh
Copy link
Contributor

Description:

During testing, listing a folder with >1000 files on SDCard crashed TAS, and it did not reboot (had to visit the unit).

Unfortunately I'm not able to get a serial log to see what was actually going on; but path.listdir() is simply not designed to handle large quantities, and had a bug whereby it was not closing files.

So this PR limits it to 25 files, and adds a filename _more to the end if the limit is reached, as well as closing the files.

A longer term solution to listing larger folders would consist of an iteration over the folder from Berry. More thought and discussion needs to go into this before implementation.

The PR is low risk, but does change the behaviour of path.listdir() by limiting the number of files returned - this could affect exisitng users, so please review and change the number 25 as required.

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.10
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@btsimonh
Copy link
Contributor Author

the first casualty of this mod is my 'zap(folder)' function, which now can't complete as it only deletes the first 25 files in folder/subfolders!... but it's better than it killing tas.

@s-hadinger
Copy link
Collaborator

Thanks, although 25 seems a little low. Would it be safe to return at least 50 or 100 ?

@btsimonh
Copy link
Contributor Author

we could estimate, using the max filename len in the TAS file driver of 48, that 100 would take ~5kbytes.
That does not sound unreasonable.
How much memory do we consider 'normal' in a running Berry?

Also, what should we use as the 'terminator'? Could we add 'nil' rather than a string - possibly more unique?

p.s. I'm guessing it's possible to add a 2nd optional parameter to listdir which could indicate 'start index', then we would not need a new API to get more...

@Staars
Copy link
Contributor

Staars commented Jun 21, 2023

I tried to recreate your crash, but was unsuccessful.
After polluting the filesystem with 200 empty files, I could only see the truncation of the list like so:

15:06:57.230 ['/.settings', '/bspl0001.c', '/bspl0002.c', '/bspl0003.c', '/bspl0004.c', '/bspl0005.c', '/bspl0006.c', '/bspl0007.c', '/bspl0008.c', '/bspl0009.c', '/bspl0010.c', '/bspl0011.c', '/bspl0012.c', '/bspl0013.c', '/bspl0014.c', '/bspl0015.c', '/bspl0016.c', '/bspl0017.c', '/bspl0018.c', '/bspl0019.c', '/bspl0020.c', '/bspl0021.c', '/bspl0022.c', '/bspl0023.c', '/bspl0024.c', '/bspl0025.c', '/bspl0026.c', '/bspl0027.c', '/bspl0028.c', '/bspl0029.c', '/bspl0030.c', '/bspl0031.c', '/bspl0032.c', '/bspl0033.c', '/bspl0034.c', '/bspl0035.c', '/bspl0036.c', '/bspl0037.c', '/bspl0038.c', '/bspl0039.c', '/bspl0040.c', '/bspl0041.c', '/bspl0042.c', '/bspl0043.c', '/bspl0044.c', '/bspl0045.c', '/bspl004

Not necessarily related, I have rewritten the the MPATH_LISTDIR part to use the newer Arduino API and the speed of the above operation increased dramatically (from >5 seconds to about 700 milliseconds).
Did you encounter a watchdog related crash?

@Staars Staars mentioned this pull request Jun 21, 2023
6 tasks
@btsimonh
Copy link
Contributor Author

The TAS did not come back in a few minutes, and I did not have serial on it. The particual cam unit is incredibly reliable normally, but having thought about it, it is entirely possible that I was doing as lot in Berry with each file, and it was just taking a VERY long time.

@Staars
Copy link
Contributor

Staars commented Jun 21, 2023

Maybe you can upload the images while building the firmware to a device with serial. I created another PR today, that makes it more convenient.

@btsimonh
Copy link
Contributor Author

so it truncated at 45 - but that just a berry print truncation - I am guessing.
I need to test again - maybe do what you did and create 1000 files in a folder.

@Staars
Copy link
Contributor

Staars commented Jun 21, 2023

Yes, got confused by the heat 😀

The list is intact and has the correct size of 200.

@Staars
Copy link
Contributor

Staars commented Jun 21, 2023

10194files

I hope 10193 files are enough.

Note that the line var t = path.listdir("/")did get lost.
System is freaking slow at boot, I thought it did freeze.
But the rest is working as you see.

@btsimonh
Copy link
Contributor Author

your PR also avoids the non closure issue, so I'll close this one in favour of yours.... :)
Also, it ended 'nicely'. (i.e. an exception). Thanks for gripping this!

@btsimonh btsimonh closed this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants