espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.71k stars 7.3k forks source link

VFS fd translation truncates the original fd value (IDFGH-5773) #7481

Open b1ackviking opened 3 years ago

b1ackviking commented 3 years ago

Environment

Problem Description

I am trying to wrap the Reliance Edge FS with VFS.

According to the esp_vfs_t definition here, it expects the underlying FS driver to implement the POSIX file API, and the file descriptor type here is int.

The VFS implementation translates the file descriptors returned by the FS driver to the indices in the s_fd_table and vise-versa. Here is how it looks like in the code:

  1. The definition of the s_fd_table: https://github.com/espressif/esp-idf/blob/f65c8249af109de349650b9cf79ae28399261750/components/vfs/vfs.c#L42-L76
  2. A piece of code that stores the file descriptor received from the FS driver into the s_fd_table: https://github.com/espressif/esp-idf/blob/f65c8249af109de349650b9cf79ae28399261750/components/vfs/vfs.c#L405-L435
  3. A piece of code that translates the s_fd_table indices back into the FS driver's descriptors: https://github.com/espressif/esp-idf/blob/f65c8249af109de349650b9cf79ae28399261750/components/vfs/vfs.c#L298-L307
  4. An example of a code that uses the previous snippet to call the FS driver implementation with the translated file descriptor: https://github.com/espressif/esp-idf/blob/f65c8249af109de349650b9cf79ae28399261750/components/vfs/vfs.c#L437-L448

The problem is that the type local_fd_t used to store the original file descriptor in the fd_table_t is uint8_t (see the first snippet above) while the actual type of the file descriptor is int. If the value of the file descriptor returned by the open function does not fit the local_fd_t, it is silently truncated by the VFS implementation. And when we attempt to call a write function, it does not receive the original value of the file descriptor.

This is the case for Reliance Edge because it uses lower 11 bits if int to store the file descriptor and uses upper bits to store the meta-information (volume id and generation). The file descriptors are valid ints, but they are truncated when stored in the local_fd_t which is uint8_t.

Expected Behavior

The calls to the FS driver functions (read, write, lseek, close, etc) receive the same file descriptor that was returned by the open function.

Actual Behavior

The VFS fails to translate the file descriptor value as it is stored in the variable of the wrong type.

Steps to reproduce

  1. Let the FS driver's open function return the valid file descriptors with values greater than 256.
  2. Call any function that expects the original file descriptor value and see that it is truncated.

Code to reproduce this issue

#include <cstdio>

void app_main()
{
    std::FILE* f = std::fopen("hello.txt", "w"); // open() returns 1048577, VFS trancates it to 1
    if (!f)
        return;
    std::fclose(f); // close() gets 1 instead of 1048577
}
RathiSonika commented 3 months ago

In VFS, there is a limitation on the maximum number of open files (max_files), which you can configure when registering a filesystem. You can refer this example. The 'max_files' parameter defines this limit, indicating the number of available file descriptors. Due to memory constraints in embedded applications, this number cannot exceed 255 (the theoretical maximum value). Therefore, in the scenario you mentioned, it is not possible to open a file with the given file descriptor value, and it can be accommodated in a uint8_t variable.

b1ackviking commented 2 months ago

Therefore, in the scenario you mentioned, it is not possible to open a file with the given file descriptor value, and it can be accommodated in a uint8_t variable.

No, that is not correct.

The application does not exceed the max_files limit. It opens only a few (1-5) files at the same time.

The problem is that the int file descriptor from the underlying FS implementation gets truncated to uint8_t by the VFS.

Reliance Edge FS subdivides int into bit fields to store additional information as follows (not exact, it was 3 years ago):

| 11 bits volume id | 9 bits generation id | 11 bits file id |

So the first opened file gets fd == 1048577 == 0000 0000 0001 0000 0000 0000 0000 0001 which means "file 1 on volume 1 generation 0", but VFS truncates it to uint8 and looses higher bits, so subsequent calls fail due to incorrect descriptor value 1.


3 years ago we ended up fixing this by adding an additional indirection between VFS and Reliance Edge FS that maps int values to [0-255] range. But still I think it is a bug that should be fixed in VFS because it violates the POSIX interface contract: it accepts int as a file descriptor but fails to preserve its value.

RathiSonika commented 2 months ago

Got it! Thank you for the explanation. I'll take a look.