Updated nif erlang process info#2374
Conversation
| .. doxygenstruct:: BuiltInAtomRequestSignal | ||
| .. doxygenstruct:: ProcessInfoRequestSignal | ||
| :allow-dot-graphs: | ||
| .. doxygenstruct:: BuiltInAtomSignal |
| } | ||
| } // else: sender died | ||
|
|
||
| if (signal->len == 0) { |
There was a problem hiding this comment.
Let's make it an assert instead (or two, (signal->mode == PROCESS_INFO_LIST) and len > 0). nif_erlang_process_info doesn't send PROCESS_INFO_LIST if length is 0.
|
AMP, usual caveats: PR review:
|
ce3e299 to
712dd87
Compare
|
AMP keeps insisting on these, your call.. Follow-up review:
|
712dd87 to
054d8ce
Compare
My answers: 1. High — Reply allocation can crash the VM or leave the requester trapped foreverI don't think this really applies to the PR. The issue is eventually somewhere else. 2. Medium —
|
Signed-off-by: Mateusz Furga <mateusz.furga@swmansion.com>
The with_other_pid(Check) assertion was commented out in the original submission with no recorded reason; enabled it passes on both AtomVM and OTP (50/50 repeat runs) and it ran enabled with green CI in the FissionVM fork's merged copy of this feature. Signed-off-by: Davide Bettio <davide@uninstall.it>
process_info/1 is by OTP definition one process_info/2 list call plus a presentation rule, so implement it in Erlang (maintainer preference: Erlang over C when OTP-compliant semantics allow). This deletes from C the default_items table, the argc == 1 branch, the third signal mode PROCESS_INFO_LIST_OMIT_UNREGISTERED and both registered_name filter sites; the signal protocol keeps a clean single/list request. The /1 default item set (OTP's default set intersected with AtomVM-supported items, in OTP's relative order) moves to erlang.erl, where extending it no longer requires a VM release. The erlang_tests runner resolves modules from its own directory and does not load atomvmlib, so the built estdlib erlang.beam (and its JIT-precompiled variant) is now copied next to the test beams; otherwise process_info/1 cannot be resolved by the test suite. Behaviorally verified equivalent on all probes, including cross-process /1, death races and 2000-item lists (found-issues.md section E). Note: inside the module named erlang, guard BIFs must be written qualified (erlang:is_pid/1), as unqualified calls resolve locally. Signed-off-by: Davide Bettio <davide@uninstall.it>
term_is_pid also accepts external pids, whose boxed term was fed to term_to_local_process_id: an arbitrary process table lookup in release builds and an assert abort in debug builds. Match OTP behavior instead (verified on OTP 29): badarg for another node's pid, undefined for an external pid naming this node (a previous incarnation, so the process cannot be alive). Local pids are validated with VALIDATE_VALUE as process_flag/3 already does. Signed-off-by: Davide Bettio <davide@uninstall.it>
OTP validates the info request argument before considering whether the target process is alive, so a dead pid combined with an invalid item, an invalid list element or an improper list is a badarg, not undefined (five empirically-diffed divergences vs OTP 29, one of them a regression vs the old AtomVM code, which checked term_is_atom before the process lookup). Item validity is probed with context_get_process_info(ctx, NULL, NULL, item, NULL), which returns pure membership and cannot drift from the supported item set. The list walk also counts the length, so the later copy loop no longer revalidates. The stale own-node external pid undefined answer moves after the argument validation for the same reason. Signed-off-by: Davide Bettio <davide@uninstall.it>
The custom allocations-without-ensure-free CodeQL query (severity error) flags every context_get_process_info call in a NIF that is reachable before a memory_ensure_free barrier; the PR deleted the existing NOLINT suppression and the real CI raised two alerts on the size-pass call sites. Re-add the suppressions on the two size-pass sites and on the two new argument-validity probes, which the query reaches the same way. All four calls pass a NULL heap and cannot allocate. Signed-off-by: Davide Bettio <davide@uninstall.it>
mailbox_send_process_info_request_signal returned void and silently dropped the request on malloc failure, while the NIF unconditionally set the Trap flag afterwards, leaving the caller waiting forever for an answer that would never come. The allocation is now caller-sized (list length), so this is easier to hit on memory-constrained targets. Return bool from the send function and raise out_of_memory synchronously instead of trapping; the list path also frees the items array on that branch. Signed-off-by: Davide Bettio <davide@uninstall.it>
The get_item/2 helper asserted that the second process_info read returns exactly the value bound by the first, which for heap_size, total_heap_size and memory holds only while no GC lands between the two calls: on AtomVM each call runs memory_ensure_free_opt with MEMORY_CAN_SHRINK and on BEAM the first result allocation can trigger a minor GC, so the suite would pass almost always and then flake in CI. Volatile metrics now assert shape and invariants on both reads instead; exact double-read equality stays for stable items. The heap_size =< total_heap_size invariant is taken from one list request, which is an atomic snapshot. Signed-off-by: Davide Bettio <davide@uninstall.it>
The old test asserted that the memory item grows when messages are queued (strictly on AtomVM, non-strictly on BEAM); the rewritten test_memory only checked is_integer, dropping the only functional check that memory responds to actual usage. Restore it inside the message_queue_len choreography, which already queues messages against a blocked helper process. Also add alive-cross-process invalid argument coverage to test_badargs: previously only self and dead pids were exercised, so error delivery for another live process was untested. Signed-off-by: Davide Bettio <davide@uninstall.it>
Give process_info/1 its own edoc block (it previously sat under the process_info/2 doc comment), documenting the default key subset and the registered_name omission rule; extend the process_info/2 spec with the trap_exit item and the list-of-keys clause, and document both in the key list. Add a CHANGELOG note for the behavior change that ports are no longer accepted as process pids. Signed-off-by: Davide Bettio <davide@uninstall.it>
A ProcessInfoRequestSignal processed from context_destroy was answered with a mid-destruction snapshot: at that point the process has already been removed from the process table and unregistered, but links and monitors are still attached, so the requester could observe a cleared registered_name for a process that is no longer lookupable. Erlang/OTP answers undefined for an exiting/released process. Reply undefined instead of gathering info when the request is processed from the destroy path, which is uniquely identified by process_table_locked: context_destroy is the only caller that holds the process table write lock. Requests sent after the requester observed a DOWN or an exit already returned undefined from the process table lookup, so this only changes the answer for requests racing with the teardown itself. Verified with a 2000-iteration spawn/die race in both single-item and list mode: no hangs, answers are either complete results or undefined. Signed-off-by: Davide Bettio <davide@uninstall.it>
context_message_queue_len used mailbox_len, which counts every node of both mailbox lists: unprocessed system signals sitting in the outer list (monitor and demonitor requests, unlink and group leader signals, process_info requests, ...) were reported as queued messages. A process whose true message queue was always empty could observe message_queue_len values in the thousands under signal load, while Erlang/OTP counts only ordinary messages; queue-length based logic such as load shedding or the proc_lib crash report code can act on wildly wrong values. Add mailbox_normal_message_len, which filters the outer list on NormalMessage exactly like mailbox_size already does (the inner list only holds normal messages), and use it for message_queue_len. The new monitor/demonitor flood test fails on the previous code within a few samples and must always report zero with the fix, on AtomVM and BEAM alike. The bug predates the process_info list support; it is fixed here because process_info/1 now bundles message_queue_len into every result, widening the exposure. Signed-off-by: Davide Bettio <davide@uninstall.it>
tests/erlang_tests runs against the bare VM with no library archive, by design; functions implemented in estdlib belong in tests/libs/ estdlib, which is exactly how the estdlib-implemented apply/2,3 are already covered there. Move the process_info/1 tests (default item set, cross-process, dead pid undefined, non-pid and foreign external pid badarg) into a new tests/libs/estdlib/test_process_info.erl - run on AtomVM via test_estdlib.avm and on BEAM by the estdlib-with-BEAM CI step - and revert the copying of the built estdlib erlang.beam into the erlang_tests runner directory, which worked against that design. The erlang_tests module keeps every native process_info/2 path; the new estdlib module additionally checks that a registered process reports registered_name first. Signed-off-by: Davide Bettio <davide@uninstall.it>
nif_erlang_process_info answers [] before sending the signal, so a PROCESS_INFO_LIST request never carries an empty item list. Assert the two invariants the handler relies on instead of handling a length that cannot reach it. Signed-off-by: Davide Bettio <davide@uninstall.it>
ssize_t is POSIX and neither context.c nor nifs.c includes sys/types.h: it only resolves through transitive includes today. A size_t countdown loop needs no such type. Signed-off-by: Davide Bettio <davide@uninstall.it>
A request may repeat a dynamically sized item such as links or monitored_by, so the accumulated size is the request length times the per-item size and is not bounded by the item list itself. Allocating the heap multiplies that sum by sizeof(term), so the bound divides SIZE_MAX by it rather than comparing against SIZE_MAX. Cover the accumulation with a long repeated request, self and cross-process. Signed-off-by: Davide Bettio <davide@uninstall.it>
BuiltInAtomSignal was renamed to ImmediateSignal in a639cbc and the page was never updated, so the directive resolved to nothing. Restore the alphabetical order the process_info rename broke while here. Signed-off-by: Davide Bettio <davide@uninstall.it>
AtomsHashTable was removed in 2ab1acb ("Use valueshashtable for modules and remove atomshashtable"), but the libAtomVM API documentation still listed it, leaving a directive that resolves to nothing: a doxygen run over src/libAtomVM emits no such compound. Unrelated to process_info: found while fixing the neighbouring stale BuiltInAtomSignal entry pointed out in review. Signed-off-by: Davide Bettio <davide@uninstall.it>
A process that traps waits for exactly one completion signal. The answer path clears the flag in context_process_signal_trap_answer, but the exception path only installed the exception, so a process that caught it kept running with the flag set and parked for good at the next signal processing point, never draining its mailbox again. Reachable through erlang:process_info/2 on another process when the answer cannot be allocated, and through code_server:resume/2 when a module fails to load. Signed-off-by: Davide Bettio <davide@uninstall.it>
process_info_mode_t values used SCREAMING_SNAKE_CASE and the request signal paired its atoms[] array with a bare len field. New identifiers introduced with this work follow C_CODING_STYLE.md: exclusive alternatives are PascalCase (AVMCCS-N004, matching enum MessageType in the same header) and an array length carries the array's name (AVMCCS-N012), as the NIF side already did with items/items_len. Parameter order is unchanged: mode sits where the mailbox send family puts its discriminator. Signed-off-by: Davide Bettio <davide@uninstall.it>
context_get_process_info sets its out parameter to the 2-tuple it allocates up front on every true return, so ret cannot be anything else when the strip runs; the equivalent strip on the nif self path already relies on that. The request signal cannot carry an unvalidated key either: its only creator is the process_info nif, after item validation. Signed-off-by: Davide Bettio <davide@uninstall.it>
The "we could nevertheless filter items that do not exist" note predates the validate-before-aliveness change: invalid items now raise badarg well before the signal branch, so the comment suggests work that is already done. The trap_exit comment restated the two lines below it. The registered_name notes said "backward compatibility" for what is plain BEAM compatibility. Signed-off-by: Davide Bettio <davide@uninstall.it>
An external pid naming this node answered undefined, on the reasoning that it must belong to a previous incarnation and cannot be alive. OTP does not agree: probed on OTP 29, binary_to_term turns an own-name external pid into a local pid whatever its creation, so the only external pids that survive there are creation mismatches, and process_info raises badarg for every external pid. Match that instead of keeping the divergence. No new test: on AtomVM an own-name external pid cannot be built from Erlang (external term decode rejects a mismatched creation and localizes a matching one), so the existing foreign-pid badarg tests already cover every constructible case. Signed-off-by: Davide Bettio <davide@uninstall.it>
trap_exit answered by both request forms was never value-checked, the port badarg documented in the CHANGELOG and the nested-list badarg had no test, and mailbox_normal_message_len had no C-level coverage of the signal exclusion it exists for. The estdlib process_info/1 test also bound the registered_name-first ordering on BEAM, where OTP documents no order at all: presence is now asserted on both VMs and the first position only on AtomVM, additionally for a registered other process. Signed-off-by: Davide Bettio <davide@uninstall.it>
The rest of the series was reformatted commit by commit, but these lines were last touched by the original process_info commit, which stays verbatim, so they are wrapped here instead. The CodeQL suppression comments are shortened to fit the hundred column limit and now share one phrasing. Signed-off-by: Davide Bettio <davide@uninstall.it>
054d8ce to
b708c0c
Compare
| if (!term_is_list(argv[1])) { | ||
| // NOLINT(allocations-without-ensure-free) NULL heap call, only checks item validity | ||
| if (UNLIKELY(!term_is_atom(argv[1]) | ||
| || !context_get_process_info(ctx, NULL, NULL, argv[1], NULL))) { |
| term item = term_get_list_head(l); | ||
| // NOLINT(allocations-without-ensure-free) NULL heap call, only checks item validity | ||
| if (UNLIKELY(!term_is_atom(item) | ||
| || !context_get_process_info(ctx, NULL, NULL, item, NULL))) { |
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