Change the FF_MakeNameCompliant behavior (#63)

* 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:
----------
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
- [ ] 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
-----------
<!-- If any, please provide issue ID. -->

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

---------

Co-authored-by: Tobias Gruen <[email protected]>

Author: togrue

ff_dir.c

@@ -134,12 +134,6 @@ static BaseType_t FF_ValidShortChar( char cChar );
     #endif /* if ( ffconfigUNICODE_UTF16_SUPPORT != 0 ) */
 #endif /* if ( ffconfigLFN_SUPPORT != 0 ) */
 
-#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
-    static void FF_MakeNameCompliant( FF_T_WCHAR * pcName );
-#else
-    static void FF_MakeNameCompliant( char * pcName );
-#endif
-
 #if ( FF_NOSTRCASECMP == 0 )
     static portINLINE unsigned char prvToLower( unsigned char c )
     {
@@ -2875,40 +2869,48 @@ FF_Error_t FF_ExtendDirectory( FF_IOManager_t * pxIOManager,
 } /* FF_ExtendDirectory() */
 /*-----------------------------------------------------------*/
 
-static const uint8_t forbiddenChrs[] =
+/* *INDENT-OFF* */
+#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
+    static const FF_T_WCHAR forbiddenChars[] =
+#else
+    static const uint8_t forbiddenChars[] =
+#endif
 {
 /* Windows says: don't use these characters: '\/:*?"<>|'
  *  "     *     /	    :     <     >     ?    '\'    ?     | */
     0x22, 0x2A, 0x2F, 0x3A, 0x3C, 0x3E, 0x3F, 0x5C, 0x7F, 0x7C
 };
+/* *INDENT-ON* */
 
 /* *INDENT-OFF* */
 #if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
-    static void FF_MakeNameCompliant( FF_T_WCHAR * pcName )
+    BaseType_t FF_IsNameCompliant( FF_T_WCHAR * pcName )
 #else
-    static void FF_MakeNameCompliant( char * pcName )
+    BaseType_t FF_IsNameCompliant( char * pcName )
 #endif
 /* *INDENT-ON* */
 {
+    BaseType_t xReturn = pdTRUE;
     BaseType_t index;
 
-    if( ( uint8_t ) pcName[ 0 ] == FF_FAT_DELETED ) /* Support Japanese KANJI symbol0xE5. */
+    if( ( uint8_t ) pcName[ 0 ] == FF_FAT_DELETED ) /* Do not create a "deleted" entry 0xE5. */
     {
-        pcName[ 0 ] = 0x05;
+        xReturn = pdFALSE;
     }
 
     for( ; *pcName; pcName++ )
     {
-        for( index = 0; index < ( BaseType_t ) sizeof( forbiddenChrs ); index++ )
+        for( index = 0; index < ( BaseType_t ) ( sizeof( forbiddenChars ) / sizeof( forbiddenChars[ 0 ] ) ); index++ )
         {
-            if( *pcName == forbiddenChrs[ index ] )
+            if( *pcName == forbiddenChars[ index ] )
             {
-                *pcName = '_';
-                break;
+                xReturn = pdFALSE;
             }
         }
     }
-} /* FF_MakeNameCompliant() */
+
+    return xReturn;
+} /* FF_IsNameCompliant() */
 /*-----------------------------------------------------------*/
 
 FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager,
@@ -2941,15 +2943,6 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager,
     /* Round-up the number of LFN's needed: */
     xLFNCount = ( BaseType_t ) ( ( NameLen + 12 ) / 13 );
 
-    #if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
-    {
-        FF_MakeNameCompliant( pxDirEntry->pcFileName ); /* Ensure we don't break the Dir tables. */
-    }
-    #else
-    {
-        FF_MakeNameCompliant( pxDirEntry->pcFileName ); /* Ensure we don't break the Dir tables. */
-    }
-    #endif
     memset( pucEntryBuffer, 0, sizeof( pucEntryBuffer ) );
 
     #if ( ffconfigLFN_SUPPORT != 0 )
@@ -3127,7 +3120,15 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager,
     STRNCPY( xMyFile.pcFileName, pcFileName, ffconfigMAX_FILENAME - 1 );
     xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0;
 
-    xMyFile.ulObjectCluster = FF_CreateClusterChain( pxIOManager, &xError );
+    if( FF_IsNameCompliant( xMyFile.pcFileName ) == pdFALSE )
+    {
+        xError = FF_createERR( FF_ERR_FILE_INVALID_PATH, FF_CREATEFILE );
+    }
+
+    if( FF_isERR( xError ) == pdFALSE )
+    {
+        xMyFile.ulObjectCluster = FF_CreateClusterChain( pxIOManager, &xError );
+    }
 
     if( FF_isERR( xError ) == pdFALSE )
     {
@@ -3269,6 +3270,12 @@ FF_Error_t FF_CreateDirent( FF_IOManager_t * pxIOManager,
             break;
         }
 
+        if( FF_IsNameCompliant( pcDirName ) == pdFALSE )
+        {
+            xError = FF_createERR( FF_ERR_DIR_INVALID_PATH, FF_MKDIR );
+            break;
+        }
+
         xFindParams.ulDirCluster = FF_FindDir( pxIOManager, pcPath, ( uint16_t ) xIndex, &xError );
 
         if( FF_isERR( xError ) )

ff_file.c

@@ -868,6 +868,7 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager,
  *	@retval FF_ERR_FILE_DESTINATION_EXISTS if the destination file exists.
  *	@retval FF_ERR_FILE_COULD_NOT_CREATE_DIRENT if dirent creation failed (fatal error!).
  *	@retval FF_ERR_FILE_DIR_NOT_FOUND if destination directory was not found.
+ *	@retval FF_ERR_FILE_INVALID_PATH if destination path is invalid.
  *	@retval FF_ERR_FILE_SOURCE_NOT_FOUND if the source file was not found.
  *
  **/
@@ -886,11 +887,11 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager,
 #endif
 /* *INDENT-ON* */
 {
-    FF_Error_t xError;
+    FF_Error_t xError = FF_ERR_NONE;
     FF_FILE * pSrcFile, * pxDestFile;
     FF_DirEnt_t xMyFile;
     uint8_t ucEntryBuffer[ 32 ];
-    UBaseType_t xIndex;
+    size_t uxIndex = 0U;
     uint32_t ulDirCluster = 0ul;
     FF_FetchContext_t xFetchContext;
 
@@ -906,12 +907,46 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager,
     }
 
     #if ( ffconfigREMOVABLE_MEDIA != 0 )
-        else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 )
+        else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0U )
         {
             xError = FF_createERR( FF_ERR_IOMAN_DRIVER_NOMEDIUM, FF_MOVE );
         }
     #endif /* ffconfigREMOVABLE_MEDIA */
-    else
+
+    if( FF_isERR( xError ) == pdFALSE )
+    {
+        uxIndex = ( size_t ) STRLEN( szDestinationFile );
+
+        /* Find the base name. */
+        while( uxIndex != 0U )
+        {
+            if( ( szDestinationFile[ uxIndex ] == '\\' ) || ( szDestinationFile[ uxIndex ] == '/' ) )
+            {
+                break;
+            }
+
+            uxIndex--;
+        }
+
+        /* 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;
+        }
+    }
+
+    if( FF_isERR( xError ) == pdFALSE )
     {
         /* Check destination file doesn't exist! */
         pxDestFile = FF_Open( pxIOManager, szDestinationFile, FF_MODE_READ, &xError );
@@ -969,30 +1004,9 @@ static FF_FILE * prvAllocFileHandle( FF_IOManager_t * pxIOManager,
                     xMyFile.ulObjectCluster = pSrcFile->ulObjectCluster;
                     xMyFile.usCurrentItem = 0;
 
-                    xIndex = ( UBaseType_t ) STRLEN( szDestinationFile );
-
-                    while( xIndex != 0 )
-                    {
-                        if( ( szDestinationFile[ xIndex ] == '\\' ) || ( szDestinationFile[ xIndex ] == '/' ) )
-                        {
-                            break;
-                        }
-
-                        xIndex--;
-                    }
-
-                    /* Copy the base name of the destination file. */
-                    STRNCPY( xMyFile.pcFileName, ( szDestinationFile + xIndex + 1 ), ffconfigMAX_FILENAME - 1 );
-                    xMyFile.pcFileName[ ffconfigMAX_FILENAME - 1 ] = 0;
-
-                    if( xIndex == 0 )
-                    {
-                        xIndex = 1;
-                    }
-
                     /* Find the (cluster of the) directory in which the target file will be located.
                      * It must exist before calling FF_Move(). */
-                    ulDirCluster = FF_FindDir( pxIOManager, szDestinationFile, ( uint16_t ) xIndex, &xError );
+                    ulDirCluster = FF_FindDir( pxIOManager, szDestinationFile, ( uint16_t ) uxIndex, &xError );
                 }
             }
         }

include/ff_dir.h

@@ -172,6 +172,16 @@ int8_t FF_PushEntry( FF_IOManager_t * pxIOManager,
                      uint8_t * buffer,
                      void * pParam );
 
+/*
+ * FF_IsNameCompliant is a function that checks if a given path name
+ * contains any legal characters only.
+ */
+#if ( ffconfigUNICODE_UTF16_SUPPORT != 0 )
+    BaseType_t FF_IsNameCompliant( FF_T_WCHAR * pcName );
+#else
+    BaseType_t FF_IsNameCompliant( char * pcName );
+#endif
+
 static portINLINE BaseType_t FF_isEndOfDir( const uint8_t * pucEntryBuffer )
 {
     return pucEntryBuffer[ 0 ] == ( uint8_t ) 0;

include/ff_error.h

@@ -118,6 +118,7 @@
 #define FF_MKDIR                     ( ( 12 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR )
 #define FF_TRAVERSE                  ( ( 13 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR )
 #define FF_FINDDIR                   ( ( 14 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR )
+#define FF_CREATEFILE                ( ( 15 << FF_FUNCTION_SHIFT ) | FF_MODULE_DIR )
 
 /*----- FF_FILE - The FreeRTOS+FAT file handling routines. */
 #define FF_GETMODEBITS               ( ( 1 << FF_FUNCTION_SHIFT ) | FF_MODULE_FILE )