Skip to content

Commit 5035aa5

Browse files
committed
Adopted data-backed cache in lfs3_file_t to avoid undefined behavior
This fixes a strict aliasing violation in lfs3_file_sync_, where we cast the file cache -> lfs3_data_t to avoid an extra stack allocation, by modifying the file's cache struct to use an lfs3_data_t directly. - file.cache.pos -> file.cache.pos - file.cache.buffer -> file.cache.d.u.buffer_ - file.cache.size -> file.cache.d.size - (const lfs3_data_t*)&file->cache -> &file->cache.d Note the underscore_ in file.cache.d.u.buffer_. This did not fit together as well as I had hoped, due to different const expectation between the file cache and lfs3_data_t. Up until this point lfs3_data_t has only been used to refer to const data (ignoring side-band pointer casting in lfs3_mtree_traverse*), while the file cache very much contains mutable data. To work around this I added data.u.buffer_ as a mutable variant, which works, but risks an accidental const violation in the future. --- Unfortunately this does come with a minor RAM cost, since we no longer hide file.cache.pos in lfs3_data_t's buffer padding: code stack ctx before: 36844 2368 684 after: 36832 (-0.0%) 2376 (+0.3%) 684 (+0.0%) lfs3_file_t before: 164 lfs3_file_t after: 168 (+2.4%) I think it's pretty fair to call C's strict aliasing rules a real wet blanket. It would be interesting to create a -fno-strict-aliasing variant of littlefs in the future, to see how much code/RAM could be saved if we were given free reign to abuse the available memory. Probably not enough to justify the extra work, but it would be an interesting experiment.
1 parent 14d1f47 commit 5035aa5

File tree

2 files changed

+54
-56
lines changed

2 files changed

+54
-56
lines changed

lfs3.c

Lines changed: 52 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -11586,7 +11586,7 @@ static inline void lfs3_file_discardcache(lfs3_file_t *file) {
1158611586
#ifndef LFS3_KVONLY
1158711587
file->cache.pos = 0;
1158811588
#endif
11589-
file->cache.size = 0;
11589+
file->cache.d.size = 0;
1159011590
}
1159111591

1159211592
#ifndef LFS3_KVONLY
@@ -11611,7 +11611,7 @@ static inline lfs3_size_t lfs3_file_cachesize(lfs3_t *lfs3,
1161111611

1161211612
static inline lfs3_off_t lfs3_file_size_(const lfs3_file_t *file) {
1161311613
return lfs3_max(
11614-
LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.size,
11614+
LFS3_IFDEF_KVONLY(0, file->cache.pos) + file->cache.d.size,
1161511615
file->b.shrub.r.weight);
1161611616
}
1161711617

@@ -11729,17 +11729,17 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
1172911729
// the file cache, so make sure not to clobber it
1173011730
if (lfs3_o_iswrset(file->b.h.flags)) {
1173111731
file->b.h.flags |= LFS3_o_UNFLUSH;
11732-
file->cache.buffer = file->cfg->cache_buffer;
11732+
file->cache.d.u.buffer_ = file->cfg->cache_buffer;
1173311733
#ifndef LFS3_KVONLY
1173411734
file->cache.pos = 0;
1173511735
#endif
11736-
file->cache.size = file->cfg->cache_size;
11736+
file->cache.d.size = file->cfg->cache_size;
1173711737
} else if (file->cfg->cache_buffer) {
11738-
file->cache.buffer = file->cfg->cache_buffer;
11738+
file->cache.d.u.buffer_ = file->cfg->cache_buffer;
1173911739
} else {
1174011740
#ifndef LFS3_KVONLY
11741-
file->cache.buffer = lfs3_malloc(lfs3_file_cachesize(lfs3, file));
11742-
if (!file->cache.buffer) {
11741+
file->cache.d.u.buffer_ = lfs3_malloc(lfs3_file_cachesize(lfs3, file));
11742+
if (!file->cache.d.u.buffer_) {
1174311743
return LFS3_ERR_NOMEM;
1174411744
}
1174511745
#else
@@ -11821,9 +11821,9 @@ int lfs3_file_opencfg_(lfs3_t *lfs3, lfs3_file_t *file,
1182111821
// small file wrset? can we atomically commit everything in one
1182211822
// commit? currently this is only possible via lfs3_set
1182311823
if (lfs3_o_iswrset(file->b.h.flags)
11824-
&& file->cache.size <= lfs3->cfg->inline_size
11825-
&& file->cache.size <= lfs3->cfg->fragment_size
11826-
&& file->cache.size < lfs3->cfg->crystal_thresh) {
11824+
&& file->cache.d.size <= lfs3->cfg->inline_size
11825+
&& file->cache.d.size <= lfs3->cfg->fragment_size
11826+
&& file->cache.d.size < lfs3->cfg->crystal_thresh) {
1182711827
// we need to mark as unsync for sync to do anything
1182811828
file->b.h.flags |= LFS3_o_UNSYNC;
1182911829

@@ -11944,7 +11944,7 @@ static void lfs3_file_close_(lfs3_t *lfs3, const lfs3_file_t *file) {
1194411944
(void)lfs3;
1194511945
// clean up memory
1194611946
if (!file->cfg->cache_buffer) {
11947-
lfs3_free(file->cache.buffer);
11947+
lfs3_free(file->cache.d.u.buffer_);
1194811948
}
1194911949

1195011950
// are we orphaning a file?
@@ -12165,14 +12165,14 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
1216512165
lfs3_ssize_t d = lfs3_min(size, lfs3_file_size_(file) - pos_);
1216612166

1216712167
// any data in our cache?
12168-
if (pos_ < file->cache.pos + file->cache.size
12169-
&& file->cache.size != 0) {
12168+
if (pos_ < file->cache.pos + file->cache.d.size
12169+
&& file->cache.d.size != 0) {
1217012170
if (pos_ >= file->cache.pos) {
1217112171
lfs3_ssize_t d_ = lfs3_min(
1217212172
d,
12173-
file->cache.size - (pos_ - file->cache.pos));
12173+
file->cache.d.size - (pos_ - file->cache.pos));
1217412174
lfs3_memcpy(buffer_,
12175-
&file->cache.buffer[pos_ - file->cache.pos],
12175+
&file->cache.d.u.buffer_[pos_ - file->cache.pos],
1217612176
d_);
1217712177

1217812178
pos_ += d_;
@@ -12207,13 +12207,13 @@ lfs3_ssize_t lfs3_file_read(lfs3_t *lfs3, lfs3_file_t *file,
1220712207
// try to fill our cache with some data
1220812208
if (!lfs3_o_isunflush(file->b.h.flags)) {
1220912209
lfs3_ssize_t d_ = lfs3_file_readnext(lfs3, file,
12210-
pos_, file->cache.buffer, d);
12210+
pos_, file->cache.d.u.buffer_, d);
1221112211
if (d_ < 0) {
1221212212
LFS3_ASSERT(d != LFS3_ERR_NOENT);
1221312213
return d_;
1221412214
}
1221512215
file->cache.pos = pos_;
12216-
file->cache.size = d_;
12216+
file->cache.d.size = d_;
1221712217
continue;
1221812218
}
1221912219
}
@@ -13338,10 +13338,10 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1333813338
// note we need to clear the cache anyways to avoid any
1333913339
// out-of-date data
1334013340
file->cache.pos = pos + size - lfs3_file_cachesize(lfs3, file);
13341-
lfs3_memcpy(file->cache.buffer,
13341+
lfs3_memcpy(file->cache.d.u.buffer_,
1334213342
&buffer_[size - lfs3_file_cachesize(lfs3, file)],
1334313343
lfs3_file_cachesize(lfs3, file));
13344-
file->cache.size = lfs3_file_cachesize(lfs3, file);
13344+
file->cache.d.size = lfs3_file_cachesize(lfs3, file);
1334513345

1334613346
file->b.h.flags &= ~LFS3_o_UNFLUSH;
1334713347
written += size;
@@ -13363,25 +13363,25 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1336313363
//
1336413364
if (!lfs3_o_isunflush(file->b.h.flags)
1336513365
|| (pos >= file->cache.pos
13366-
&& pos <= file->cache.pos + file->cache.size
13366+
&& pos <= file->cache.pos + file->cache.d.size
1336713367
&& pos
1336813368
< file->cache.pos
1336913369
+ lfs3_file_cachesize(lfs3, file))) {
1337013370
// unused cache? we can move it where we need it
1337113371
if (!lfs3_o_isunflush(file->b.h.flags)) {
1337213372
file->cache.pos = pos;
13373-
file->cache.size = 0;
13373+
file->cache.d.size = 0;
1337413374
}
1337513375

1337613376
lfs3_size_t d = lfs3_min(
1337713377
size,
1337813378
lfs3_file_cachesize(lfs3, file)
1337913379
- (pos - file->cache.pos));
13380-
lfs3_memcpy(&file->cache.buffer[pos - file->cache.pos],
13380+
lfs3_memcpy(&file->cache.d.u.buffer_[pos - file->cache.pos],
1338113381
buffer_,
1338213382
d);
13383-
file->cache.size = lfs3_max(
13384-
file->cache.size,
13383+
file->cache.d.size = lfs3_max(
13384+
file->cache.d.size,
1338513385
pos+d - file->cache.pos);
1338613386

1338713387
file->b.h.flags |= LFS3_o_UNFLUSH;
@@ -13394,7 +13394,7 @@ lfs3_ssize_t lfs3_file_write(lfs3_t *lfs3, lfs3_file_t *file,
1339413394

1339513395
// flush our cache so the above can't fail
1339613396
err = lfs3_file_flush_(lfs3, file,
13397-
file->cache.pos, file->cache.buffer, file->cache.size);
13397+
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size);
1339813398
if (err) {
1339913399
goto failed;
1340013400
}
@@ -13458,13 +13458,13 @@ int lfs3_file_flush(lfs3_t *lfs3, lfs3_file_t *file) {
1345813458
if (lfs3_o_isunflush(file->b.h.flags)) {
1345913459
#ifdef LFS3_KVONLY
1346013460
err = lfs3_file_flushset_(lfs3, file,
13461-
file->cache.buffer, file->cache.size);
13461+
file->cache.d.u.buffer_, file->cache.d.size);
1346213462
if (err) {
1346313463
goto failed;
1346413464
}
1346513465
#else
1346613466
err = lfs3_file_flush_(lfs3, file,
13467-
file->cache.pos, file->cache.buffer, file->cache.size);
13467+
file->cache.pos, file->cache.d.u.buffer_, file->cache.d.size);
1346813468
if (err) {
1346913469
goto failed;
1347013470
}
@@ -13547,7 +13547,7 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1354713547
#ifndef LFS3_KVONLY
1354813548
LFS3_ASSERT(file->cache.pos == 0);
1354913549
#endif
13550-
LFS3_ASSERT(file->cache.size == lfs3_file_size_(file));
13550+
LFS3_ASSERT(file->cache.d.size == lfs3_file_size_(file));
1355113551

1355213552
// discard any lingering bshrub state
1355313553
#ifndef LFS3_KVONLY
@@ -13556,10 +13556,10 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1355613556
lfs3_file_discardbshrub(file);
1355713557

1355813558
// build a small shrub commit
13559-
if (file->cache.size > 0) {
13559+
if (file->cache.d.size > 0) {
1356013560
shrub_rattrs[shrub_rattr_count++] = LFS3_RATTR_DATA(
13561-
LFS3_TAG_DATA, +file->cache.size,
13562-
(const lfs3_data_t*)&file->cache);
13561+
LFS3_TAG_DATA, +file->cache.d.size,
13562+
&file->cache.d);
1356313563

1356413564
LFS3_ASSERT(shrub_rattr_count
1356513565
<= sizeof(shrub_rattrs)/sizeof(lfs3_rattr_t));
@@ -13695,14 +13695,14 @@ static int lfs3_file_sync_(lfs3_t *lfs3, lfs3_file_t *file,
1369513695
//
1369613696
// note we need to be careful if caches have different
1369713697
// sizes, prefer the most recent data in this case
13698-
lfs3_size_t d = file->cache.size - lfs3_min(
13698+
lfs3_size_t d = file->cache.d.size - lfs3_min(
1369913699
lfs3_file_cachesize(lfs3, file_),
13700-
file->cache.size);
13700+
file->cache.d.size);
1370113701
file_->cache.pos = file->cache.pos + d;
13702-
lfs3_memcpy(file_->cache.buffer,
13703-
file->cache.buffer + d,
13704-
file->cache.size - d);
13705-
file_->cache.size = file->cache.size - d;
13702+
lfs3_memcpy(file_->cache.d.u.buffer_,
13703+
file->cache.d.u.buffer_ + d,
13704+
file->cache.d.size - d);
13705+
file_->cache.d.size = file->cache.d.size - d;
1370613706

1370713707
// update any custom attrs
1370813708
for (lfs3_size_t i = 0; i < file->cfg->attr_count; i++) {
@@ -13778,10 +13778,10 @@ int lfs3_file_sync(lfs3_t *lfs3, lfs3_file_t *file) {
1377813778
// though don't flush quite yet if our file is small and can be
1377913779
// combined with sync in a single commit
1378013780
int err;
13781-
if (!(file->cache.size == lfs3_file_size_(file)
13782-
&& file->cache.size <= lfs3->cfg->inline_size
13783-
&& file->cache.size <= lfs3->cfg->fragment_size
13784-
&& file->cache.size < lfs3->cfg->crystal_thresh)) {
13781+
if (!(file->cache.d.size == lfs3_file_size_(file)
13782+
&& file->cache.d.size <= lfs3->cfg->inline_size
13783+
&& file->cache.d.size <= lfs3->cfg->fragment_size
13784+
&& file->cache.d.size < lfs3->cfg->crystal_thresh)) {
1378513785
err = lfs3_file_flush(lfs3, file);
1378613786
if (err) {
1378713787
goto failed;
@@ -13991,12 +13991,12 @@ int lfs3_file_truncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
1399113991
}
1399213992

1399313993
// truncate our cache
13994-
file->cache.size = lfs3_min(
13995-
file->cache.size,
13994+
file->cache.d.size = lfs3_min(
13995+
file->cache.d.size,
1399613996
size_ - lfs3_min(file->cache.pos, size_));
1399713997
file->cache.pos = lfs3_min(file->cache.pos, size_);
1399813998
// mark as flushed if this completely truncates our cache
13999-
if (file->cache.size == 0) {
13999+
if (file->cache.d.size == 0) {
1400014000
lfs3_file_discardcache(file);
1400114001
}
1400214002

@@ -14073,27 +14073,27 @@ int lfs3_file_fruncate(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t size_) {
1407314073
}
1407414074

1407514075
// fruncate our cache
14076-
lfs3_memmove(file->cache.buffer,
14077-
&file->cache.buffer[lfs3_min(
14076+
lfs3_memmove(file->cache.d.u.buffer_,
14077+
&file->cache.d.u.buffer_[lfs3_min(
1407814078
lfs3_smax(
1407914079
size - size_ - file->cache.pos,
1408014080
0),
14081-
file->cache.size)],
14082-
file->cache.size - lfs3_min(
14081+
file->cache.d.size)],
14082+
file->cache.d.size - lfs3_min(
1408314083
lfs3_smax(
1408414084
size - size_ - file->cache.pos,
1408514085
0),
14086-
file->cache.size));
14087-
file->cache.size -= lfs3_min(
14086+
file->cache.d.size));
14087+
file->cache.d.size -= lfs3_min(
1408814088
lfs3_smax(
1408914089
size - size_ - file->cache.pos,
1409014090
0),
14091-
file->cache.size);
14091+
file->cache.d.size);
1409214092
file->cache.pos -= lfs3_smin(
1409314093
size - size_,
1409414094
file->cache.pos);
1409514095
// mark as flushed if this completely truncates our cache
14096-
if (file->cache.size == 0) {
14096+
if (file->cache.d.size == 0) {
1409714097
lfs3_file_discardcache(file);
1409814098
}
1409914099

lfs3.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,7 @@ typedef struct lfs3_data {
636636
lfs3_size_t size;
637637
union {
638638
const uint8_t *buffer;
639+
uint8_t *buffer_;
639640
struct {
640641
lfs3_block_t block;
641642
lfs3_size_t off;
@@ -741,14 +742,11 @@ typedef struct lfs3_file {
741742
#endif
742743

743744
// in-RAM cache
744-
//
745-
// note this lines up with lfs3_data_t's buffer representation
746745
struct {
747-
lfs3_off_t size;
748-
uint8_t *buffer;
749746
#ifndef LFS3_KVONLY
750747
lfs3_off_t pos;
751748
#endif
749+
lfs3_data_t d;
752750
} cache;
753751

754752
// on-disk leaf bptr

0 commit comments

Comments
 (0)