Repository navigation
Reject settings.toml numbers with leading zeros, as TOML requires - #11620
mikeysklar wants to merge 6 commits into
Conversation
TOML does not allow leading zeros on decimal integers or on the integer part of floats (https://toml.io/en/v1.0.0#integer). strtol base 0 read 017 as octal 15, and values like 08 or 01.5 fell through to the float parser. They now return SETTINGS_ERR_BAD_VALUE like other invalid numbers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dhalbert
left a comment
There was a problem hiding this comment.
Thanks - couple of minor points.
| if (*str == '+' || *str == '-') { | ||
| str++; | ||
| } | ||
| return str[0] == '0' && (unichar_isdigit(str[1]) || str[1] == '_'); |
There was a problem hiding this comment.
environnment.rst says underscores are not allowed, but you are checking for an underscore here.
Is the underscore restriction in the TOML spec or just an issue with our parser? I wonder if we can now handle underscores automatically.
There was a problem hiding this comment.
Underscores between digits are allowed by the TOML spec, so I added support here. 1_000 is now the int 1000, and 1__0, _1, 1_ and 1_.5 are errors. The _ in the leading zero check stays because TOML never allows 0_17.
There was a problem hiding this comment.
There isn't an automatic option. Both MicroPython parsers skip _ anywhere without checking where it is, so they accept 1__0, _1 and 1_, which TOML forbids.
mp_parse_num_integerskips any_(parsenum.c:109). It also raises and allocates, so it can't be used in settings.c.mp_parse_float_internalskips any_too (parsenum.c:367). That's why1_000used to come back as1000.0.
So settings.c checks that each _ has a digit on both sides, removes the underscores, and then parses.
There was a problem hiding this comment.
We don't need to match the TOML spec exactly. If the parsers we are using already accept underscores, just leave them be. Take out the remove_underscores(). It's extra code for basically no benefit. I was thinking that the line I have marked here could just be:
return str[0] == '0' && unichar_isdigit(str[1]);
There was a problem hiding this comment.
Done: remove_underscores() is reverted and has_leading_zero() is now your one-liner. It saves 232 bytes on the Metro RP2040.
Tested on a Metro RP2040 with and without the helper. 017, 01.5 and 08 are still rejected. These change back to how main behaves:
| settings.toml | with helper | now |
|---|---|---|
1_000 |
1000 |
1000.0 (float) |
0x1234_abcd |
305441741 |
ValueError |
0_17 |
ValueError |
17.0 |
1__0 |
ValueError |
10.0 |
So an int setting written with an underscore, like CIRCUITPY_WEB_API_PORT = 8_080, gives "Invalid format" and uses the default, same as on main.
Manual settings tests 26/26 and unix coverage 951/951 pass on 1c5e76a.
There was a problem hiding this comment.
I have pushed a PR to your branch with an alternate smaller implementation.
TOML allows leading zeros after the 0x prefix, so check that 0x000F still parses as 15. Note in environment.rst that TOML allows 0o, 0b and underscores, and only this parser does not. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TOML allows underscores between digits in integers and floats, such as 1_000 or 0x1234_abcd. Each underscore must have a digit on both sides. Before, strtol() stopped at the underscore, so 1_000 fell through to the float parser and came back as 1000.0. mp_parse_float_internal() skips any underscore, so 1__0, _1 and 1_ were also accepted. Remove underscores in place after checking each one, before parsing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dhalbert
left a comment
There was a problem hiding this comment.
What provoked this PR in the first place? Was there some case where someone had leading zeros and it was interpreted as octal?
| if (*str == '+' || *str == '-') { | ||
| str++; | ||
| } | ||
| return str[0] == '0' && (unichar_isdigit(str[1]) || str[1] == '_'); |
There was a problem hiding this comment.
We don't need to match the TOML spec exactly. If the parsers we are using already accept underscores, just leave them be. Take out the remove_underscores(). It's extra code for basically no benefit. I was thinking that the line I have marked here could just be:
return str[0] == '0' && unichar_isdigit(str[1]);
|
This was part of the test fixture design for CircuitPython. A hardware test that checks settings.toml against values TOML rejects found that It seemed worth fixing before anyone hit it. |
This reverts commit 50ee27e. The extra code is not worth it: we do not need to match the TOML underscore rules exactly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Only a digit after the leading 0 needs to be rejected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Replace strtol() in get_int() and strtoul() in read_unicode_escape() with a small digit parser. This drops strtol, strtoul, _ctype_, errno and _impure_data from the link: -660 bytes flash and -80 bytes RAM on the ItsyBitsy nRF52840, -620 bytes flash on the Feather RP2040. Integers now also accept 0o and 0b prefixes and underscores (1_000). As a superset of TOML, a sign is allowed before 0x/0o/0b, the prefix may be uppercase, and underscores are not checked to be between digits. Out-of-range integers are no longer clamped to LONG_MAX. settings_get_int() returns an error for them; supervisor.get_setting() raises ValueError for hex, octal and binary, and returns a float for decimal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
supervisor.get_setting()now raisesValueErrorfor numbers like017or01.5.Why
017was read as octal 15, and08came back as the float 8.0.TOML does not allow leading zeros (Integer, Float).
01715ValueError-017-15ValueError000ValueError088.0ValueError01.51.5ValueError017.017.0ValueError007e170.0ValueError0,-0,17,-17,+17,0x11,0x000F,0.5Startup settings like
CIRCUITPY_WEB_API_PORTnow print "Invalid format" and keep their default, like any other bad number.docs/environment.rstsaid011is 9, so that line is updated too.tests/circuitpython-manual/settingson that boardmake teston 1c5e76aOut of scope:
0o17/0b10001support, signed hex (-0x11) being accepted, underscores in ints (1_000returns1000.0), and the repeatedinfand\vin the docstring.Written with Claude Code, which also ran the tests above. I reviewed the code and this description.
🤖 Generated with Claude Code