Add value-range checks for user-definable macros at compile-time #886

BrianPugh · 2023-10-29T20:52:24Z

Added some sanity check for the user-providable #define macro values.

This was brought up due to a bug introduced by yours truly 💅 jrast/littlefs-python#61

geky-bot · 2023-10-29T21:19:46Z

Tests passed ✓, Code: 16838 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16838 B (+0.0%)	1448 B (+0.0%)	800 B (+0.0%)	Lines	2357/2533 lines (+0.0%)
Readonly	6130 B (+0.0%)	448 B (+0.0%)	800 B (+0.0%)	Branches	1202/1528 branches (+0.0%)
Threadsafe	17722 B (+0.0%)	1448 B (+0.0%)	808 B (+0.0%)		Benchmarks
Multiversion	16898 B (+0.0%)	1448 B (+0.0%)	804 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18514 B (+0.0%)	1752 B (+0.0%)	804 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17498 B (+0.0%)	1440 B (+0.0%)	800 B (+0.0%)	Erased	1568888832 B (+0.0%)

BenBE

Would be nice, to reference the design/spec for the 1022 value (I know, where it comes from, but without that ref, a casual reader of the source may believe this to be in error.

lfs.c

BenBE · 2023-10-29T22:42:17Z

lfs.c

+#endif
+
+#if (LFS_FILE_MAX > 2147483647)
+#warning "LFS_FILE_MAX>2147483647; lfs_file_seek, lfs_file_size, and lfs_file_tell will not function properly."


This is only valid in C23 and compiler-dependent before that.

Also, given that some people opt to use -Werror for compilation, this unintentionally breaks their builds.

I was a little hesitant on this as well; not sure how frequently users change LFS_FILE_MAX, would be open to alternative proposals (or even just fully removing this warning).

Curiously gcc does not warn about this, usually gcc -Werror -std=c99 -Wall -Wextra -pedantic warns about c99 compat issues. Otherwise CI would have caught this.
https://github.com/littlefs-project/littlefs/actions/runs/6685719140/job/18164313418?pr=886#step:7:50

I wonder why this is...

But @BenBE is right, there's already enough requests for C89 support, we need to stick to C99.

Alternative proposal: we can just remove the $>2^{31}$ support. I think realistically no one is using this. It's already only partially supported and untested.

It's a desperate attempt to stretch out what 32-bits can do, but I already have some plans to eventually add a 64-bit mode, and that will probably be a better way to support larger file sizes when needed.

Worst case, if someone does depend on this we can always revert.

I'm in favor of removing >2**31 support for the reasons you outlined.

BrianPugh · 2023-10-29T23:19:27Z

Would be nice, to reference the design/spec for the 1022 value (I know, where it comes from, but without that ref, a casual reader of the source may believe this to be in error.

It's actually not referenced in SPEC.md or anywhere else beyond lfs.h. I could say "reference lfs.h" in the warning if we'd like.

BenBE · 2023-10-30T11:13:39Z

Would be nice, to reference the design/spec for the 1022 value (I know, where it comes from, but without that ref, a casual reader of the source may believe this to be in error.

It's actually not referenced in SPEC.md or anywhere else beyond lfs.h. I could say "reference lfs.h" in the warning if we'd like.

Rechecked the DESIGN.md and SPEC.md and they both only mention the bit-field size this value is used in and some special values that need to be avoided. Would be nice to have those consequences to limits pointed out a bit more directly in the design/spec docs; not just in the header/implementation.

geky · 2023-10-30T18:00:14Z

Point taken SPEC.md could document these limits better, but I don't think lfs.h/lfs.c should reference SPEC.md. They should both contain copies of all relevant documentation so a reader doesn't need to jump between documents.

If lfs.h/lfs.c and SPEC.md fall out of sync you have bigger problems.

I'm curious, is there an issue with putting these in lfs.h? Then they could be next to the relevant comments.

If not (multiple include warning spam?), then maybe just a comment saying something like "Must fit in 10-bits with 1023 reserved, i.e. <= 1022." is sufficient.

geky · 2023-10-30T18:05:35Z

lfs.c

+#endif
+
+#if (LFS_FILE_MAX > 2147483647)
+#warning "LFS_FILE_MAX>2147483647; lfs_file_seek, lfs_file_size, and lfs_file_tell will not function properly."


nit: Must fit in 80-column width:

#warning "LFS_FILE_MAX>2147483647; lfs_file_seek, lfs_file_size, and \ lfs_file_tell will not function properly."

I should really get around to adding a lint for this...

BrianPugh · 2023-10-30T18:16:48Z

I'm curious, is there an issue with putting these in lfs.h? Then they could be next to the relevant comments.

I wanted to keep the header file clean and focus on the library API. I didn't want to crowd the header file with sanity checks, but I'm open to either option.

geky-bot · 2023-10-30T18:44:43Z

Tests passed ✓, Code: 16838 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16838 B (+0.0%)	1448 B (+0.0%)	800 B (+0.0%)	Lines	2357/2533 lines (+0.0%)
Readonly	6130 B (+0.0%)	448 B (+0.0%)	800 B (+0.0%)	Branches	1202/1528 branches (+0.0%)
Threadsafe	17722 B (+0.0%)	1448 B (+0.0%)	808 B (+0.0%)		Benchmarks
Multiversion	16898 B (+0.0%)	1448 B (+0.0%)	804 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18514 B (+0.0%)	1752 B (+0.0%)	804 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17498 B (+0.0%)	1440 B (+0.0%)	800 B (+0.0%)	Erased	1568888832 B (+0.0%)

geky · 2024-01-17T05:10:50Z

I wanted to keep the header file clean and focus on the library API. I didn't want to crowd the header file with sanity checks, but I'm open to either option.

That makes sense, I think you're right these should probably stay in the .c file.

Controversial thought: What if these were runtime asserts and live in lfs_init? That way they live with all of the other configuration-level asserts. And while they do have a cost at runtime... in theory runtime asserts have no cost when it matters.

Unrelated, how badly do you want this in the next minor release? I think think the next minor release can be cut pretty much whenever.

BrianPugh · 2024-01-17T05:28:07Z

What if these were runtime asserts and live in lfs_init? That way they live with all of the other configuration-level asserts.

You mean just from an organizational standpoint? Seems fine by me. This wouldn't even make it to the compiled code since these all get handled by the preprocessor, so there wouldn't be any runtime impact.

how badly do you want this in the next minor release?

I don't have any strong opinions, would just like these asserts to make it into the codebase at some point so other people avoid making stupid mistakes 😄

lfs_init handles the checks/asserts of most configuration, moving these checks near lfs_init attempts to keep all of these checks nearby each other. Also updated the comments to avoid somtimes-ambiguous range notation. And removed negative bounds checks. Negative bounds should be obviously incorrect, and 0 is _technically_ not illegal for any define (though admittedly unlikely to be correct).

I think realistically no one is using this. It's already only partially supported and untested. Worst case, if someone does depend on this we can always revert.

geky · 2024-01-17T05:56:19Z

You mean just from an organizational standpoint? Seems fine by me.

I was thinking more make these actual runtime asserts, but I've compromised by just moving them near lfs_init. Compile-configuration needs a rework anyways (#491 is related).

I've updated this PR with what was discussed in the PR. Feel free to let me know if anything looks wrong. I think this can make it into the next minor release.

One maybe controversial change: I removed the check against <=0:

LFS_FILE_MAX <= 0, zero-sized files are technically different from not-created files. It is theoretically possible to have a filesystem where every file is effectively a boolean flag of does/does-not exist. Is this reasonable? probably not. But no reason we should disallow it on principle.
LFS_NAME_MAX <= 0, a little bit harder to argue for, but technically you could create a zero-length filename in each directory. As long as the directory's filename is also zero-length. Which would imply the entire filesystem is a linked-list of zero-length directories terminating in a single file, but reasonableness aside, I don't think we should assert in this case. It's not an error as far as littlefs is concerned, just crippling and weird.

Anyways, let me know if anything looks concerning, otherwise I'm happy to merge this PR into the next minor release.

geky-bot · 2024-01-17T05:58:13Z

Tests passed ✓, Code: 16828 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16828 B (+0.0%)	1448 B (+0.0%)	800 B (+0.0%)	Lines	2357/2533 lines (+0.0%)
Readonly	6130 B (+0.0%)	448 B (+0.0%)	800 B (+0.0%)	Branches	1202/1528 branches (+0.0%)
Threadsafe	17696 B (+0.0%)	1448 B (+0.0%)	808 B (+0.0%)		Benchmarks
Multiversion	16892 B (+0.0%)	1448 B (+0.0%)	804 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18508 B (+0.0%)	1752 B (+0.0%)	804 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17484 B (+0.0%)	1440 B (+0.0%)	800 B (+0.0%)	Erased	1568888832 B (+0.0%)

BrianPugh · 2024-01-17T06:23:54Z

Looks good to me, I think your reasoning is sound. The upper limits are much more easily broken by accident, and are the more important checks.

geky · 2024-01-23T19:03:17Z

This is on master now, thanks for the PR!

Add value-range checks for user-definable macros

8f9427d

BenBE reviewed Oct 29, 2023

View reviewed changes

geky reviewed Oct 30, 2023

View reviewed changes

geky added enhancement needs minor version new functionality only allowed in minor versions labels Oct 30, 2023

Replace erroneous LFS_FILE_MAX upper bound 4294967296 to 4294967295

c531a5e

geky added 2 commits January 16, 2024 23:39

Restricted LFS_FILE_MAX to signed 32-bits, <2^31, <=2147483647

6691718

I think realistically no one is using this. It's already only partially supported and untested. Worst case, if someone does depend on this we can always revert.

geky added next minor and removed needs minor version new functionality only allowed in minor versions labels Jan 17, 2024

geky added this to the v2.9 milestone Jan 17, 2024

geky changed the base branch from master to devel January 19, 2024 18:22

geky approved these changes Jan 19, 2024

View reviewed changes

geky merged commit 1711bde into littlefs-project:devel Jan 19, 2024
93 checks passed

geky mentioned this pull request Jan 19, 2024

Minor release: v2.9 #929

Merged

geky mentioned this pull request Jan 23, 2024

Infer block_count from superblock if not provided in config. #866

Merged

BrianPugh mentioned this pull request Jan 26, 2024

is it possible to support larger sdcard by changing SIZE_MAX joltwallet/esp_littlefs#173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add value-range checks for user-definable macros at compile-time #886

Add value-range checks for user-definable macros at compile-time #886

BrianPugh commented Oct 29, 2023

geky-bot commented Oct 29, 2023

BenBE left a comment

BenBE Oct 29, 2023

BrianPugh Oct 29, 2023

geky Oct 30, 2023

BrianPugh Oct 30, 2023

BrianPugh commented Oct 29, 2023

BenBE commented Oct 30, 2023

geky commented Oct 30, 2023

geky Oct 30, 2023

BrianPugh commented Oct 30, 2023

geky-bot commented Oct 30, 2023

geky commented Jan 17, 2024

BrianPugh commented Jan 17, 2024 •

edited

geky commented Jan 17, 2024

geky-bot commented Jan 17, 2024

BrianPugh commented Jan 17, 2024

geky commented Jan 23, 2024

Add value-range checks for user-definable macros at compile-time #886

Add value-range checks for user-definable macros at compile-time #886

Conversation

BrianPugh commented Oct 29, 2023

geky-bot commented Oct 29, 2023

BenBE left a comment

Choose a reason for hiding this comment

BenBE Oct 29, 2023

Choose a reason for hiding this comment

BrianPugh Oct 29, 2023

Choose a reason for hiding this comment

geky Oct 30, 2023

Choose a reason for hiding this comment

BrianPugh Oct 30, 2023

Choose a reason for hiding this comment

BrianPugh commented Oct 29, 2023

BenBE commented Oct 30, 2023

geky commented Oct 30, 2023

geky Oct 30, 2023

Choose a reason for hiding this comment

BrianPugh commented Oct 30, 2023

geky-bot commented Oct 30, 2023

geky commented Jan 17, 2024

BrianPugh commented Jan 17, 2024 • edited

geky commented Jan 17, 2024

geky-bot commented Jan 17, 2024

BrianPugh commented Jan 17, 2024

geky commented Jan 23, 2024

BrianPugh commented Jan 17, 2024 •

edited