littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
4.92k stars 774 forks source link

Inconsistent / undefined behavoiur of lfs_file_seek around LFS_FILE_MAX #1002

Open m-kostrzewa opened 1 week ago

m-kostrzewa commented 1 week ago

Hello,

consider these 4 test cases.

Checked on d01280e64934a09ba16cac60cf9d3a37e228bb66.

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;
'''