Skip to content

Implement minimal Rust-based dynamic linker (ld-vibix.so)#868

Merged
dburkart merged 3 commits intomainfrom
m859-dynamic-linker
May 5, 2026
Merged

Implement minimal Rust-based dynamic linker (ld-vibix.so)#868
dburkart merged 3 commits intomainfrom
m859-dynamic-linker

Conversation

@dburkart
Copy link
Copy Markdown
Owner

@dburkart dburkart commented May 5, 2026

Summary

  • Implements Phase 5 dynamic linking (Implement minimal Rust-based dynamic linker (ld-vibix.so) with eager binding #859): a minimal Rust-based dynamic linker (ld-vibix.so) with eager binding
  • The linker self-relocates on entry, handles R_X86_64_RELATIVE/GLOB_DAT/JUMP_SLOT/64, loads DT_NEEDED libraries, sets up TLS, and transfers control to the main binary
  • Builds vibix_libc as a shared object (libc.so) via a new PIC target spec (x86_64-unknown-vibix-dyn.json)
  • Adds a dynamically-linked test binary (hello_dyn) with PT_INTERP = /lib/ld-vibix.so
  • Integrates all artifacts into the xtask ISO build pipeline and Limine module list

Test plan

  • cargo xtask build — kernel builds cleanly
  • cargo xtask iso — full ISO built with ld-vibix.so, libc.so, and hello_dyn included
  • readelf -h confirms ld-vibix.so is ET_DYN with entry at offset 0x974
  • readelf -l hello_dyn confirms PT_INTERP = /lib/ld-vibix.so
  • readelf -r ld-vibix.so shows R_X86_64_RELATIVE relocations (self-relocation target)
  • xtask compiles with new build functions (build_ld_vibix, build_libc_so, build_userspace_hello_dyn)

Closes #859

🤖 Generated with Claude Code

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

📝 Walkthrough

Walkthrough

Adds a minimal Rust dynamic linker (ld-vibix.so), a dynamically-linked hello test, libc as a cdylib, a PIC dynamic target spec, linker scripts, build/ISO integration, and ext2/rootfs updates to install interpreter and libraries.

Changes

Dynamic Linking Stack

Layer / File(s) Summary
Workspace & Target
Cargo.toml, x86_64-unknown-vibix-dyn.json
Workspace members gained userspace/ld_vibix and userspace/hello_dyn. New LLVM Rust target JSON enables PIC/dynamic-linking (relocation-model: "pic", dynamic-linking: true).
Linker Scripts
userspace/ld_vibix/link.ld, userspace/hello_dyn/link.ld
New linker scripts to produce ET_DYN shared-object layouts for the linker and dynamic executables, placing PHDRs, dynamic sections, and discarding debug/note sections.
ELF Types
userspace/ld_vibix/src/elf.rs
Adds #[repr(C)] ELF64 structs and constants (Ehdr/Phdr/Dyn/Sym/Rela) and helper methods for symbol decoding.
Relocation Engine
userspace/ld_vibix/src/reloc.rs
Implements relocate_object processing .rela.dyn and .rela.plt, handles R_X86_64_RELATIVE/GLOB_DAT/JUMP_SLOT/64, and performs cross-object symbol lookup with fallbacks and logging.
Serial Logging
userspace/ld_vibix/src/serial.rs
Adds minimal puts(&[u8]) to write diagnostics via syscall to FD 1.
Dynamic Linker Core
userspace/ld_vibix/src/main.rs
Implements no_std/no_main ld-vibix interpreter: self-relocation, auxv parsing, PT_LOAD mapping for DT_NEEDED libraries (breadth-first), parsing .dynamic, performing relocations for all loaded objects, TLS setup via arch_prctl, and transfer to main entry.
Dynamic libc
userspace/vibix_libc/Cargo.toml, userspace/vibix_libc/src/lib.rs
vibix_libc now emits cdylib (in addition to rlib) and adds a gated panic-handler feature providing an aborting panic handler.
Dynamically-linked Hello
userspace/hello_dyn/src/main.rs, userspace/hello_dyn/Cargo.toml
New no_std/no_main test binary embedding .interp -> /lib/ld-vibix.so, _start uses raw syscalls to write a marker message and exit.
Build/ISO Integration
xtask/src/main.rs, xtask/src/ext2_image.rs
Adds new VIBIX_USERSPACE_DYN_TARGET and build helpers to build/copy ld-vibix.so, libc.so, and userspace_hello_dyn.elf into iso_root/boot/; ext2 fixture creation now stamps /lib.
Boot Config & Fixtures
kernel/limine.conf, tests/fixtures/ext2_image.sha256
Limine boot modules list extended with ld-vibix.so, libc.so, and userspace_hello_dyn.elf. ext2 fixture checksum updated to reflect added /lib content.

Sequence Diagram

sequenceDiagram
    participant Kernel
    participant Interp as ld-vibix.so
    participant Main as hello_dyn.elf
    participant LibC as libc.so
    participant Memory as MM/Loader

    Kernel->>Interp: Load interpreter (PT_INTERP) and jump to _start (stack)
    Interp->>Interp: Self-relocate (.rela.dyn R_X86_64_RELATIVE)
    Interp->>Memory: Parse auxv, PT_PHDR/AT_ENTRY
    Interp->>Main: Read PT_DYNAMIC (DT_NEEDED)
    Interp->>Memory: mmap /lib/libc.so PT_LOAD segments
    Memory-->>Interp: libc mapped at load_base
    Interp->>Interp: Parse .dynamic for each object
    Interp->>Interp: relocate_all (process .rela.dyn/.rela.plt)
    Interp->>Kernel: arch_prctl set FS (TLS)
    Interp->>Main: restore stack, jump to entry
    Main->>Kernel: syscall write(1, "hello_dyn...") / exit(0)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

Possibly related PRs

  • dburkart/vibix#819: Related to kernel/PT_INTERP loading and interpreter handling; complements this PR's interpreter artifacts and boot wiring.
  • dburkart/vibix#611: Related ext2 fixture tooling and deterministic image generation; this PR updates ext2 population and checksum.
  • dburkart/vibix#822: Related ISO/limine interpreter staging; both PRs modify boot module wiring and ISO payloads.

🐰 A Linker's Journey
From kernel's call, the rabbit hops fast,
Relocates symbols and binds them at last.
Libraries loaded, their names intertwined,
A dynamic new world, so carefully designed. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main objective of the PR: implementing a minimal Rust-based dynamic linker (ld-vibix.so).
Description check ✅ Passed The description is directly related to the changeset, detailing the implementation of dynamic linking with specific technical accomplishments and testing evidence.
Linked Issues check ✅ Passed The PR meets the coding objectives from issue #859: builds vibix_libc as shared object, implements ld-vibix.so with self-relocation and relocation handling, creates PIC target spec, adds dynamically-linked test binary, and integrates into build pipeline.
Out of Scope Changes check ✅ Passed All changes are in scope; they directly support dynamic linking implementation: new ld_vibix crate, hello_dyn test binary, PIC target spec, vibix_libc shared object build, linker scripts, ext2 fixture updates, and build pipeline integration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch m859-dynamic-linker

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (3)
userspace/hello_dyn/Cargo.toml (1)

13-14: 💤 Low value

Consider removing unused dependency.

The vibix_libc dependency is declared but userspace/hello_dyn/src/main.rs is fully self-contained with its own panic handler and raw syscall implementations. Unless this dependency is needed for linking artifacts or future expansion, it could be removed to keep the test binary minimal.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/hello_dyn/Cargo.toml` around lines 13 - 14, The Cargo.toml
currently lists the vibix_libc dependency but userspace/hello_dyn/src/main.rs is
self-contained (provides its own panic handler and raw syscalls), so remove the
vibix_libc = { path = "../vibix_libc" } entry from the [dependencies] section of
Cargo.toml; after removing it, run a cargo build for userspace/hello_dyn to
ensure no symbol/linkage from vibix_libc (or references in main.rs) remain and
update Cargo.lock if necessary.
userspace/ld_vibix/src/main.rs (2)

270-275: 💤 Low value

Use named constants for dynamic tags.

Magic numbers 7 and 8 are used here, but elf::DT_RELA and elf::DT_RELASZ constants are used elsewhere in the file (e.g., line 345-346). Consistency would improve readability.

Proposed fix
         match tag {
-            7 => rela_off = (*d).d_val, // DT_RELA
-            8 => rela_sz = (*d).d_val,  // DT_RELASZ
+            elf::DT_RELA => rela_off = (*d).d_val,
+            elf::DT_RELASZ => rela_sz = (*d).d_val,
             _ => {}
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/ld_vibix/src/main.rs` around lines 270 - 275, Replace magic numbers
7 and 8 in the match over tag with the named ELF dynamic tag constants used
elsewhere (use elf::DT_RELA and elf::DT_RELASZ) so the branch setting rela_off
and rela_sz is consistent and readable; update the match arm keys (the match on
tag where currently "7 => rela_off = (*d).d_val" and "8 => rela_sz =
(*d).d_val") to use elf::DT_RELA and elf::DT_RELASZ respectively, leaving the
assignments and the d = d.add(1) logic unchanged.

349-354: 💤 Low value

Silent truncation of DT_NEEDED entries beyond 4.

If an ELF object has more than 4 DT_NEEDED entries, the extra dependencies are silently ignored. This could cause cryptic symbol resolution failures at runtime. Consider logging a warning when the limit is exceeded.

Proposed diagnostic
             elf::DT_NEEDED => {
                 if obj.needed_count < 4 {
                     obj.needed[obj.needed_count] = (*d).d_val;
                     obj.needed_count += 1;
+                } else {
+                    serial::puts(b"ld-vibix: warning: too many DT_NEEDED entries, some ignored\n");
                 }
             }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/ld_vibix/src/main.rs` around lines 349 - 354, The code silently
drops DT_NEEDED values when obj.needed_count >= 4 (obj.needed array), causing
missing dependency issues; change the DT_NEEDED match arm to handle the overflow
by emitting a warning when obj.needed_count is at capacity (use the project’s
logging facility e.g., log::warn or eprintln! if none) instead of silently
ignoring the entry, and optionally consider replacing the fixed-size
obj.needed/obj.needed_count with a Vec to support more entries in the future
(update uses of obj.needed and obj.needed_count accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@userspace/ld_vibix/src/main.rs`:
- Around line 442-448: The mmap'd file (created via syscall6(SYS_MMAP, ...,
file_size) returning map_addr) is never released; after parsing/loading PT_LOAD
segments and after parse_dynamic completes, add an munmap syscall (SYS_MUNMAP)
to unmap map_addr with the original file_size to avoid leaking the whole
library; ensure you only call syscall1(SYS_MUNMAP, map_addr, file_size) (or
equivalent wrapper) when map_addr is valid and loading succeeded (and skip it if
early-returned due to map_addr check), and add the SYS_MUNMAP constant alongside
SYS_MMAP so the unmap syscall can be invoked safely.
- Around line 539-543: The ptr::copy_nonoverlapping call may read past the ELF
buffer when seg_offset + copy_len > file_size; add a bounds check before copying
(using the local symbols elf_bytes, seg_offset, copy_len, and file_size) and
bail out or clamp safely if the range is invalid. Specifically, verify that
(seg_offset as usize).checked_add(copy_len as usize) exists and is <= file_size
(or convert to usize and compare) and return an error or skip the mapping if the
check fails, so ptr::copy_nonoverlapping is only ever called with a validated
source range.

In `@userspace/ld_vibix/src/reloc.rs`:
- Around line 122-157: find_symbol_in_object currently probes for a zero
sentinel and a fixed 4096 limit which is incorrect because .dynsym is not
zero-terminated; instead, parse DT_HASH or DT_GNU_HASH when loading the object,
store the symbol count on LoadedObject (e.g., a sym_count or dynsym_count field
populated during object load), and then change find_symbol_in_object to iterate
from index 0..LoadedObject.sym_count using obj.symtab + i * sizeof(Elf64Sym) to
bound the scan; also remove the sentinel/all-zero check and keep the existing
filtering (is_defined, binding) and name comparison against obj.strtab.
- Around line 43-81: The relocation handler (the match on r_type that uses
lookup_symbol, symbol_name, r.r_addend and writes *target) must fail-fast:
change the relocation processing function to return a Result and propagate
errors, and replace the places that currently write *target = 0 or silently skip
unknown types with an Err describing the unresolved or unsupported non-weak
relocation (including the relocation type and symbol). Specifically, for
R_X86_64_GLOB_DAT, R_X86_64_JUMP_SLOT and R_X86_64_64, if lookup_symbol(r_sym,
obj, all_objects) returns None then check the symbol binding (treat STB_WEAK as
allowed to become 0) and otherwise return Err instead of zeroing; for the
wildcard _ arm return Err for unknown/unsupported r_type (or handle
R_X86_64_TPOFF64 explicitly or return Err if unhandled). Ensure the caller of
the relocation function aborts startup on Err.

---

Nitpick comments:
In `@userspace/hello_dyn/Cargo.toml`:
- Around line 13-14: The Cargo.toml currently lists the vibix_libc dependency
but userspace/hello_dyn/src/main.rs is self-contained (provides its own panic
handler and raw syscalls), so remove the vibix_libc = { path = "../vibix_libc" }
entry from the [dependencies] section of Cargo.toml; after removing it, run a
cargo build for userspace/hello_dyn to ensure no symbol/linkage from vibix_libc
(or references in main.rs) remain and update Cargo.lock if necessary.

In `@userspace/ld_vibix/src/main.rs`:
- Around line 270-275: Replace magic numbers 7 and 8 in the match over tag with
the named ELF dynamic tag constants used elsewhere (use elf::DT_RELA and
elf::DT_RELASZ) so the branch setting rela_off and rela_sz is consistent and
readable; update the match arm keys (the match on tag where currently "7 =>
rela_off = (*d).d_val" and "8 => rela_sz = (*d).d_val") to use elf::DT_RELA and
elf::DT_RELASZ respectively, leaving the assignments and the d = d.add(1) logic
unchanged.
- Around line 349-354: The code silently drops DT_NEEDED values when
obj.needed_count >= 4 (obj.needed array), causing missing dependency issues;
change the DT_NEEDED match arm to handle the overflow by emitting a warning when
obj.needed_count is at capacity (use the project’s logging facility e.g.,
log::warn or eprintln! if none) instead of silently ignoring the entry, and
optionally consider replacing the fixed-size obj.needed/obj.needed_count with a
Vec to support more entries in the future (update uses of obj.needed and
obj.needed_count accordingly).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 452d392b-f609-43ed-bb48-2eae78d30a96

📥 Commits

Reviewing files that changed from the base of the PR and between 145e884 and 40a8bac.

📒 Files selected for processing (17)
  • Cargo.toml
  • kernel/limine.conf
  • tests/fixtures/ext2_image.sha256
  • userspace/hello_dyn/Cargo.toml
  • userspace/hello_dyn/link.ld
  • userspace/hello_dyn/src/main.rs
  • userspace/ld_vibix/Cargo.toml
  • userspace/ld_vibix/link.ld
  • userspace/ld_vibix/src/elf.rs
  • userspace/ld_vibix/src/main.rs
  • userspace/ld_vibix/src/reloc.rs
  • userspace/ld_vibix/src/serial.rs
  • userspace/vibix_libc/Cargo.toml
  • userspace/vibix_libc/src/lib.rs
  • x86_64-unknown-vibix-dyn.json
  • xtask/src/ext2_image.rs
  • xtask/src/main.rs

Comment thread userspace/ld_vibix/src/main.rs
Comment thread userspace/ld_vibix/src/main.rs Outdated
Comment on lines +43 to +81
match r_type {
elf::R_X86_64_NONE => {}

elf::R_X86_64_RELATIVE => {
// S + A where S = base
*target = (obj.base as i64 + r.r_addend) as u64;
}

elf::R_X86_64_GLOB_DAT | elf::R_X86_64_JUMP_SLOT => {
// Symbol lookup: find the symbol in all loaded objects.
if let Some(value) = lookup_symbol(r_sym, obj, all_objects) {
*target = value;
} else {
let name = symbol_name(r_sym, obj);
serial::puts(b"ld-vibix: unresolved symbol: ");
serial::puts(name);
serial::puts(b"\n");
// Write 0 — will fault if called. Better than leaving
// stale data.
*target = 0;
}
}

elf::R_X86_64_64 => {
// S + A where S = symbol value
if let Some(value) = lookup_symbol(r_sym, obj, all_objects) {
*target = (value as i64 + r.r_addend) as u64;
} else {
let name = symbol_name(r_sym, obj);
serial::puts(b"ld-vibix: unresolved R_X86_64_64: ");
serial::puts(name);
serial::puts(b"\n");
*target = 0;
}
}

_ => {
// Unknown relocation type — skip.
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Fail closed on relocation errors instead of zeroing/skipping them.

Unresolved GLOB_DAT/JUMP_SLOT/64 relocations are written as 0, and unknown relocation types are ignored. That lets the loader continue into a corrupted image and fail later at an arbitrary instruction. It also means the TLS relocation called out in the work item (R_X86_64_TPOFF64) is currently unhandled. Please make relocation processing return an error and abort startup for unsupported or unresolved non-weak relocations.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/ld_vibix/src/reloc.rs` around lines 43 - 81, The relocation handler
(the match on r_type that uses lookup_symbol, symbol_name, r.r_addend and writes
*target) must fail-fast: change the relocation processing function to return a
Result and propagate errors, and replace the places that currently write *target
= 0 or silently skip unknown types with an Err describing the unresolved or
unsupported non-weak relocation (including the relocation type and symbol).
Specifically, for R_X86_64_GLOB_DAT, R_X86_64_JUMP_SLOT and R_X86_64_64, if
lookup_symbol(r_sym, obj, all_objects) returns None then check the symbol
binding (treat STB_WEAK as allowed to become 0) and otherwise return Err instead
of zeroing; for the wildcard _ arm return Err for unknown/unsupported r_type (or
handle R_X86_64_TPOFF64 explicitly or return Err if unhandled). Ensure the
caller of the relocation function aborts startup on Err.

Comment on lines +122 to +157
/// Find a symbol by name in a single object's symbol table.
/// Linear search (no hash table yet — acceptable for small dep counts).
unsafe fn find_symbol_in_object(obj: &LoadedObject, name: *const u8) -> Option<u64> {
// We need to walk the symbol table. The number of entries isn't
// directly stored in .dynamic (it's implied by .hash or .gnu.hash).
// For simplicity, walk until we hit a zero entry or a reasonable limit.
// The symbol table is terminated by DT_SYMENT-aligned entries.
// A practical limit: 4096 symbols.
let sym_entry_size = core::mem::size_of::<elf::Elf64Sym>() as u64;

for i in 1..4096u32 {
let sym = (obj.symtab + i as u64 * sym_entry_size) as *const elf::Elf64Sym;

// Stop if we hit unmapped memory (rough heuristic: st_name
// would be garbage). We rely on the symbol table being
// well-formed.
if (*sym).st_name == 0 && (*sym).st_info == 0 && (*sym).st_value == 0 {
break;
}

if !(*sym).is_defined() {
continue;
}

// Only consider global/weak symbols.
let binding = (*sym).binding();
if binding != elf::STB_GLOBAL && binding != elf::STB_WEAK {
continue;
}

// Compare names.
let sym_name = (obj.strtab + (*sym).st_name as u64) as *const u8;
if cstr_eq(sym_name, name) {
return Some((*sym).st_value + obj.base);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Use a real dynsym bound instead of sentinel probing.

.dynsym is not zero-terminated. Stopping on an all-zero entry or after 4096 slots can walk past the symbol table, miss valid symbols beyond the cap, or compare names out of unrelated mapped bytes. Parse DT_HASH/DT_GNU_HASH once, store the symbol count on LoadedObject, and bound the scan to that count.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/ld_vibix/src/reloc.rs` around lines 122 - 157,
find_symbol_in_object currently probes for a zero sentinel and a fixed 4096
limit which is incorrect because .dynsym is not zero-terminated; instead, parse
DT_HASH or DT_GNU_HASH when loading the object, store the symbol count on
LoadedObject (e.g., a sym_count or dynsym_count field populated during object
load), and then change find_symbol_in_object to iterate from index
0..LoadedObject.sym_count using obj.symtab + i * sizeof(Elf64Sym) to bound the
scan; also remove the sentinel/all-zero check and keep the existing filtering
(is_defined, binding) and name comparison against obj.strtab.

claude added 3 commits May 5, 2026 07:11
Phase 5 dynamic linking (issue #859): add ld-vibix.so, a minimal
dynamic linker with eager binding that the kernel loads at
INTERP_LOAD_BASE (0x4000_0000) when a binary has PT_INTERP.

Components:
- userspace/ld_vibix/: the dynamic linker crate (ET_DYN, PIC)
  - Self-relocation via own .rela.dyn (R_X86_64_RELATIVE)
  - Symbol lookup across loaded objects (R_X86_64_GLOB_DAT,
    R_X86_64_JUMP_SLOT, R_X86_64_64)
  - PT_LOAD segment mapping for DT_NEEDED libraries
  - TLS setup (FS base via arch_prctl) when kernel didn't set it
  - Control transfer to main binary's e_entry via auxv AT_ENTRY

- vibix_libc: add cdylib crate-type + panic-handler feature so it
  can be built as libc.so (shared object) for dynamic linking

- x86_64-unknown-vibix-dyn.json: target spec variant with PIC and
  dynamic-linking enabled for building shared libraries

- userspace/hello_dyn/: dynamically-linked test binary with
  PT_INTERP = /lib/ld-vibix.so

- xtask integration: build_ld_vibix(), build_libc_so(),
  build_userspace_hello_dyn() — all included in ISO build

- ext2 rootfs: add /lib directory for runtime library lookup

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address CodeRabbit review feedback:
- Clamp copy_len to file_size to prevent OOB reads from malformed ELFs
  with inflated p_filesz or p_offset values
- munmap the temporary file mapping after PT_LOAD segments are copied
  to their final locations, avoiding a memory leak per loaded library

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dburkart dburkart force-pushed the m859-dynamic-linker branch from aba2268 to 055cb50 Compare May 5, 2026 07:12
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
userspace/hello_dyn/link.ld (2)

15-22: 🏗️ Heavy lift

Add PT_GNU_RELRO segment to protect GOT and dynamic metadata post-relocation.

The linker script currently lacks a RELRO segment, leaving writable relocation targets (.dynamic and .got sections) unprotected for the process lifetime. Adding RELRO makes these sections read-only after the dynamic linker completes relocations, a standard hardening practice.

Apply the following changes to the PHDRS and section definitions:

Proposed changes
 PHDRS
 {
     interp PT_INTERP FLAGS(4);  /* R */
     text   PT_LOAD FLAGS(5);    /* R+X */
     rodata PT_LOAD FLAGS(4);    /* R   */
     data   PT_LOAD FLAGS(6);    /* R+W */
     dynamic PT_DYNAMIC FLAGS(6); /* R+W */
+    relro PT_GNU_RELRO FLAGS(4); /* R after reloc */
 }
@@
     .dynamic : {
         *(.dynamic)
-    } :data :dynamic
+    } :data :dynamic :relro
 
     .got : {
         *(.got .got.plt)
-    } :data
+    } :data :relro
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/hello_dyn/link.ld` around lines 15 - 22, Add a PT_GNU_RELRO program
header and assign .dynamic and .got (and any GOTPLT) into it so they become
read-only after relocations: update the PHDRS block to include "relro
PT_GNU_RELRO FLAGS(4)" (read-only) and modify the section assignments (e.g., the
.dynamic and .got/.got.plt entries) to target the relro segment instead of
data/text; ensure the writable PT_LOAD (data) still contains any sections that
must remain writable during relocations and that the relro segment is ordered so
the dynamic linker can perform relocations before it becomes read-only.

32-45: ⚡ Quick win

Make PLT section placement explicit in .text output section.

Line 33 currently captures only .text*; .plt and .iplt are left to orphan-section heuristics, which can vary across linker implementations. Explicit placement makes ELF layout stable across ld/lld and prevents unexpected relocation ordering.

Additionally, add .rela.iplt to the .rela.dyn section to capture indirect PLT relocations alongside direct ones.

Proposed diff
     .text : {
         *(.text .text.*)
+        *(.plt .plt.*)
+        *(.iplt)
     } :text
@@
     .rela.dyn : {
         *(.rela.dyn .rela.dyn.*)
         *(.rela.plt)
+        *(.rela.iplt)
     } :rodata
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@userspace/hello_dyn/link.ld` around lines 32 - 45, The .text output section
currently only collects *(.text .text.*) leaving .plt and .iplt to linker
heuristics; update the .text output section to explicitly include *(.plt .plt.*
.iplt .iplt.*) so PLT entries are placed deterministically, and extend the
.rela.dyn output section to also include *(.rela.iplt .rela.iplt.*) alongside
*(.rela.dyn .rela.dyn.*) and *(.rela.plt) to ensure indirect PLT relocations are
captured; modify the section descriptor lines that reference .text, .plt/.iplt
and .rela.dyn/.rela.iplt accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@userspace/hello_dyn/link.ld`:
- Around line 15-22: Add a PT_GNU_RELRO program header and assign .dynamic and
.got (and any GOTPLT) into it so they become read-only after relocations: update
the PHDRS block to include "relro PT_GNU_RELRO FLAGS(4)" (read-only) and modify
the section assignments (e.g., the .dynamic and .got/.got.plt entries) to target
the relro segment instead of data/text; ensure the writable PT_LOAD (data) still
contains any sections that must remain writable during relocations and that the
relro segment is ordered so the dynamic linker can perform relocations before it
becomes read-only.
- Around line 32-45: The .text output section currently only collects *(.text
.text.*) leaving .plt and .iplt to linker heuristics; update the .text output
section to explicitly include *(.plt .plt.* .iplt .iplt.*) so PLT entries are
placed deterministically, and extend the .rela.dyn output section to also
include *(.rela.iplt .rela.iplt.*) alongside *(.rela.dyn .rela.dyn.*) and
*(.rela.plt) to ensure indirect PLT relocations are captured; modify the section
descriptor lines that reference .text, .plt/.iplt and .rela.dyn/.rela.iplt
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c5d3f225-8445-40b4-9809-dc7f3da79495

📥 Commits

Reviewing files that changed from the base of the PR and between 40a8bac and 055cb50.

📒 Files selected for processing (17)
  • Cargo.toml
  • kernel/limine.conf
  • tests/fixtures/ext2_image.sha256
  • userspace/hello_dyn/Cargo.toml
  • userspace/hello_dyn/link.ld
  • userspace/hello_dyn/src/main.rs
  • userspace/ld_vibix/Cargo.toml
  • userspace/ld_vibix/link.ld
  • userspace/ld_vibix/src/elf.rs
  • userspace/ld_vibix/src/main.rs
  • userspace/ld_vibix/src/reloc.rs
  • userspace/ld_vibix/src/serial.rs
  • userspace/vibix_libc/Cargo.toml
  • userspace/vibix_libc/src/lib.rs
  • x86_64-unknown-vibix-dyn.json
  • xtask/src/ext2_image.rs
  • xtask/src/main.rs
✅ Files skipped from review due to trivial changes (11)
  • userspace/ld_vibix/Cargo.toml
  • userspace/vibix_libc/Cargo.toml
  • tests/fixtures/ext2_image.sha256
  • userspace/ld_vibix/src/serial.rs
  • userspace/hello_dyn/src/main.rs
  • userspace/vibix_libc/src/lib.rs
  • x86_64-unknown-vibix-dyn.json
  • userspace/ld_vibix/src/elf.rs
  • userspace/ld_vibix/src/reloc.rs
  • xtask/src/ext2_image.rs
  • userspace/ld_vibix/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • Cargo.toml
  • userspace/hello_dyn/Cargo.toml
  • xtask/src/main.rs
  • userspace/ld_vibix/link.ld

@dburkart dburkart merged commit e764c53 into main May 5, 2026
16 checks passed
@dburkart dburkart deleted the m859-dynamic-linker branch May 5, 2026 07:19
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.

Implement minimal Rust-based dynamic linker (ld-vibix.so) with eager binding

2 participants