Inconsistent / undefined behavoiur of lfs_file_seek around LFS_FILE_MAX

[m-kostrzewa]

Hello,

consider these 4 test cases.

• [cases.test_cur_int32max] and [cases.test_end_int32max] return different values for current cursor position depending on whether we use LFS_SEEK_CUR or LFS_SEEK_END (note that CUR should be equal to END, since no additional writes or seeks were performed, but returned value is different). The return value in both cases should be the same (either INT32_MAX (preferably) or 0).
• [cases.test_ub_cur_int32max] and [cases.test_ub_set_int32max] work as expected (return error when seeking past LFS_FILE_MAX) but rely on undefined behaviour in lfs_file_seek (signed integer overflow), so they may not work as expected for every platform and compile options combination.

Checked on d01280e.

Proposed solution: compute temporary offset calculations in lfs_file_seek using int64_t before casting them to 32-bit values.

[cases.test_cur_int32max]
code = '''
    lfs_t lfs;
    lfs_format(&lfs, cfg) => 0;
    lfs_mount(&lfs, cfg) => 0;
    lfs_file_t file;
    lfs_file_open(&lfs, &file, "kitty",
            LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0;

    assert(LFS_FILE_MAX == INT32_MAX);

    lfs_file_seek(&lfs, &file, INT32_MAX, LFS_SEEK_SET) => INT32_MAX;


    // behaviour: returns INT32_MAX, not 0 (what LFS_SEEK_END returns) /////////////////////////////
    lfs_file_seek(&lfs, &file, 0, LFS_SEEK_CUR) => INT32_MAX;


    lfs_file_close(&lfs, &file) => 0;
    lfs_unmount(&lfs) => 0;
'''

[cases.test_end_int32max]
code = '''
    lfs_t lfs;
    lfs_format(&lfs, cfg) => 0;
    lfs_mount(&lfs, cfg) => 0;
    lfs_file_t file;
    lfs_file_open(&lfs, &file, "kitty",
            LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0;
    
    assert(LFS_FILE_MAX == INT32_MAX);

    lfs_file_seek(&lfs, &file, INT32_MAX, LFS_SEEK_SET) => INT32_MAX;


    // behaviour: this returns 0, not INT32_MAX (what LFS_SEEK_CUR returns) ////////////////////////
    lfs_file_seek(&lfs, &file, 0, LFS_SEEK_END) => 0;


    lfs_file_close(&lfs, &file) => 0;
    lfs_unmount(&lfs) => 0;
'''

[cases.test_ub_cur_int32max]
code = '''
    lfs_t lfs;
    lfs_format(&lfs, cfg) => 0;
    lfs_mount(&lfs, cfg) => 0;
    lfs_file_t file;
    lfs_file_open(&lfs, &file, "kitty",
            LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0;

    assert(LFS_FILE_MAX == INT32_MAX);

    lfs_file_seek(&lfs, &file, INT32_MAX, LFS_SEEK_SET) => INT32_MAX;


    // ok behaviour: returns LFS_ERR_INVAL /////////////////////////////////////////////////////////
    // lfs_file_seek(&lfs, &file, 10, LFS_SEEK_CUR) => 0; // [*]
    lfs_file_seek(&lfs, &file, 10, LFS_SEEK_CUR) => LFS_ERR_INVAL;

    // BUT, relies on UNDEFINED behaviour (signed integer overflow) on line 
    //    if ((lfs_soff_t)file->pos + off < 0) { ... }
    // when compiling with -fsanitize=undefined:
    //   lfs.c:3671:35: runtime error: signed integer overflow: 2147483647 + 10 cannot be represented in type 'int'
    // Note, you must uncomment the line marked with [*], otherwise runtime error is not reported for some reason.


    lfs_file_close(&lfs, &file) => 0;
    lfs_unmount(&lfs) => 0;
'''


[cases.test_ub_set_int32max]
code = '''
    lfs_t lfs;
    lfs_format(&lfs, cfg) => 0;
    lfs_mount(&lfs, cfg) => 0;
    lfs_file_t file;
    lfs_file_open(&lfs, &file, "kitty",
            LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0;

    assert(LFS_FILE_MAX == INT32_MAX);

    lfs_file_seek(&lfs, &file, INT32_MAX, LFS_SEEK_SET) => INT32_MAX;

    file.ctz.size = INT32_MAX; // simulate writing INT32_MAX bytes to file to force the error
    lfs_file_size(&lfs, &file) => INT32_MAX;


    // ok behaviour: returns LFS_ERR_INVAL /////////////////////////////////////////////////////////
    // lfs_file_seek(&lfs, &file, 10, LFS_SEEK_END) => 0; // [*]
    lfs_file_seek(&lfs, &file, 10, LFS_SEEK_END) => LFS_ERR_INVAL;

    // BUT, relies on UNDEFINED behaviour (signed integer overflow) on line 
    //    lfs_soff_t res = lfs_file_size_(lfs, file) + off;
    // when compiling with -fsanitize=undefined:
    //   lfs.c:3677:20: runtime error: signed integer overflow: 2147483647 + 10 cannot be represented in type 'int'
    // Note, you must uncomment the line marked with [*], otherwise runtime error is not reported for some reason.


    lfs_file_close(&lfs, &file) => 0;
    lfs_unmount(&lfs) => 0;
'''

[geky]

Hi @m-kostrzewa, thanks for creating an issue and providing reproducible tests, sorry about the late response.

It looks like @lucic71 created a PR over here to address this: #1017

Some of the trickiness is we want to avoid uint64_ts where possible since they can bring quite a penalty code-size-wise on 32-bit and 8-bit devices.

[geky]

Ah, @m-kostrzewa actually there's an explanation for the first issue:

[cases.test_cur_int32max] and [cases.test_end_int32max] return different values for current cursor position depending on whether we use LFS_SEEK_CUR or LFS_SEEK_END (note that CUR should be equal to END, since no additional writes or seeks were performed, but returned value is different). The return value in both cases should be the same (either INT32_MAX (preferably) or 0).

This matches POSIX behavior in that seek changes the current position of the file pointer, but not the size of the file. The size is not changed until a write occurs on the file.

So when you seek to LFS_SEEK_END+0, it seeks to the end of the file, which is zero since no write occurred. If the file contained some data the LFS_SEEK_END+0 would return the end of the file.

In theory you could fill the file with zeros by writing a single byte at INT32_MAX-1, or with lfs_file_truncate(&lfs, &file, INT32_MAX), but littlefs doesn't currently support sparse files so this would quickly explode.