fix #730 - adds padding to CFE_TBL_FileDef_t #731
Conversation
The existing build system built target executables grouped by toolchain as a proxy for CPU architecture + machine options/flags. The app binaries would be built once and copied to any/all targets sharing that toolchain. The side effect of doing this is that the application needs to be written in an CPU-agnostic manner, performing its subscriptions and configurations from runtime table data rather than hardcoded/fixed values. Unfortunately most apps are not coded that way, so workarounds were needed. This changes the top level process to include the "platform" within this target build logic, effectively treating different platform configs as entirely different builds, even if they share the same toolchain file. As a result, binaries will only be shared between targets that explicitly set the "TGTx_PLATFORM" setting in targets.cmake to the same value.
CDKnightNASA
added
CCB:Ready
|
Note that CFE_TBL_FILEDEF(), being a macro, does not have any unit tests. As such I have not put forward any tests. Perhaps for discussion whether tests should/could be developed. |
…ix-730-cfe_tbl_filedef
There was a problem hiding this comment.
Note that SB API has a utility function to deal with this: CFE_SB_MessageStringGet
This is like a version of strcpy() that has fixed sizes for both input and output buffers and deals with the null termination issues on both sides (src + dest) in a more sane manner than strncpy does (or does not, in reality).
Even though this isn't a "message" per se, all FSW code can use this function when reading any fixed size string buffer. It's pretty generic. Recommend to use that for all of these fields.
CFE_TBL_FILEDEF is a macro that maps to a static struct initialization, no code. |
|
Note also that CFE_TBL_LoadFromFile fails if the table file name is exactly CFE_MISSION_MAX_FILE_LEN bytes long. (This is cause by |
|
pushed an alternative approach of tacking on "\0" to all strings in the FILEDEF() macro, so if strlen is equal to the array size, it will generate a compiler error. |
|
(I'll squash the commits prior to the CCB meeting, assuming this approach is acceptable.) |
There was a problem hiding this comment.
The alternate take in 511b5a2 is better.
However I don't think it completely lets elf2cftbl off the hook -- when reading data from a file/structure you do not control, it must enforce null termination on strings. It's still a buffer overrun bug in that tool.
Also - just to confirm - if I remember correctly we do indicate in the mission config doxygen documentation that the actual length of these strings is one less than the configured value? If the intent is to allow all the way up to the configured value, then we need a "+1" in all the field lengths. However the FSW doesn't have a +1 so I think the practical limit is actually one less than the configured value.
Copy on elf2cfetbl I'll take a look at it today. WRT +1, yes OSAL barfs when I tried that. So yes, docs should make clear that those strings are all LEN-1. |
See #735 |
|
@CDKnightNASA can I remove the CCB label or do you still want to discuss this closed PR? |
astrogeco
removed
the
CCB:Ready
Describe the contribution
Adds a uint32 padding (initialized with 0) to CFE_TBL_FileDef_t to ensure that the filename is null-terminated.
Testing performed
Builds fine with "sample_app_table.tbl" as the filename (which is exactly the length of the Filename field.)
System(s) tested on
Debian 10.3
Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov