kawal547 / open9x

Automatically exported from code.google.com/p/open9x
0 stars 0 forks source link

SD card filenames longer than 8 chars cause crash(?) #90

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Give a model a name longer than 8 chars -- like "Groovy 53a"
2. Backup this model to SD card
3. Now try to restore it in an empty slot. Note that the list of model names in 
"Restore Model" is truncated to 8 characters.

Sometimes -- only sometimes, "SD card error" results from attempting to load. 
Other models/times, a full WDT reset / crash takes place and often EEPROM data 
end up corrupted.

I suspect there is a buffer overflow happening, due to the new long filename 
support. (Model names can be up to 10 characters, of course.)

[Bertrand -- Romolo said he would look into this when he gets time.]

Original issue reported on code.google.com by gru...@gmail.com on 18 Aug 2012 at 1:18

GoogleCodeExporter commented 8 years ago
Perhaps we should return to short names with gruvin9x board, no?

Original comment by bson...@gmail.com on 20 Aug 2012 at 3:52

GoogleCodeExporter commented 8 years ago
It doesn't seem to be causing any other problems. So I'm happy to keep the long 
names, unless you know something I don't?

Original comment by gru...@gmail.com on 21 Aug 2012 at 6:20

GoogleCodeExporter commented 8 years ago
I have now reviewed all the code related to listing files for restore and 
cannot find any buffer overflow issues. I will have to do more experimenting to 
try and determine why the crash occurs. Meanwhile, I propose increasing the 
sub-menu MENU_LINE_LENGTH to (10+1) when long-file-name in effect -- so as to 
match the max model name length, which ends up being the filename length also. 

In bed now, so will test it tomorrow, before committing.

--- menus.h (revision 1250)
+++ menus.h (working copy)
@@ -207,7 +207,11 @@

 #if defined(SDCARD)
 #define MENU_MAX_LINES 4
-#define MENU_LINE_LENGTH (8+1)
+#if _USE_LFN
+#  define MENU_LINE_LENGTH (10+1)
+#else
+#  define MENU_LINE_LENGTH (8+1)
+#endif
 extern const char *s_menu[MENU_MAX_LINES];
 extern char s_bss_menu[MENU_MAX_LINES*MENU_LINE_LENGTH];
 extern uint8_t s_menu_count;

Original comment by gru...@gmail.com on 21 Aug 2012 at 1:10

GoogleCodeExporter commented 8 years ago
The crash I experience is likely a watchdog_timer issue and could be caused 
simply by some kind of lock-up in the SD card reading routines, which in turn 
could be caused by a slow or defective SD card, maybe. (So far, it's only ever 
happen on file names longer than 8 chars. But that could be coincidence. More 
testing.)

To help with this for all such future unwanted resets and for all users, I am 
thinking of adding a global register to record a copy of MCUCSR register taken 
straight after reset as part of existing WDT reset detection. This could then 
be shown in the STAT screen as, "[Power|Soft|Hard|WDT] Reset".

Original comment by gru...@gmail.com on 21 Aug 2012 at 1:19

GoogleCodeExporter commented 8 years ago
Do you confirm that the Backup is OK? The crash only occurs on Restore, right?

Original comment by bson...@gmail.com on 28 Aug 2012 at 6:47

GoogleCodeExporter commented 8 years ago
No possibility it's linked to issue 110 (the other crash)?

Original comment by bson...@gmail.com on 28 Aug 2012 at 6:52

GoogleCodeExporter commented 8 years ago
It actually could be related to issue 110, since I was not aware of that 
problem when I logged this one. I'll test again.

Original comment by gru...@gmail.com on 28 Aug 2012 at 12:35

GoogleCodeExporter commented 8 years ago
Thanks! I really can't see another problem with WDT there, because when we 
backup / restore a model, the pulses and the mixer are not stopped, and running 
in interrupts while the SD work is done in main (less prriority)

Original comment by bson...@gmail.com on 28 Aug 2012 at 12:39

GoogleCodeExporter commented 8 years ago
I think the crash still happens sometimes if a file-not-found occurs during 
restore. However, that problem has already been fixed by extended the max 
length out to 16 char in one of my previous commits (PCBV4 only, of course.) So 
we can let this one go.

Original comment by gru...@gmail.com on 28 Aug 2012 at 12:42