Skip to content

RTEMS: fix off-by-one array read in cpu_ticks() - #88

Open
hjunkes wants to merge 7 commits into
epics-modules:masterfrom
hjunkes:fix-cpu-usage-offbyone
Open

RTEMS: fix off-by-one array read in cpu_ticks()#88
hjunkes wants to merge 7 commits into
epics-modules:masterfrom
hjunkes:fix-cpu-usage-offbyone

Conversation

@hjunkes

@hjunkes hjunkes commented Aug 6, 2026

Copy link
Copy Markdown

Summary

cpu_ticks() in devIocStats/os/RTEMS/osdCpuUsage.c indexes obj->local_table[y] for y in 1..obj->maximum. But OBJECTS_INFORMATION_DEFINE allocates local_table[max] with valid indices 0..max-1, since RTEMS stores each object at id_index - OBJECTS_INDEX_MINIMUM (OBJECTS_INDEX_MINIMUM is 1). So this loop:

  • reads one Thread_Control pointer past the end of local_table on every pass, and
  • never reads the object actually stored at index 0.

I found this while chasing a hard crash under RTEMS 7 on a BeagleBone Black BSP. I initially assumed it was a Thread_Control struct layout change in RTEMS 7 (see discussion on #61), but verified with offsetof/sizeof against the real RTEMS 6 and RTEMS 7 headers/compilers for that BSP that the struct layout is byte-for-byte identical. The actual cause is this off-by-one, which apparently reads harmless memory under older RTEMS memory layouts but faults under RTEMS 7's.

Fix

Index with y - 1 instead of y.

Test plan

  • Compiled osdCpuUsage.c cleanly with -Wall -Wextra against real RTEMS 6 and RTEMS 7 headers/cross-compilers for arm-rtems*-beagleboneblack.
  • Confirmed via a small offsetof/sizeof probe that Thread_Control layout (and thus cpu_time_used's offset) is unchanged between RTEMS 6 and 7 for this target, ruling out a struct-layout explanation for the original crash.

OBJECTS_INFORMATION_DEFINE allocates local_table[max] with valid
indices 0..max-1, since RTEMS stores each object at
id_index - OBJECTS_INDEX_MINIMUM (OBJECTS_INDEX_MINIMUM == 1). The
loop here indexed local_table[y] directly for y in 1..max, reading
one Thread_Control pointer past the end of the array on every pass,
and skipping the object actually stored at index 0.

Found while chasing a crash under RTEMS 7; the stale/garbage pointer
this reads happens to be more likely to fault there than under older
RTEMS memory layouts, which is why the bug went unnoticed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread devIocStats/os/RTEMS/osdCpuUsage.c Outdated
Comment on lines +104 to +108
* class (y) is 1-based (RTEMS stores objects at
* id_index - OBJECTS_INDEX_MINIMUM, and OBJECTS_INDEX_MINIMUM is 1).
* Indexing with y directly reads one entry past the end of the
* array on every pass. */
tc = (Thread_Control *)obj->local_table[y - 1];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the macro OBJECTS_INDEX_MINIMUM visible to the compiler here? If so this code should probably use that instead of the literal 1.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's visible — OBJECTS_INDEX_MINIMUM is defined in rtems/score/objectimpl.h, and _Objects_Information_table (used a couple of lines above) is declared 8 lines below it in that same header. Switched to it.

While verifying this actually compiles against a current RTEMS 7 build (2026.03.04), I found this file wasn't building at all against it, for reasons unrelated to this specific line:

  • devIocStatsOSD.h relied on <rtems.h> transitively pulling in <rtems/score/objectimpl.h> — current RTEMS no longer does that, so _Objects_Information_table/Objects_Information/OBJECTS_INDEX_MINIMUM weren't visible at all until I added the include explicitly.
  • Objects_Information::maximum has been renamed to maximum_id and is now an encoded Objects_Id (API/class/node/index bits), not a plain count. Switched to _Objects_Get_maximum_index(), which is what RTEMS's own internals use for this.
  • Thread_Control::cpu_time_used (Timestamp_Control) is int64_t on this build, not struct timespec — its representation is a per-architecture/config choice by design. Switched CPU_ELAPSED_TIME to the portable _Timestamp_Get_as_nanoseconds() accessor instead of assuming tv_sec/tv_nsec.

osdCpuUsage.c now compiles cleanly against this toolchain. Pushed as a follow-up commit.

Separately, I noticed the same kind of RTEMS-Score-API drift also breaks a few other files in this module against this RTEMS 7 build (osdSuspTasks.c: Objects_Locations no longer exists; osdWorkspaceUsage.c: _Workspace_Area undeclared), plus two unrelated failure classes (rtemsReboot implicit declaration, and -Wincompatible-pointer-types on the DSET init macros in devIocStatsAnalog/String/Waveform.c under a stricter modern GCC). Didn't touch any of those here since they're outside this PR's scope — flagging in case they're useful to know about.

hjunkes and others added 5 commits August 7, 2026 15:36
Per anjohnson's review comment on epics-modules#88: use OBJECTS_INDEX_MINIMUM
instead of the literal 1 for the local_table index adjustment.

Verifying that this and the surrounding code actually compile against
a current RTEMS 7 build (2026.03.04) surfaced two further issues, now
fixed:
- devIocStatsOSD.h relied on <rtems.h> transitively pulling in
  <rtems/score/objectimpl.h> for Objects_Information/
  _Objects_Information_table/OBJECTS_INDEX_MINIMUM; current RTEMS no
  longer does this, so include it explicitly.
- Objects_Information::maximum was renamed to maximum_id and is now an
  encoded Objects_Id (API/class/node/index), not a plain count. Use
  _Objects_Get_maximum_index() instead, matching what RTEMS's own
  internals use for this.
- Thread_Control::cpu_time_used (Timestamp_Control) is int64_t on this
  build, not struct timespec -- its representation is a per-
  architecture/config choice by design. Use the portable
  _Timestamp_Get_as_nanoseconds() accessor instead of assuming
  tv_sec/tv_nsec fields.

osdCpuUsage.c now compiles cleanly against this RTEMS 7 toolchain.
Other pre-existing RTEMS-7 compatibility issues remain elsewhere in
this module's RTEMS OSD (osdSuspTasks.c, osdWorkspaceUsage.c,
osdClustInfo.c, devIocStatsOSD.h's rtemsReboot, and unrelated
-Wincompatible-pointer-types in the Analog/String/Waveform DSETs) --
out of scope here, not touched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…MS 7

Same RTEMS-Score-API-drift theme as the previous commit, in two more
files:

osdSuspTasks.c:
- _Objects_Get_next()'s signature changed: no more separate
  Objects_Locations out-param (it now just returns NULL when nothing
  local is found), argument order is (id, information, next_id_p),
  and it internally locks the object allocator mutex on success --
  the caller must release it with _Objects_Allocator_unlock().
  Followed the reference usage in RTEMS's own
  cpukit/libcsupport/src/resource_snapshot.c.
- _RTEMS_tasks_Information is now a Thread_Information (which wraps
  an Objects_Information as its .Objects member), not an
  Objects_Information directly.
- Use the found object's own o->id for rtems_task_is_suspended()
  rather than relying on the exact value _Objects_Get_next() leaves
  in the next-id out-param.
- Needed <rtems/rtems/tasksdata.h> for _RTEMS_tasks_Information,
  which isn't pulled in transitively any more.

osdWorkspaceUsage.c:
- _Workspace_Area needed <rtems/score/wkspace.h> included explicitly,
  same transitive-include story as the previous commit.
- Configuration.work_space_size doesn't compile: the global
  `Configuration` object is only declared when the application itself
  includes <rtems/confdefs.h> (it's produced by CONFIGURE_INIT in the
  app's own source, not by any library-includable header), so a
  support library can't reach it directly. Switched to the public
  rtems_configuration_get_work_space_size() accessor instead.

Both files now compile cleanly against the current RTEMS 7 build.
Remaining unrelated pre-existing issues in this module (osdClustInfo.c,
osdIFErrors.c, devIocStatsOSD.h's rtemsReboot, and
-Wincompatible-pointer-types in the Analog/String/Waveform DSETs)
still untouched -- out of scope here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same RTEMS-Score-API-drift theme, at the libbsd network-stack boundary
this time.

osdClustInfo.c:
- The legacy network stack's `extern struct mbstat mbstat` doesn't
  exist under libbsd. Gated the old code behind
  `#if RTEMS_LIBBSD_STACK` / `#else`, and for the libbsd case query
  the same mbuf/cluster counts through libbsd's portable memstat API
  (memstat_mtl_alloc/memstat_sysctl_all/memstat_mtl_find +
  memstat_get_size/count/free), the same mechanism `netstat -m` uses.
  Also actually implements devIocStatsGetClusterUsage() for the
  libbsd case rather than returning -1 unconditionally.

  (epics-modules#61 took the same memstat approach for its
  RTEMS 6 support, but writes the mbuf_cluster stats into row [0]
  again instead of row [1], clobbering the mbuf row -- fixed that
  here.)

osdIFErrors.c:
- `extern struct ifnet *ifnet` walked via `if_next`, and
  if_ierrors/if_oerrors, are kernel-internal under libbsd (ifnet is a
  CK_STAILQ now, and the counters are behind the if_get_counter KPI) --
  neither is meant to be reachable from outside the kernel proper, so
  `__RTEMS_VIOLATE_KERNEL_VISIBILITY__` doesn't save this approach.
  Switched to getifaddrs()/struct if_data, the portable userspace-
  visible way to read the same per-interface error counters (each
  interface contributes one AF_LINK entry whose ifa_data is its
  struct if_data).

Both compile cleanly against the current RTEMS 7 build. Remaining
unrelated pre-existing issues in this module (devIocStatsOSD.h's
rtemsReboot, and -Wincompatible-pointer-types in the
Analog/String/Waveform DSETs) still untouched -- out of scope here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hains

These two issues weren't RTEMS-Score-API drift like the earlier commits
-- they blocked devIocStats.dbd/libdevIocStats.a from being produced at
all on RTEMS-beagleboneblack (the archive step never runs if any .o in
the module fails), so none of the fixes in the earlier commits were
actually reachable in a linkable library until now.

devIocStatsAnalog.c/String.c/Waveform.c:
- Their `aStats`/`sStats`/`wStats` DSET tables assign functions like
  `long ai_init(int pass)` to fields typed `DEVSUPFUN` (`long (*)()`).
  This has always been slightly-incorrect-but-tolerated: under C17 and
  earlier, empty parens in a function pointer type meant "unspecified
  arguments", compatible with any actual signature by definition. C23
  changed this -- empty parens now mean the same as `(void)` -- turning
  every one of these long-standing assignments into a genuine
  incompatible-pointer-types error under GCC 15's default C23 mode.
  This is a known, ecosystem-wide EPICS/C23 compatibility issue, not
  specific to this module. Fixed with explicit `(DEVSUPFUN)` casts at
  each assignment, the standard way to spell out the intentional type
  pun these DSET tables have always relied on.

devIocStatsOSD.h:
- `reboot(x)`'s fallback branch (taken on arm-rtems7, since none of the
  uC5282/PPC/i386-specific branches above it match) called
  `rtemsReboot()`, which no longer exists in current RTEMS. Added an
  RTEMS-6+ branch using the modern `bsp_reset(source, code)` API from
  <bsp/bootcard.h>, matching the two-argument form RTEMS 6+ needs.

Module now builds and links cleanly for RTEMS-beagleboneblack --
libdevIocStats.a actually gets produced for the first time since this
round of fixes started, and beaglePyroIOC (which depends on it) links
successfully against it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hardware-verified crash: memstat_sysctl_all() (used by the previous
commit's libbsd implementation of devIocStatsGetClusterInfo/Usage)
takes a data abort inside libbsd's UMA per-CPU counter stats path.
Confirmed via addr2line against the actual crash PC/LR from the RTEMS
fatal-exception dump:

  PC  0x8023e148 -> _bsd_counter_u64_alloc
                    (rtems-libbsd/.../freebsd/sys/kern/subr_counter.c:64)
  LR  0x802cc904 -> uma_vm_zone_stats
                    (rtems-libbsd/.../freebsd/sys/vm/uma_core.c:5718)

This is reached from a periodic devIocStats timer callback shortly
after iocInit starts (ai_clusts's driver init registers an
initHookAfterCaServerInit hook), so it crashes essentially every boot
once a CLUST_* record is loaded.

The memstat approach was adapted from epics-modules#61's
(still open, unmerged) RTEMS 6 port, which apparently was never run on
real hardware either -- it also has a row-index bug fixed in the
previous commit, independent of this crash. Something in this
RTEMS-libbsd build's counter(9)/UMA-zone-stats support is broken or
incomplete; root-causing that is future work.

Reverted to reporting "not available" (-1), matching what epics-modules#61 itself
does for the GetClusterUsage case it left unimplemented. CLUST_*
records will show INVALID/alarm rather than a real reading, but the
IOC boots and runs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hardware-verified via testAll.cmd: RAM_WS_USED (57409520) +
RAM_WS_FREE (180063944) came back ~227MB, but RAM_WS_MAX
(rtems_configuration_get_work_space_size()) reported 639056 (~624KB)
-- off by more than two orders of magnitude, badly violating the
used+free<=max invariant this field is supposed to represent.

rtems_configuration_get_work_space_size() reports the compile-time
*configured* workspace size (via the `Configuration`/`_Workspace_Size`
globals). On this BBB port (RTEMS_INIT=new) that apparently doesn't
match the actual runtime-negotiated workspace region at all -- most of
the board's RAM ends up in the workspace by the time the IOC queries
it, well past whatever was configured at link time.

Fixed by deriving numBytesTotal from the same Heap_Information_block
the Free/Used numbers already come from (info.Free.total +
info.Used.total) instead of a separately-sourced "configured" value.
This is trivially self-consistent by construction and doesn't depend
on how any given port sizes its workspace at boot.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants