OS_rename() doesn't first check if a file is in use

[skliper]

The osfileapi.c OS_rename() doesn't check if a file is in use or not (in an entry in the global OS_FDTable) before it makes the system call to rename the file.

However, this is in contrast to OS_cp(), OS_mv(), or OS_remove() - which do check if the file is open (according to the OS_FDTable entry). If those functions see that the file is open then they return and don't alter the filesystem. So this behavior seems quite different for functions at affect a file.

The documentation of the OSAL API doesn't explicitly mention any behavior tied to a file that is in use by the OSAL for any of these functions (although the implementation is very clear for those other three functions.)

Discovered in #106, as part of #45 unit testing.

[skliper]

Imported from trac issue 93. Created by abrown4 on 2015-07-28T16:59:17, last modified: 2019-08-14T14:11:46

[skliper]

Trac comment by jphickey on 2018-05-14 16:08:11:

I concur that OSAL should be consistent here. Marking for discussion for next osal release.

That being said, I would suggest that OSAL should //not// be checking the in-use status on any of these calls. For many operating systems it doesn't matter; you can have a file open and yet still rename it on the disk, so this would be artificially limiting. In the same sense, if the file is open and the OS does not allow renaming/moving/removing an open file, it should return an error code from the system call. The OS is better suited to make this check anyway, since the file could be open by a non-OSAL application.

[skliper]

Trac comment by cdknight on 2018-07-24 13:24:59:

I agree with Joe, I say punt to the OS (should do same for cp/mv/remove)...

But it wouldn't be hard to add a compile-time platform option to define that operations like rename/mv/remove cannot be performed while CFS has the file open.

[skliper]

Trac comment by cdknight on 2019-05-23 14:50:47:

Looking at OS_cp, which does a system call to the "cp" command, this is really ugly, not realtime-friendly, and probably a security risk (if you could somehow inject a ';' into the system call, which shouldn't be difficult...Doesn't look like OS_TranslatePath catches this.)

I suggest re-writing OS_cp to do an open, a loop of read/write, and close (with the appropriate checks to ensure errors are handled.)

Also looks like OS_TranslatePath does the check for NULL and length, so the OS_{cp|mv|remove|...} should not do the same.

[skliper]

Trac comment by jhageman on 2019-05-23 15:33:17:

Submitted a new ticket with this issue, see #238.

Replying to [comment:5 cdknight]:

Looking at OS_cp, which does a system call to the "cp" command, this is really ugly, not realtime-friendly, and probably a security risk (if you could somehow inject a ';' into the system call, which shouldn't be difficult...Doesn't look like OS_TranslatePath catches this.)

I suggest re-writing OS_cp to do an open, a loop of read/write, and close (with the appropriate checks to ensure errors are handled.)

Also looks like OS_TranslatePath does the check for NULL and length, so the OS_{cp|mv|remove|...} should not do the same.

[skliper]

Trac comment by jhageman on 2019-05-29 12:24:58:

CCB 5/29/2019 - Discussed, 3 options - "wontfix", make them consistent for no check, make them consistent for all check.

Action is "wontfix" and close after updating documentation.

Suggestion is to convert the API to markdown.

[skliper]

Trac comment by jhageman on 2019-07-03 13:47:08:

Moved 4.3.0 tickets that won't make release to 4.4.0

[skliper]

Trac comment by jhageman on 2019-07-08 15:55:03:

This is a documentation update only, so revision number update. See comment:7 for action.

[skliper]

Trac comment by jhageman on 2019-07-08 15:58:56:

Note [changeset:b10d3e2] in TRAC-93+215_cp_rename was suggested but it applies to last gen and isn't applicable to current development work.

[skliper]

Trac comment by jhageman on 2019-07-26 15:13:16:

Milestone renamed

[skliper]

At first glance it looks to me like the behavior is still inconsistent, but not immediately clear. Needs detailed explanation and/or test of current behavior to resolve (or exactly what to change in documentation).

[jphickey]

The behavior of the OS_rename function while the file is open is a matter of the underlying OS and filesystem in use. Some (maybe most?) OS's do allow this but some do not (VxWorks, maybe?).

If anything, we might consider simplifying the code to remove the extra check, and run a quick test to document what happens on each of the supported platforms when attempting to rename a file that is open.

[jphickey]

To clarify though - for the OSAL documentation we should just refer to the RTOS documentation, if available, for whether or not this is allowed. Because even on the same OS the behavior can change for a variety of reasons (i.e. the file system type, its "mount" flags, read-only vs read-write, and others).

Even if we document what the MCP750 or Ubuntu Linux does, that might not hold true for other VxWorks platforms or other embedded Linux platforms that are still "posix" but employ different file systems.

[skliper]

Copy. How about at minimum:

Behavior of this API on an open file is not defined at the OSAL level due to dependencies on the underlying OS. Some will allow the related operation and others will error. Best practice is to limit use to files that are not open to maintain portability.

Further documentation/testing is certainly allowed if someone has free time but I'd rather punt due to the complexities you note.

[skliper]

Maybe improve the 2nd sentence - The underlying OS may or may not allow the related operation based on a variety of potential configurations.

[jphickey]

The proposed wording sounds good to me...

[skliper]

@klystron78 @jphickey @astrogeco @ejtimmon @acudmore @wmoleski Ping for previous discussion on different file operation behavior across OS's. Suggestion is to handle these differences operationally if possible (some attempted actions in FM may fail if the underlying OS doesn't support it, but these are reported and the alternate action can be performed). Basically the goal is to not introduce abstraction logic that can't fully replicate the expected behavior (like atomic rename). Basically for the rename with a a file already existing, the requirement and/or test could be updated to indicate the operating system dependence.