[BUG] Possible latent bug: dereference of NULL 'pxFile' in ff_file.c

[carlk3]

Describe the bug
A concise description of what the bug is. If possible, that is the code is not proprietary, please upload the code in a GitHub fork such that we can reproduce the bug.

Compiling ff_file.c with
arm-none-eabi-gcc.exe (GNU Tools for STM32 11.3.rel1.20230912-1600) 11.3.1 20220712
and option -fanalyzer I get this warning:

Lab-Project-FreeRTOS-FAT/ff_file.c:332:24: warning: dereference of NULL 'pxFile' [CWE-476] [-Wanalyzer-null-dereference]
  332 |         pxFile->ucMode = ucMode;
      |         ~~~~~~~~~~~~~~~^~~~~~~~
  'FF_Move': events 1-6
    |
    |  882 |     FF_Error_t FF_Move( FF_IOManager_t * pxIOManager,
    |      |                ^~~~~~~
    |      |                |
    |      |                (1) entry to 'FF_Move'
    |......
    |  903 |     if( pxIOManager == NULL )
    |      |       ~         
    |      |       |
    |      |       (2) following 'false' branch (when 'pxIOManager' is non-NULL)...
    |......
    |  909 |         else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 )
    |      |              ~~~
    |      |              | |
    |      |              | (4) following 'false' branch...
    |      |              (3) ...to here
    |......
    |  917 |         pxDestFile = FF_Open( pxIOManager, szDestinationFile, FF_MODE_READ, &xError );
    |      |         ~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |            |
    |      |         |            (6) calling 'FF_Open' from 'FF_Move'
    |      |         (5) ...to here
    |
    +--> 'FF_Open': events 7-16
           |
           |  227 |     FF_FILE * FF_Open( FF_IOManager_t * pxIOManager,
           |      |               ^~~~~~~
           |      |               |
           |      |               (7) entry to 'FF_Open'
           |......
           |  260 |     if( ( ucMode & FF_MODE_CREATE ) != 0 )
           |      |       ~        
           |      |       |
           |      |       (8) following 'false' branch...
           |......
           |  265 |     if( pxIOManager == NULL )
           |      |     ~~~        
           |      |     | |
           |      |     | (10) following 'false' branch (when 'pxIOManager' is non-NULL)...
           |      |     (9) ...to here
           |......
           |  273 |         else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 )
           |      |              ~~~
           |      |              | |
           |      |              | (12) following 'false' branch...
           |      |              (11) ...to here
           |......
           |  280 |         xError = FF_ERR_NONE;
           |      |         ~~~~~~ 
           |      |         |
           |      |         (13) ...to here
           |......
           |  300 |         if( xIndex == 0 )
           |      |           ~    
           |      |           |
           |      |           (14) following 'false' branch (when 'xIndex != 0')...
           |......
           |  308 |         FF_CreateShortName( &xFindParams, pcFileName );
           |      |         ~~~~~~~~~~~~~~~~~~
           |      |         |
           |      |         (15) ...to here
           |......
           |  315 |             if( ( ucMode & FF_MODE_WRITE ) != 0 )
           |      |               ~
           |      |               |
           |      |               (16) following 'false' branch...
           |
         'FF_Open': event 17
           |
           |cc1.exe:
           | (17): ...to here
           |
         'FF_Open': events 18-20
           |
           |  329 |     if( FF_isERR( xError ) == pdFALSE )
           |      |       ^
           |      |       |
           |      |       (18) following 'true' branch...
           |......
           |  332 |         pxFile->ucMode = ucMode;
           |      |         ~~~~~~~~~~~~~~~~~~~~~~~
           |      |         |              |
           |      |         |              (20) dereference of NULL 'pxFile'
           |      |         (19) ...to here
           |

Target

• Development board: [e.g. HiFive11 RevB]

NUCLEO-L496ZG

• Instruction Set Architecture: [e.g. RV32IMAC]

ARMv7-M

• IDE and version: [e.g. Freedom Studio 4.12.0.2019-08-2]

STM32CubeIDE

Version: 1.14.0
Build: 19471_20231121_1200 (UTC)

• Toolchain and version: [e.g. riscv64-unknown-elf-gcc-8.3.0-2019.08.0]

GNU Tools for STM32 11.3.rel1.20230912-1600

Host

• Host OS: [e.g. MacOS]

FreeRTOS 10.6.0

• Version: [e.g. Mojave 10.14.6]

To Reproduce

• Use project ... and configure with ...
• Run on ... and could observe ...

Expected behavior
A concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Wireshark logs
To help us identify the issue and/or reproduce it, please attach Wireshark logs if applicable.

Additional context
Add any other context about the problem here.
e.g. code snippet to reproduce the issue.
e.g. stack trace, memory dump, debugger log, and many etc.

[htibosch]

@carlk3, thanks for taking the effort to report this.

I think that it be will enough to add this line here :

         if( xFindParams.ulDirCluster == 0ul )
         {
             if( ( ucMode & FF_MODE_WRITE ) != 0 )
             {
                 FF_PRINTF( "FF_Open[%s]: Path not found\n", pcPath );
             }
 
             /* The user tries to open a file but the path leading to the file does not exist. */
+            xError = FF_createERR( FF_ERR_FILE_INVALID_PATH, FF_OPEN );
         }

and this branch will not be taken because of xError:

     if( FF_isERR( xError ) == pdFALSE )
     {
         /* Copy the Mode Bits. */
         pxFile->ucMode = ucMode;

Agreed?

[aggarg]

@htibosch I created a project to repro this issue and I can confirm that the fix suggested by you fixes this warning. Would you like to raise a PR for this?

[htibosch]

You are very quick, thank you for testing.
I will create a PR today.

[htibosch]

@aggarg @carlk3 : could it be that the compilers see a possible problem that will not occur in reality?

I tried to catch the dereference of NULL:

    configASSERT( pxFile != NULL );
    /* Copy the Mode Bits. */
    pxFile->ucMode = ucMode;

I ran the following test:

void ff_test( void )
{
    #define SOURCE_1 "/ram/file_001.txt"
    #define SOURCE_2 "/ram/file_002.txt"

    create_file(SOURCE_1, "hello_world");
    create_file(SOURCE_2, "hello_world");

    ff_mkdir( "/ram/dir_01", pdFALSE );
    /* Do not create "/ram/dir_02" */

    #define TARGET_1 "/ram/dir_01/file_001.txt"   /* Directory 'dir_01' OK. */
    #define TARGET_2 "/ram/dir_02/file_002.txt"   /* Directory 'dir_02' does not exist. */

    ff_remove( TARGET_1 );
    ff_remove( TARGET_2 );

    /* Should be OK: */
    int32_t rc1 = ff_rename( SOURCE_1, TARGET_1, pdTRUE );
    /* Should fail: */
    int32_t rc2 = ff_rename( SOURCE_2, TARGET_2, pdTRUE );

    FreeRTOS_printf( ( "rename rc = %d %d\n", ( int ) rc1, ( int ) rc2 ) );
}

I get this output when running the test 2 times:

FF_Open[/dir_02/file_002.txt]: Path not found
rename rc = 0 -1
FF_Open[/dir_02/file_002.txt]: Path not found
rename rc = 0 -1

The thing is, when FF_FindDir() returns zero:

xFindParams.ulDirCluster = FF_FindDir( pxIOManager, pcPath, ( uint16_t ) xIndex, &xError );

the error will be stored in the variable xError. I checked that and found that FF_Move() returned this error :

xError: 0x820E0022
Code:   FILE_INVALID_PATH
Module: ff_dir.c
GetErr: FF_FindDir

which I think is correct and as expected.

[carlk3]

Hi Hein,

On Mon, Jan 8, 2024 at 7:54 AM Hein Tibosch ***@***.***> wrote: @aggarg <https://github.com/aggarg> @carlk3 <https://github.com/carlk3> : could it be that the compilers see a possible problem that will not occur in reality? ... The thing is, when FF_FindDir() returns zero: xFindParams.ulDirCluster = FF_FindDir( pxIOManager, pcPath, ( uint16_t ) xIndex, &xError ); the error will be stored in the variable xError.

I don't claim to fully understand this code, but... That would seem to depend on these conditions in FF_FindDir: if( ( pcToken != NULL ) && ( ( FF_isERR( xError ) == pdFALSE ) || ( FF_GETERROR( xError ) == FF_ERR_DIR_END_OF_DIR ) ) ) { xError = FF_createERR( FF_FINDDIR, FF_ERR_FILE_INVALID_PATH ); } Could there be a case where, say, FF_isERR( xError ) == pdTRUE at that point? -Carl

[carlk3]

Oh, never mind, in that case xError is already set. So, maybe it is a false alarm.

…

On Mon, Jan 8, 2024 at 11:14 AM Carl Kugler ***@***.***> wrote: Hi Hein, On Mon, Jan 8, 2024 at 7:54 AM Hein Tibosch ***@***.***> wrote: > @aggarg <https://github.com/aggarg> @carlk3 <https://github.com/carlk3> > : could it be that the compilers see a possible problem that will not occur > in reality? > > ... > > The thing is, when FF_FindDir() returns zero: > > xFindParams.ulDirCluster = FF_FindDir( pxIOManager, pcPath, ( uint16_t ) xIndex, &xError ); > > the error will be stored in the variable xError. > I don't claim to fully understand this code, but... That would seem to depend on these conditions in FF_FindDir: if( ( pcToken != NULL ) && ( ( FF_isERR( xError ) == pdFALSE ) || ( FF_GETERROR( xError ) == FF_ERR_DIR_END_OF_DIR ) ) ) { xError = FF_createERR( FF_FINDDIR, FF_ERR_FILE_INVALID_PATH ); } Could there be a case where, say, FF_isERR( xError ) == pdTRUE at that point? -Carl

[carlk3]

A slightly different path:

  403 |         pxFile->ulDirCluster = xFindParams.ulDirCluster;
      |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
  'FF_Move': events 1-6
    |
    |  882 |     FF_Error_t FF_Move( FF_IOManager_t * pxIOManager,
    |      |                ^~~~~~~
    |      |                |
    |      |                (1) entry to 'FF_Move'
    |......
    |  903 |     if( pxIOManager == NULL )
    |      |       ~         
    |      |       |
    |      |       (2) following 'false' branch (when 'pxIOManager' is non-NULL)...
    |......
    |  909 |         else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 )
    |      |              ~~~
    |      |              | |
    |      |              | (4) following 'false' branch...
    |      |              (3) ...to here
    |......
    |  917 |         pxDestFile = FF_Open( pxIOManager, szDestinationFile, FF_MODE_READ, &xError );
    |      |         ~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |            |
    |      |         |            (6) calling 'FF_Open' from 'FF_Move'
    |      |         (5) ...to here
    |
    +--> 'FF_Open': events 7-14
           |
           |  227 |     FF_FILE * FF_Open( FF_IOManager_t * pxIOManager,
           |      |               ^~~~~~~
           |      |               |
           |      |               (7) entry to 'FF_Open'
           |......
           |  260 |     if( ( ucMode & FF_MODE_CREATE ) != 0 )
           |      |       ~        
           |      |       |
           |      |       (8) following 'false' branch...
           |......
           |  265 |     if( pxIOManager == NULL )
           |      |     ~~~        
           |      |     | |
           |      |     | (10) following 'false' branch (when 'pxIOManager' is non-NULL)...
           |      |     (9) ...to here
           |......
           |  273 |         else if( ( pxIOManager->ucFlags & FF_IOMAN_DEVICE_IS_EXTRACTED ) != 0 )
           |      |              ~~~
           |      |              | |
           |      |              | (12) following 'false' branch...
           |      |              (11) ...to here
           |......
           |  280 |         xError = FF_ERR_NONE;
           |      |         ~~~~~~ 
           |      |         |
           |      |         (13) ...to here
           |......
           |  315 |             if( ( ucMode & FF_MODE_WRITE ) != 0 )
           |      |               ~
           |      |               |
           |      |               (14) following 'false' branch...
           |
         'FF_Open': event 15
           |
           |cc1.exe:
           | (15): ...to here
           |
         'FF_Open': events 16-22
           |
           |  329 |     if( FF_isERR( xError ) == pdFALSE )
           |      |       ^
           |      |       |
           |      |       (16) following 'true' branch...
           |......
           |  332 |         pxFile->ucMode = ucMode;
           |      |         ~~~~~~
           |      |         |
           |      |         (17) ...to here
           |......
           |  384 |         else if( ( ( ucMode & ( FF_MODE_WRITE | FF_MODE_APPEND ) ) != 0 ) && ( ( xDirEntry.ucAttrib & FF_FAT_ATTR_READONLY ) != 0 ) )
           |      |                ~
           |      |                |
           |      |                (18) following 'false' branch...
           |......
           |  390 |     if( FF_isERR( xError ) == pdFALSE )
           |      |     ~~~
           |      |     | |
           |      |     | (20) following 'true' branch...
           |      |     (19) ...to here
           |  391 |     {
           |  392 |         pxFile->pxIOManager = pxIOManager;
           |      |         ~~~~~~
           |      |         |
           |      |         (21) ...to here
           |......
           |  403 |         pxFile->ulDirCluster = xFindParams.ulDirCluster;
           |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                              |
           |      |                              (22) dereference of NULL 'pxFile'
           |

[htibosch]

Did you or @aggarg find a dereference of NULL 'pxFile' message by the compiler?

Or did you see it happening in a (new) configASSERT() here?

    configASSERT( pxFile != NULL );
    /* Copy the Mode Bits. */
    pxFile->ucMode = ucMode;

[carlk3]

I haven't been able to contrive a test case that actually causes the dereference of NULL. But, I haven't spent infinite time trying. Probably, the warning can be safely ignored.

[aggarg]

Apologies for the delayed response. @htibosch I just compiled with the flag -fanalyzer and then applied the changes you suggested which made the warning disappear. If the warning is a false alarm, we can close this issue. What do you say?

[htibosch]

I think that we can close the issue. @carlk3 also confirmed so.
Thanks a lot to all who contributed.