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

Default file name for Task Info is too long #1160

Closed
jphickey opened this issue Feb 8, 2021 · 3 comments · Fixed by #1175 or #1171
Closed

Default file name for Task Info is too long #1160

jphickey opened this issue Feb 8, 2021 · 3 comments · Fixed by #1175 or #1171
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Feb 8, 2021

Describe the bug
The default filename for the task info file is defined here:

#define CFE_PLATFORM_ES_DEFAULT_TASK_LOG_FILE "/ram/cfe_es_task_info.log"

The filename portion of the string - "cfe_es_task_info.log" - is exactly 20 chars, and OSAL_CONFIG_MAX_FILE_NAME is also 20 chars, so it fails the max length test - because it needs to be less than the max for the NUL char.

To Reproduce
Build with default config, issue CFE_ES_QUERY_ALL_TASKS_CC command with no filename - which causes it to use default.
Observe error about failure to create file - error code OS_FS_ERR_NAME_TOO_LONG.

Expected behavior
Defaults should work.

System observed on:
Ubuntu 20.04

Additional context
The prefix cfe_es_ is 7 chars by itself. A simple fix would be to trim this back to just cfe_. Would recommend changing all the default filenames for consistency. ER log is already just cfe_erlog.log (no es).

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@skliper
Copy link
Contributor

skliper commented Feb 8, 2021

@dmknutsen - Was this observed in testing at all?

@skliper skliper added this to the 7.0.0 milestone Feb 8, 2021
@skliper skliper added the bug label Feb 8, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Feb 8, 2021

@skliper - if I remember correctly OS_TranslatePath() did not always enforce the file name length part correctly. The full pathname is still well within the real buffer limit (OS_MAX_PATH_LEN) so there is no overflow in either case - at least not with this example - but it is just enforcing more details than it used to.

It would be a problem if someone passed it to CFE_FS_ExtractFilenameFromPath() - which does NOT seem to check the length against any limit ... ironically the stub for this function does enforce its output is less than OS_MAX_FILE_NAME, but the real implementation does not. Probably another bug?

@skliper
Copy link
Contributor

skliper commented Feb 9, 2021

@jphickey had a side conversation w/ Dan and it turns out the default name case was not being checked as part of requirements verification since it doesn't have an explicit requirement. For this situation (functionality derived from design rather than explicit requirement), I'd expect this case to be covered in a functional test (which haven't been implemented yet for cFE). CFE_FS_ExtractFilenameFromPath() - debatable, I wouldn't call it a bug. From the API assumptions The extracted filename (including terminator) is no longer than #OS_MAX_PATH_LEN... it really doesn't have to be less than OS_MAX_FILE_NAME. We could change it (arguably more consistent with expected use), but it would get caught if used with any of the APIs that care. I don't think we really need to change it at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants