Change the FF_MakeNameCompliant behavior

[togrue]

• Change the handling of invalid file-names to cause an error. Depending on the entry to create (directory/file), a different error code is returned.

Description

This changes the behavior of the FF_CreateDirent function to return an error in case of an invalid path.
Filenames with the first character of 0xE5 are still modified. (Which might have some issues. TODO)

Note: Invalid directory paths and invalid file paths now result in two different FreeRTOS+ return values. Is this the intended behavior or should only one error code be used?

case FF_ERR_FILE_INVALID_PATH:
    return pdFREERTOS_ERRNO_ENOTDIR; /* The path of the file was not found. */
case FF_ERR_DIR_INVALID_PATH:
    return pdFREERTOS_ERRNO_EINVAL; /* Could not find the directory specified by the path. */

FF_MakeNameCompliant returns a boolean and not an error-code, because the function
would need an additional parameter to select the right error code (FF_ERR_DIR_INVALID_PATH or FF_ERR_FILE_INVALID_PATH).

Test Steps

A invalid filepath "test:file.txt" resulted in setting the FF_ERR_FILE_INVALID_PATH. (Tested through breakpoint in prvFFErrorToErrno).

Todo:

• mkdir with invalid path
• handling of names starting with 0xE5

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[paulbartell]

@togrue Thanks for your contribution. To get this merged, could you run look at the build failure and address it?

[Skptak]

/bot run formatting

[togrue]

Sorry for the force-push, i thought i can modify my fork, without changing the pull-request.

[Skptak]

Thanks for raising this PR, and no issue with the force push. @cookpate @phelter how do these changes look to you?

[htibosch]

One moment please, I am working on it

[htibosch]

You chose for a different approach than I would have done. But yours is good too: you just refuse to process an invalid file name at the lowest level: at FF_CreateDirent().
I approve this PR.

[htibosch]

You wrote:

What about the case of the first filename character being '\xE5'?
The character has to be remapped to 0x05 to have no collision with the file deleted marker (which is 0xE5).

My tests showed that, filenames starting with '\xE5' still result in duplicated file-entries.

Yes, it escaped from my attention.

If we want to keep this feature, it must be checked in 3 functions:

FF_MkDir()
FF_Move()
FF_CreateFile()

And then we can as well have FF_MakeNameCompliant() return void, as it used to do. And preserve the same behaviour as before.

[htibosch]

Hi @togrue, I worked out a different approach, please see this attachment:

make_compliant.zip

FF_CheckNameCompliant() will become a global function, and it will be called as early as possible in three locations:

FF_CreateFile()
FF_MkDir()
FF_Move()

That avoids that FAT changes are made and removed, in case FF_CheckNameCompliant() failed.

Also it repairs this loop:

#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
-    for( index = 0; index < ( BaseType_t ) sizeof( forbiddenChars ); index++ )
+    for( index = 0; index < ( BaseType_t ) ( sizeof( forbiddenChars ) / sizeof ( forbiddenChars[ 0 ] ) ); index++ )

Without that the UTF16 version would not be correct.

[htibosch]

It looks good, thank you. I am testing all changes when using FF_CreateFile(), FF_MkDir(), and FF_Move(). The last function needs another change. I'll report back here.

[htibosch]

The last version of ff_move() would always fail because we were checking the compliance of a whole path including subdirectories, like /etc/file.txt, where only file.txt must be checked.

Please have a look at this version of ff_move()

The tests with FF_CreateFile() and FF_MkDir() went all well.

[togrue]

The last version of ff_move() would always fail because we were checking the compliance of a whole path including subdirectories, like /etc/file.txt, where only file.txt must be checked.

The tests with FF_CreateFile() and FF_MkDir() went all well.

Is there a testsuite, or does everyone have to rewrite all tests?

I have a failing testcase with
open(".secret.txt", "w") and mkdir(".secretdir")
It can create duplicated entries, when called multiple times. (However that should be part of another issue?)

[htibosch]

One more proposal for ff_file.c:

-    if( uxIndex == 0U )
-    {
-        /* Give the directory part a minimum
-         * length of 1, to get '/' at least. */
-        uxIndex = 1U;
-    }
 
     /* Copy the base name of the destination file. */
     STRNCPY( xMyFile.pcFileName, ( szDestinationFile + uxIndex + 1 ), ffconfigMAX_FILENAME - 1 );
     xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0;
 
     /* Now check if the target base name is compliant. */
     if( FF_IsNameCompliant( xMyFile.pcFileName ) == pdFALSE )
     {
         xError = FF_createERR( FF_ERR_FILE_INVALID_PATH, FF_MOVE );
     }

+    if( uxIndex == 0U )
+    {
+        /* Give the directory part a minimum
+         * length of 1, to get '/' at least. */
+        uxIndex = 1U;
+    }

@togrue wrote:

Is there a testsuite, or does everyone have to rewrite all tests?

Yes there are, for instance ff_stdio_tests_with_cwd.c.

Another good test that I often do is FTP copying the entire repo of Linux from a laptop to the DUT and copy it back.
After that I compare the results on my laptop, and also I put the SD-card of the DUT in a Linux machine to have it tested on correctness.

Here you find the tests that I ran today:

ff_tests.zip

[htibosch]

@togrue, you wrote:

I have a failing testcase with
open(".secret.txt", "w") and mkdir(".secretdir")
It can create duplicated entries, when called multiple times

Have you re-tested this after the last change?

[togrue]

Have you re-tested this after the #63 (comment)?

I used too long filenames without enabling LFN support.
I fear, that creating files with too long names (for the short-filename entries) should actually lead to an error.
I'd prefer not to include this fix as part of this pull-request.

[htibosch]

I fear, that creating files with too long names (for the short-filename entries) should actually lead to an error.

I must admit that I have LFN enabled in all of my projects that use +FAT.
It would be worth retesting the +FAT library while LFN is disabled.

Another option to re-test would be Unicode/UTF16.

[htibosch]

Can anybody please have a look to get this PR merged?
I think it is well tested and ready to get merged.

Thanks, Hein

[htibosch]

To summarise the PR: when an application wants to create a file with illegal characters, the file or directory will not be created.

In the current version, the illegal characters are being replaced with underscores, which was not a good idea.

Create "file:01.txt" results in "file_01.txt".
But when you want to updated "file:01.txt", the fill will not be found and a second instance is created.

So thank you @togrue for this PR. ONce again I approve it.