Skip to content

dhcp: add configurable backoff parameters for DHCPv4#593

Open
cjp256 wants to merge 1 commit intoNetworkConfiguration:masterfrom
cjp256:backoff
Open

dhcp: add configurable backoff parameters for DHCPv4#593
cjp256 wants to merge 1 commit intoNetworkConfiguration:masterfrom
cjp256:backoff

Conversation

@cjp256
Copy link
Copy Markdown

@cjp256 cjp256 commented Mar 27, 2026

In cloud and virtual environments the DHCP service is typically ready within hundreds of milliseconds of the interface coming up, and once ready, responds within single-digit milliseconds. The RFC 2131 defaults (4s initial interval, 64s backoff cap) are designed for congested broadcast networks and are unnecessarily conservative in this context.

Add three new configuration options to tune DHCPv4 retransmission:

initial_interval - initial retransmission interval (default 4s)
backoff_cutoff - exponential backoff cap (default 64s)
backoff_jitter - random jitter per retry (default ±1000ms)

Defaults match RFC 2131 so existing behaviour is unchanged. Option naming aligns with dhclient (initial-interval, backoff-cutoff). Minimum of 1 is enforced at parse time for interval and cutoff; invalid values are logged and the default is used.

These options are DHCPv4-only; DHCPv6 retransmission follows RFC 8415 constants and is not user-configurable.

A test harness (tests/backoff/test_backoff.c) was used to validate timing assumptions and correctness across multiple scenarios (defaults, min-latency, cloud-recommended) and to confirm rejection of invalid values. It requires root and network namespaces so may not be suitable for all CI environments. It can be removed if it doesn't have long-term value.

Example test output:

$ sudo ./tests/backoff/test_backoff -b ./src/dhcpcd -n 128
dhcpcd DHCPDISCOVER backoff integration test
=============================================
Binary: /home/cpatterson/git/dhcpcd/src/dhcpcd
Runs:   128 per test

--- defaults (N=128) ---
  dhcpcd.conf: timeout=75
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      1±1.0s      0.0s     1.0s     2.0s     128   PASS
  1         4±1.0s      3.0s     4.0s     5.0s     128   PASS
  2         8±1.0s      7.0s     8.0s     9.0s     128   PASS
  3         16±1.0s     15.0s    16.0s    17.0s    128   PASS
  4         32±1.0s     31.1s    32.1s    33.0s    128   PASS
  5         64±1.0s     63.0s    64.0s    65.0s    128   PASS
  Elapsed: 75.9s

--- min-latency (N=128) ---
  dhcpcd.conf: timeout=12, nodelay, initial_interval=1, backoff_cutoff=1, backoff_jitter=0
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      0           -        -        -        128   PASS
  1         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  2         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  3         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  4         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  5         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  6         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  7         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  8         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  9         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  10        1±0.0s      1.0s     1.0s     1.0s     128   PASS
  Elapsed: 12.9s

--- cloud (N=128) ---
  dhcpcd.conf: timeout=12, nodelay, initial_interval=1, backoff_cutoff=1, backoff_jitter=100
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      0           -        -        -        128   PASS
  1         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  2         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  3         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  4         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  5         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  6         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  7         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  8         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  9         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  10        1±0.1s      0.9s     1.0s     1.1s     128   PASS
  Elapsed: 13.0s

--- reject initial_interval=0 ---
  dhcpcd.conf: initial_interval 0 (expect rejection)
  PASS: dhcpcd rejected initial_interval 0 (exit=0)
    output: invalid initial interval: 0

--- reject backoff_cutoff=0 ---
  dhcpcd.conf: backoff_cutoff 0 (expect rejection)
  PASS: dhcpcd rejected backoff_cutoff 0 (exit=0)
    output: invalid backoff cutoff: 0

=============================================
All tests passed.

Closes #406

In cloud and virtual environments the DHCP service is typically
ready within hundreds of milliseconds of the interface coming up,
and once ready, responds within single-digit milliseconds.  The
RFC 2131 defaults (4s initial interval, 64s backoff cap) are
designed for congested broadcast networks and are unnecessarily
conservative in this context.

Add three new configuration options to tune DHCPv4 retransmission:

  initial_interval  - initial retransmission interval (default 4s)
  backoff_cutoff    - exponential backoff cap (default 64s)
  backoff_jitter    - random jitter per retry (default ±1000ms)

Defaults match RFC 2131 so existing behaviour is unchanged.
Option naming aligns with dhclient (initial-interval, backoff-cutoff).
Minimum of 1 is enforced at parse time for interval and cutoff;
invalid values are logged and the default is used.

These options are DHCPv4-only; DHCPv6 retransmission follows
RFC 8415 constants and is not user-configurable.

A test harness (tests/backoff/test_backoff.c) was used to validate
timing assumptions and correctness across multiple scenarios
(defaults, min-latency, cloud-recommended) and to confirm rejection
of invalid values.  It requires root and network namespaces so may
not be suitable for all CI environments.  It can be removed if it
doesn't have long-term value.

Example test output:

```
$ sudo ./tests/backoff/test_backoff -b ./src/dhcpcd -n 128
dhcpcd DHCPDISCOVER backoff integration test
=============================================
Binary: /home/cpatterson/git/dhcpcd/src/dhcpcd
Runs:   128 per test

--- defaults (N=128) ---
  dhcpcd.conf: timeout=75
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      1±1.0s      0.0s     1.0s     2.0s     128   PASS
  1         4±1.0s      3.0s     4.0s     5.0s     128   PASS
  2         8±1.0s      7.0s     8.0s     9.0s     128   PASS
  3         16±1.0s     15.0s    16.0s    17.0s    128   PASS
  4         32±1.0s     31.1s    32.1s    33.0s    128   PASS
  5         64±1.0s     63.0s    64.0s    65.0s    128   PASS
  Elapsed: 75.9s

--- min-latency (N=128) ---
  dhcpcd.conf: timeout=12, nodelay, initial_interval=1, backoff_cutoff=1, backoff_jitter=0
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      0           -        -        -        128   PASS
  1         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  2         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  3         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  4         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  5         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  6         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  7         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  8         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  9         1±0.0s      1.0s     1.0s     1.0s     128   PASS
  10        1±0.0s      1.0s     1.0s     1.0s     128   PASS
  Elapsed: 12.9s

--- cloud (N=128) ---
  dhcpcd.conf: timeout=12, nodelay, initial_interval=1, backoff_cutoff=1, backoff_jitter=100
  PASS: 128/128 runs produced data

  Retry     Expected    Min      Avg      Max      N     Result
  ------------------------------------------------------
  Init      0           -        -        -        128   PASS
  1         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  2         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  3         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  4         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  5         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  6         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  7         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  8         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  9         1±0.1s      0.9s     1.0s     1.1s     128   PASS
  10        1±0.1s      0.9s     1.0s     1.1s     128   PASS
  Elapsed: 13.0s

--- reject initial_interval=0 ---
  dhcpcd.conf: initial_interval 0 (expect rejection)
  PASS: dhcpcd rejected initial_interval 0 (exit=0)
    output: invalid initial interval: 0

--- reject backoff_cutoff=0 ---
  dhcpcd.conf: backoff_cutoff 0 (expect rejection)
  PASS: dhcpcd rejected backoff_cutoff 0 (exit=0)
    output: invalid backoff cutoff: 0

=============================================
All tests passed.
```

Closes NetworkConfiguration#406

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Walkthrough

Introduces three new DHCPv4-only configuration directives (initial_interval, backoff_cutoff, backoff_jitter) to replace hardcoded retransmit backoff parameters with configurable values. Includes configuration parsing, documentation, and comprehensive integration tests validating exponential backoff behavior across multiple scenarios.

Changes

Cohort / File(s) Summary
Header and Constants
src/if-options.h
Added three default macros (DEFAULT_INITIAL_INTERVAL=4, DEFAULT_BACKOFF_CUTOFF=64, DEFAULT_BACKOFF_JITTER=1000), three option identifiers (O_INITIAL_INTERVAL, O_BACKOFF_CUTOFF, O_BACKOFF_JITTER), and three new uint32_t fields to struct if_options.
Configuration Parsing
src/if-options.c
Added command-line/config-file option handlers for three new directives with unsigned integer parsing, lower-bound validation, and default initialization within #ifdef INET blocks.
Core DHCP Logic
src/dhcp.c
Replaced hardcoded retransmit parameters in send_message() with configurable values from ifo->: initial interval (4 → ifo->initial_interval), backoff cutoff (64 → ifo->backoff_cutoff), and jitter (fixed ±1 sec → ifo->backoff_jitter). Removed trailing whitespace in make_message().
Documentation
src/dhcpcd.conf.5.in
Added manpage entries for three new DHCPv4-only options documenting minimum values, defaults, and retransmission behavior (exponential backoff with configurable cap and optional jitter).
Test Infrastructure
tests/Makefile, tests/backoff/Makefile
Extended SUBDIRS to include new backoff test directory; added makefile for compiling test_backoff with math library linkage and test/clean targets.
Integration Tests
tests/backoff/test_backoff.c
New root-only integration test (898 lines) validating DHCPDISCOVER exponential backoff behavior in isolated Linux namespaces across multiple scenarios: standard backoff, custom intervals, jitter effects, and invalid option detection. Supports parallel runs with configurable binary path, run count, and scenario selection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'dhcp: add configurable backoff parameters for DHCPv4' clearly and concisely summarizes the main change—adding three new configuration options for tuning DHCPv4 retransmission timing.
Description check ✅ Passed The description thoroughly explains the rationale, the three new options (initial_interval, backoff_cutoff, backoff_jitter), defaults matching RFC 2131, validation behavior, DHCPv4-only scope, and includes test harness details and example output.
Linked Issues check ✅ Passed Issue #406 requests a backoff-cutoff equivalent in dhcpcd; the PR adds backoff_cutoff plus initial_interval and backoff_jitter options that directly address the user's need to configure DHCP retransmission timing parameters.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the stated objectives: core DHCP backoff parameter fields/parsing, documentation, and a comprehensive test harness; no unrelated modifications detected.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/dhcp.c`:
- Around line 1867-1880: The jitter subtraction currently happens in unsigned
space causing wrap; change the jitter computation to use a signed/wider integer:
compute a signed jitter_offset =
(int64_t)arc4random_uniform((uint32_t)ifo->backoff_jitter * 2) -
(int64_t)ifo->backoff_jitter, clamp jitter_offset to a reasonable range if
needed, then compute RT using signed/wider arithmetic as RT =
(int64_t)state->interval * MSEC_PER_SEC + jitter_offset and assign back ensuring
types match (use explicit casts) to avoid unsigned wrap; update the code around
variables jitter, state->interval, RT and the arc4random_uniform call.

In `@src/if-options.c`:
- Around line 2586-2603: The new options backoff_cutoff and initial_interval
must be validated together in finish_config(): after parsing, check if
ifo->backoff_cutoff is less than ifo->initial_interval and either clamp
backoff_cutoff to initial_interval or return an error; update finish_config() to
perform this comparison and adjust or reject the configuration (modify the
function that finalizes options, e.g., finish_config()) and provide a clear
logerrx or warning message indicating the clamp or failure including both
values.

In `@tests/backoff/test_backoff.c`:
- Around line 847-859: The test currently tears down host dhcpcd state by
calling run_cmd("pkill -9 dhcpcd ...") and manipulating /var/run/dhcpcd and
/var/db/dhcpcd; change this to operate only inside the per-test temporary
directory created via tmpdir/mkdtemp: remove the pkill and any run_cmd
invocations that create/delete /var/run/dhcpcd or /var/db/dhcpcd, and instead
create and use equivalent per-test directories under tmpdir (e.g., snprintf
paths under tmpdir and call run_cmd or mkdir -p on those) so cleanup is limited
to tmpdir and the host daemon is not affected; update any code that expects
global paths to point to the tmpdir equivalents so the timing/reject case stays
isolated.
- Around line 730-742: Help text for the -n option is incorrect: update the
fprintf format string in tests/backoff/test_backoff.c (the multi-line usage
string passed to fprintf) so the "-n N" line shows the actual default of
num_runs (128) instead of 1 (e.g., change "(default: 1, max: %d)" to "(default:
128, max: %d)"). Ensure the format still uses MAX_RUNS for the max placeholder
and that the rest of the usage string remains unchanged.
🪄 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: 1a842ef5-281a-42bd-a4e0-68df4153f8f5

📥 Commits

Reviewing files that changed from the base of the PR and between 0edb765 and d5f27da.

📒 Files selected for processing (7)
  • src/dhcp.c
  • src/dhcpcd.conf.5.in
  • src/if-options.c
  • src/if-options.h
  • tests/Makefile
  • tests/backoff/Makefile
  • tests/backoff/test_backoff.c

Comment on lines +1867 to +1880
unsigned int jitter = ifo->backoff_jitter;

if (state->interval == 0)
state->interval = 4;
state->interval = ifo->initial_interval;
else {
unsigned int cutoff = ifo->backoff_cutoff;

state->interval *= 2;
if (state->interval > 64)
state->interval = 64;
if (state->interval > cutoff)
state->interval = cutoff;
}

RT = (state->interval * MSEC_PER_SEC) +
(arc4random_uniform(MSEC_PER_SEC * 2) - MSEC_PER_SEC);
(arc4random_uniform(jitter * 2) - jitter);
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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cd src && sed -n '1860,1890p' dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 969


🏁 Script executed:

# Search for RT definition and type in dhcp.c
rg -n 'unsigned int.*RT\s*=' src/dhcp.c | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Check MSEC_PER_SEC definition
rg -n 'define MSEC_PER_SEC' --type c

Repository: NetworkConfiguration/dhcpcd

Length of output: 112


🏁 Script executed:

# Look for struct state definition to understand state->interval type
rg -n 'struct.*state\s*{' -A 20 src/dhcp.c | head -50

Repository: NetworkConfiguration/dhcpcd

Length of output: 168


🏁 Script executed:

# Check struct if_options to see types of backoff_* fields
rg -n 'struct if_options\s*{' -A 50 --type c | grep -E '(backoff|interval|jitter)' | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 176


🏁 Script executed:

# Search for RT declaration in send_message function
rg -n 'unsigned int.*RT' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 93


🏁 Script executed:

# Find the function containing this code - likely send_message
rg -n 'send_message' src/dhcp.c | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 415


🏁 Script executed:

# Read more context around line 1867 to see RT declaration
sed -n '1800,1880p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 2107


🏁 Script executed:

# Search for state->interval type - check struct definition
rg -n 'struct.*state' src/dhcp.c | grep -v '//' | head -10

Repository: NetworkConfiguration/dhcpcd

Length of output: 529


🏁 Script executed:

# Find dhcp_state struct definition
rg -n 'struct dhcp_state' src/dhcp.c | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 282


🏁 Script executed:

# Look for struct dhcp_state { in header files
fd -e h -e c src | xargs rg 'struct dhcp_state\s*\{' -A 30 | head -80

Repository: NetworkConfiguration/dhcpcd

Length of output: 991


🏁 Script executed:

# Find if_options struct - likely in a header file
fd -e h src | xargs rg 'struct if_options' -A 50 | head -100

Repository: NetworkConfiguration/dhcpcd

Length of output: 3882


🏁 Script executed:

# Search for struct if_options definition more directly
fd -e h src | xargs grep -l 'if_options'

Repository: NetworkConfiguration/dhcpcd

Length of output: 55


🏁 Script executed:

# Look at if.h which likely has if_options
cat src/if.h | head -200

Repository: NetworkConfiguration/dhcpcd

Length of output: 6139


🏁 Script executed:

# Search for if_options struct definition across all header files
fd -e h src | xargs grep -n 'struct if_options' 2>/dev/null | head -5

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Look at dhcpcd.h or similar for if_options
rg -n 'struct if_options' -A 100 src/ | head -150

Repository: NetworkConfiguration/dhcpcd

Length of output: 5685


🏁 Script executed:

# Search for if_options definition in dhcpcd.h or similar main headers
rg -n 'struct if_options\s*\{' -A 150 src/dhcpcd.h 2>/dev/null | head -200

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Try looking in if-options.h
fd if-options.h src

Repository: NetworkConfiguration/dhcpcd

Length of output: 87


🏁 Script executed:

# Search more broadly for struct if_options with open brace
rg 'struct if_options\s*\{' -l src/

Repository: NetworkConfiguration/dhcpcd

Length of output: 87


🏁 Script executed:

cat src/if-options.h | head -300

Repository: NetworkConfiguration/dhcpcd

Length of output: 9442


🏁 Script executed:

# Get the types in struct dhcp_state
sed -n '1,50p' src/dhcp.h | grep -A 20 'struct dhcp_state'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Verify how RT is used after calculation to understand the impact
sed -n '1880,1920p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 1520


🏁 Script executed:

# Check where RT is used after the calculation - look for the timeout scheduling
sed -n '1920,1960p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 1186


🏁 Script executed:

# Look for eloop_timeout_add or similar functions that might use RT
rg -n 'eloop_timeout_add|eloop_add_timeout' src/dhcp.c | head -10

Repository: NetworkConfiguration/dhcpcd

Length of output: 582


🏁 Script executed:

# Search for how RT is used in the send_message function and after
rg -n 'RT' src/dhcp.c | grep -A 2 -B 2 '188[0-9]:'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Check line 1974 - how RT is passed to eloop_timeout_add_msec
sed -n '1970,1980p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 337


🏁 Script executed:

# Check the function signature for eloop_timeout_add_msec to understand what it expects
rg -n 'eloop_timeout_add_msec' -A 3 src/eloop.c src/eloop.h | head -20

Repository: NetworkConfiguration/dhcpcd

Length of output: 356


🏁 Script executed:

# Now let's trace through the arithmetic more carefully
# Check if there are any bounds on backoff_jitter or initial_interval
rg -n 'backoff_jitter\|initial_interval\|backoff_cutoff' src/if-options.c | head -30

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Find eloop_q_timeout_add_msec definition
rg -n 'eloop_q_timeout_add_msec' src/eloop.h -A 5

Repository: NetworkConfiguration/dhcpcd

Length of output: 736


🏁 Script executed:

# Check the actual implementation to see what type is expected
rg -n 'eloop_q_timeout_add_msec' src/eloop.c -A 10 | head -30

Repository: NetworkConfiguration/dhcpcd

Length of output: 363


🏁 Script executed:

# Look at how backoff_jitter is parsed/set - are there bounds?
sed -n '1,100p' src/if-options.c | grep -i 'backoff'

Repository: NetworkConfiguration/dhcpcd

Length of output: 53


🏁 Script executed:

# Search for where backoff_jitter gets assigned to understand its bounds
rg -n 'backoff_jitter\s*=' src/if-options.c -B 3 -A 3 | head -50

Repository: NetworkConfiguration/dhcpcd

Length of output: 581


🏁 Script executed:

# Check if there's any validation of RT value before passing to eloop_timeout_add_msec
sed -n '1868,1975p' src/dhcp.c

Repository: NetworkConfiguration/dhcpcd

Length of output: 3172


🏁 Script executed:

# Verify the default config makes this vulnerable
rg -n 'DEFAULT_INITIAL_INTERVAL|DEFAULT_BACKOFF_JITTER' src/if-options.h

Repository: NetworkConfiguration/dhcpcd

Length of output: 199


Fix unsigned integer arithmetic wrapping in the DHCP retransmission timeout calculation.

The expression arc4random_uniform(jitter * 2) - jitter wraps when the random value is less than jitter, since both operands are unsigned int. With default config (initial_interval 4 and backoff_jitter 1000), a random value under 500 causes RT to wrap to near UINT_MAX, scheduling an incorrect timeout. The scenario with initial_interval 1 and backoff_jitter 2000 reliably triggers wrap on any random value below 2000.

Perform the jitter calculation in signed or wider integer space and clamp the result before assigning to RT.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/dhcp.c` around lines 1867 - 1880, The jitter subtraction currently
happens in unsigned space causing wrap; change the jitter computation to use a
signed/wider integer: compute a signed jitter_offset =
(int64_t)arc4random_uniform((uint32_t)ifo->backoff_jitter * 2) -
(int64_t)ifo->backoff_jitter, clamp jitter_offset to a reasonable range if
needed, then compute RT using signed/wider arithmetic as RT =
(int64_t)state->interval * MSEC_PER_SEC + jitter_offset and assign back ensuring
types match (use explicit casts) to avoid unsigned wrap; update the code around
variables jitter, state->interval, RT and the arc4random_uniform call.

Comment on lines +2586 to +2603
case O_INITIAL_INTERVAL:
ARG_REQUIRED;
ifo->initial_interval =
(uint32_t)strtou(arg, NULL, 0, 1, UINT32_MAX, &e);
if (e) {
logerrx("invalid initial interval: %s", arg);
return -1;
}
break;
case O_BACKOFF_CUTOFF:
ARG_REQUIRED;
ifo->backoff_cutoff =
(uint32_t)strtou(arg, NULL, 0, 1, UINT32_MAX, &e);
if (e) {
logerrx("invalid backoff cutoff: %s", arg);
return -1;
}
break;
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 | 🟡 Minor

Validate the new knobs together, not just individually.

Right now backoff_cutoff can be set below initial_interval. That makes the sequence shrink (4s -> 1s -> 1s) instead of cap, which contradicts the new manpage text and the expected meaning of a cutoff. Please clamp or reject that combination in finish_config() once both values are known.

🛠️ Possible clamp in finish_config()
 static void
 finish_config(struct if_options *ifo)
 {
+	if (ifo->backoff_cutoff < ifo->initial_interval) {
+		logerrx("backoff_cutoff must be >= initial_interval");
+		ifo->backoff_cutoff = ifo->initial_interval;
+	}
+
 	/* Terminate the encapsulated options */
 	if (ifo->vendor[0] && !(ifo->options & DHCPCD_VENDORRAW)) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/if-options.c` around lines 2586 - 2603, The new options backoff_cutoff
and initial_interval must be validated together in finish_config(): after
parsing, check if ifo->backoff_cutoff is less than ifo->initial_interval and
either clamp backoff_cutoff to initial_interval or return an error; update
finish_config() to perform this comparison and adjust or reject the
configuration (modify the function that finalizes options, e.g.,
finish_config()) and provide a clear logerrx or warning message indicating the
clamp or failure including both values.

Comment on lines +730 to +742
fprintf(stderr,
"Usage: %s [-b path-to-dhcpcd] [-n num-runs] [-s name] [-v]\n"
"\n"
"Options:\n"
" -b path Path to dhcpcd binary "
"(default: ../../src/dhcpcd)\n"
" -n N Number of parallel runs per test "
"(default: 1, max: %d)\n"
" -s name Run only scenario name "
"(defaults, min-latency, cloud, reject)\n"
" -v Print all observed values per retry\n"
" -h Show this help\n",
prog, MAX_RUNS);
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 | 🟡 Minor

Fix the documented default for -n.

The help text says -n defaults to 1, but num_runs is initialized to 128. That makes the harness much heavier than the CLI advertises.

📝 Possible fix
-	    "  -n N      Number of parallel runs per test "
-	    "(default: 1, max: %d)\n"
+	    "  -n N      Number of parallel runs per test "
+	    "(default: 128, max: %d)\n"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fprintf(stderr,
"Usage: %s [-b path-to-dhcpcd] [-n num-runs] [-s name] [-v]\n"
"\n"
"Options:\n"
" -b path Path to dhcpcd binary "
"(default: ../../src/dhcpcd)\n"
" -n N Number of parallel runs per test "
"(default: 1, max: %d)\n"
" -s name Run only scenario name "
"(defaults, min-latency, cloud, reject)\n"
" -v Print all observed values per retry\n"
" -h Show this help\n",
prog, MAX_RUNS);
fprintf(stderr,
"Usage: %s [-b path-to-dhcpcd] [-n num-runs] [-s name] [-v]\n"
"\n"
"Options:\n"
" -b path Path to dhcpcd binary "
"(default: ../../src/dhcpcd)\n"
" -n N Number of parallel runs per test "
"(default: 128, max: %d)\n"
" -s name Run only scenario name "
"(defaults, min-latency, cloud, reject)\n"
" -v Print all observed values per retry\n"
" -h Show this help\n",
prog, MAX_RUNS);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/backoff/test_backoff.c` around lines 730 - 742, Help text for the -n
option is incorrect: update the fprintf format string in
tests/backoff/test_backoff.c (the multi-line usage string passed to fprintf) so
the "-n N" line shows the actual default of num_runs (128) instead of 1 (e.g.,
change "(default: 1, max: %d)" to "(default: 128, max: %d)"). Ensure the format
still uses MAX_RUNS for the max placeholder and that the rest of the usage
string remains unchanged.

Comment on lines +847 to +859
/* Create temp directory for all test artifacts */
snprintf(tmpdir, sizeof(tmpdir), "/tmp/testbackoff.XXXXXX");
if (mkdtemp(tmpdir) == NULL) {
fprintf(stderr, "mkdtemp: %s\n", strerror(errno));
return 1;
}

/* Clean stale state */
run_cmd("pkill -9 dhcpcd 2>/dev/null; true");
run_cmd("mkdir -p /var/db/dhcpcd 2>/dev/null");
run_cmd("mkdir -p /var/run/dhcpcd 2>/dev/null");
run_cmd("find /var/run/dhcpcd/ -type f -delete 2>/dev/null; true");
run_cmd("find /var/run/dhcpcd/ -type s -delete 2>/dev/null; true");
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

Don't tear down the host's real dhcpcd state from this harness.

pkill -9 dhcpcd plus deleting /var/run/dhcpcd and /var/db/dhcpcd can kill the system daemon and disrupt networking on the machine running the test. The timing runs already use private namespaces and state; the reject case should too, so cleanup can stay under tmpdir.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/backoff/test_backoff.c` around lines 847 - 859, The test currently
tears down host dhcpcd state by calling run_cmd("pkill -9 dhcpcd ...") and
manipulating /var/run/dhcpcd and /var/db/dhcpcd; change this to operate only
inside the per-test temporary directory created via tmpdir/mkdtemp: remove the
pkill and any run_cmd invocations that create/delete /var/run/dhcpcd or
/var/db/dhcpcd, and instead create and use equivalent per-test directories under
tmpdir (e.g., snprintf paths under tmpdir and call run_cmd or mkdir -p on those)
so cleanup is limited to tmpdir and the host daemon is not affected; update any
code that expects global paths to point to the tmpdir equivalents so the
timing/reject case stays isolated.

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.

backoff-cutoff time; equivalant in dhcpcd.conf

1 participant