Repository navigation
Make external term encoding and decoding iterative - #2316
Conversation
e120095 to
0d41344
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0d41344 to
bbfe0c2
Compare
|
Review item 1 is worth fixing in this PR, I amended the code. |
02d765d to
acc9b54
Compare
acc9b54 to
088546a
Compare
088546a to
59991da
Compare
59991da to
afb6f8d
Compare
afb6f8d to
daf8a79
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bettio
left a comment
bettio
left a comment
There was a problem hiding this comment.
I suggest an improvement to the commit message.
The message reads as if deeply nested terms can no longer exhaust the C stack, but closures that capture closures still recurse (serialize_simple_term -> serialize_term, now with a TempStack frame per level). In a quick test with ulimit -s 512, encoding a fun() -> F end chain still crashes around depth 1750. Maybe narrow the wording to lists, tuples and maps, or mention that nested closures still recurse, so the message does not promise more than the diff does. The CHANGELOG entry on line 31 could use the same tweak.
Maybe we might add a TODO in order to make clear that there is still room for improvement from this point of view.
| - `erlang:term_to_binary/1,2` and `erlang:binary_to_term/1,2` now encode and decode terms | ||
| iteratively instead of with unbounded C recursion, so a deeply nested term no longer risks | ||
| exhausting the C stack | ||
| - `erlang:binary_to_term/1,2` now rejects a `LIST_EXT` with no tail (raising `badarg`), matching OTP |
There was a problem hiding this comment.
We already reject <<131,108,0,0,0,0>> with badarg.
What actually changes is that a zero-length LIST_EXT now decodes to its tail term like OTP 29: <<131,108,0,0,0,0,97,5>> gives 5 instead of [].
This also fixes the decoder consuming fewer bytes than the validator counted, so {LIST_EXT(0) 5, 6} decoded as {[],5} and now gives {5,6}.
Reword this entry (and let's move it to fixed) and the commit body's "Also reject LIST_EXT with no tail".
Nested lists, tuples and maps no longer recurse on the C stack. Also decode a zero-length LIST_EXT to its tail, like OTP. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
daf8a79 to
556a0ee
Compare
Until atomvm#2316 the size pass of binary_to_term/1,2 accounted only for the tag byte of a SMALL_TUPLE_EXT and not for its arity byte, so the number of remaining bytes was over-counted by one for the whole tuple. An input whose last element was missing its final byte passed validation and the decoder read one byte past the end of the input, returning it as part of the term: binary_to_term(<<131, 104, 2, 97, 1, 97>>) gave {1, X} with X taken from whatever followed the buffer, where badarg is expected. With a refc binary as input the read was outside the allocation. The rewrite of the size pass in atomvm#2316 accounts for both header bytes, but nothing in the suite exercised a truncated small tuple, so add the cases that used to fail, along with boundary checks on the maximum arity, nested empty tuples and trailing bytes with [used]. Signed-off-by: Davide Bettio <davide@uninstall.it>
binary_to_term/1andterm_to_binary/1decoded and encoded terms using unbounded C recursion, so a deeply nested term could exhaust the C stack. Observed on macOS which has a 512KB default.Also reject LIST_EXT with no tail following OTP
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later