edgetx
edgetx copied to clipboard
EdgeTX
Lua Mixer Scripts Cont'd
- [ ] Lua mixer scripts are limited to 6+3 filename length, but it seems that more than 5+3 maybe a EM trigger
- [ ] "the output value isn't correctly displayed, always 0"
- [ ] Possibly computationally intense Lua scripts may crash or freeze the radio
- [ ] #1318
The next problem that still remains is that the radio freezes / crashes if you use the fourth instance of this script:
This is a little bit more computational intense than the other.
Maybe it helps to identify this one: the first three instances are not saved in the model. After crash and restart the model does not contain the first three instances of the script. This is reproducible.
Originally posted by @wimalopaan in https://github.com/EdgeTX/edgetx/issues/1312#issuecomment-1002897453
The 6 char problem is located here:
https://github.com/EdgeTX/edgetx/blob/d37b3e40518af42b6e54b3bda1b432a8f0431427/radio/src/gui/colorlcd/model_mixer_scripts.cpp#L87
strncpy() does not copy the \0 if filename is 6 chars (or more), so the string remains unterminated.
This
strncpy(sd->file, newValue.c_str(), LEN_SCRIPT_FILENAME);
sd->file[LEN_SCRIPT_FILENAME - 1] = '\0';
terminates correctly, but leaves the problem, that then a file with that filename does not exist or is simply another lua-file with the same prefix-name. This misleads the user.
The best solution would to only list those files with filenames with not more than 5 +3 chars
Looks like this fixed it (display only files according 5 + 3 chars):
auto fc = new FileChoice(
window, grid.getFieldSlot(), SCRIPTS_MIXES_PATH, SCRIPTS_EXT,
LEN_SCRIPT_FILENAME - 1,
[=]() { return std::string(sd->file, LEN_SCRIPT_FILENAME); },
[=](std::string newValue) {
strncpy(sd->file, newValue.c_str(), LEN_SCRIPT_FILENAME);
sd->file[LEN_SCRIPT_FILENAME - 1] = '\0';
if (newValue.empty()) { memset((void*)sd, 0, sizeof(ScriptData)); }
storageDirty(EE_MODEL);
LUA_LOAD_MODEL_SCRIPT(idx); // async reload...
update = true;
}, true);
@wimalopaan that is normal and used at many places to save a few bytes of memory. That being said, the code copying this file name to LUA on query should be aware of that and fix it before passing it to LUA.
@wimalopaan that is normal and used at many places to save a few bytes of memory. That being said, the code copying this file name to LUA on query should be aware of that and fix it before passing it to LUA.
Thanks, I'm not familiar with the code base and that's the reason not to create a PR here ;-)
Ok, then the root cause maybe at other place ...
Is it sure that the caller of this really knows about LEN_SCRIPT_FILENAME
https://github.com/EdgeTX/edgetx/blob/d37b3e40518af42b6e54b3bda1b432a8f0431427/radio/src/lua/interface.cpp#L588
https://github.com/EdgeTX/edgetx/blob/d37b3e40518af42b6e54b3bda1b432a8f0431427/radio/src/lua/interface.cpp#L603
and that is wrong at least here:
https://github.com/EdgeTX/edgetx/blob/d37b3e40518af42b6e54b3bda1b432a8f0431427/radio/src/lua/interface.cpp#L931
Here are some other places that require \0-terminated strings (violated by the 6-char case):
https://github.com/EdgeTX/edgetx/blob/81d77684986bdbc5181945f5eb72c054e632ca8c/radio/src/strhelpers.cpp#L577-L604
And here is a patch with some (maybe not all) corrections for review:
Well, this string-handling looks to me really weird and dangerous.
I would love really to suggest:
template<unit_8t Size>
using Cstring = std::array<char, Size>;
with some simple helper template-functions. Because there a only a few different lengths around, that should not result in code-bloat.
@wimalopaan this uses tons of dynamically allocated memory, which is an issue on B&W targets (used only for LUA). We need to do it "old-school" for now ;-)
@raphaelcoeffic std::array doesn't use dynamic allocated memory - this is not std::string nor std::vector
std::array has no runtime-overhead compared to raw c-arrays.
There might be some code-bloat (flash, not RAM) because of different template-instantiations using different parametrizsations of std::array<>.
But I thilnk that the security argument would be of much concern. I think this dicussion shows the need for this ;-)
Some more things to correct:
@jfrickmann please have a look at these changes.
I will not pretend that I understand the motivation for everything you do, but I did not spot any problems, and it built w/o problems.
As for model_mixer_scripts.cpp:
- You added some checks to clear data when necessary - it looks plausible to me.
As for interface.cpp:
-
assert(ref >= SCRIPT_MIX_FIRST);is unnecessary, asrefis unsigned andSCRIPT_MIX_FIRSTis zero. - The part with
maxlenaround line 626, I have not ever messed with this code, and I do not feel qualified to assess that. But it looks to me like the existing code was rather messy, and your change looks cleaner to me FWIW. Maybe we should just remove the out-commented code lines? - Same thing around line 670 and 705.
- You have added a string length argument to TRACE statements - I assume to prevent it crashing on error strings that are not NULL terminated? Looks fine to me!
As for strhelpers.cpp:
- Looks good, please remove the out-commented code line!
All in all, it looks like you have added some sensible checks to make the code safer and cleaner, and I suggest that you make a pull request!
Ok, but before I do that let me ask some more questions or give some remarks:
- You added some checks to clear data when necessary - it looks plausible to me.
This prevents the mixer-script menu to display stale data, if the slot was occupied before but deleted afterwards and not one wants to insert a new script.
assert(ref >= SCRIPT_MIX_FIRST);is unnecessary, asrefis unsigned andSCRIPT_MIX_FIRSTis zero.
Well, I think this is absolutely neccessary: at the time of writing such an assertion it looks unneccessary because the condition seems trivially true. But there is no guarantee that SCRIPT_MIX_FIRST remains 0 (that is the reason for introducing a cpp-macro anyway). So this assert checks for future modifications.
- The part with
maxlenaround line 626, I have not ever messed with this code, and I do not feel qualified to assess that. But it looks to me like the existing code was rather messy, and your change looks cleaner to me FWIW. Maybe we should just remove the out-commented code lines?
Yes, we can get rid of that.
But: the use of snprintf() here is a little bit overkill, since most of the string twiddling could be done at compile-time. I also wrote a TMP (template meta-programming) version instead of snprintf(). I know that most people / projects have strong concerns against TMP although it can save lots of CPU-cycles. So I don't know what to to here: stick to old-style snprintf() or use a typesafe, automatic-calculating the needed output-string-length TMP concat() version.
- You have added a string length argument to TRACE statements - I assume to prevent it crashing on error strings that are not NULL terminated? Looks fine to me!
Yes, that was an additional clear violation of the possibly-non-terminated-maxlength-cstrings rule.
- Looks good, please remove the out-commented code line!
Ok, I think this was the root-cause.
Please let me know your opinion.
Just realized that the project uses C++11, but for a (simple) general TMP solution of the problem one would need at least C++17.
What that comment shows, is that you know about 15 times more about C++ than I do! I am glad that we now have you onboard as a "puller", and I hope that we will see more PRs from you addressing issues like this one. I think that there are plenty of similar cases in code base!!