Skip to content

Options: Preserve v6 vendclass CLI optarg when parsing enterprise number - #725

Open
nbromberg-dn wants to merge 1 commit into
NetworkConfiguration:masterfrom
drivenets:nbromberg/fix-vendclass-cli-parsing
Open

nbromberg-dn wants to merge 1 commit into
NetworkConfiguration:masterfrom
drivenets:nbromberg/fix-vendclass-cli-parsing

Conversation

@nbromberg-dn

Copy link
Copy Markdown

The issue

When passing --vendclass '<EN> <DATA>' via CLI arguments, the packet itself is sent with empty data (only with the EN).

The vendclass parser split the enterprise number from its class data by writing a NUL into optarg.
The command-line options are applied more than once: first while processing global options, then again when configuring each interface.

That made the first pass succeed because it kept a local pointer to the bytes after the separator, but it permanently shortened the shared argv string to the EN for later passes.

Example

Calling

$ dhcpcd -6 --vendclass "1234 Hello" ...

Transmitted on the wire (dissected using tshark):

    Vendor Class
        Option: Vendor Class (16)
        Length: 6
        Enterprise ID: Linkage Software Inc. (1234)
        vendor-class-data:

Live-debugging with GDB confirms:

Breakpoint 2, parse_option (ctx=ctx@entry=0x7fffffffd978, ifname=ifname@entry=0x0, ifo=ifo@entry=0x56c940, opt=147, arg=0x56a6a0 "1234", ldop=ldop@entry=0x0, edop=0x0) at if-options.c:2198
2198			u = (uint32_t)strtou(arg, NULL, 0, 0, UINT32_MAX, &e);


(gdb) x/10bc arg
0x56a6a0:	49 '1'	50 '2'	51 '3'	52 '4'	0 '\000'	72 'H'	101 'e'	108 'l'
0x56a6a8:	108 'l'	111 'o'

(gdb) p arg
$1 = 0x56a6a0 "1234"

(gdb) p fp
$2 = 0x56a6a5 "Hello"

The proposed fix

Parse the enterprise number from a temporary NUL-terminated copy instead, leaving argv intact so the per-interface pass can still see and encode the vendor-class data.
fp still advances via strskipwhite into the vendor-class portion - behavior matches the old in-place split without mutating shared optarg.

With this fix, calling the same dhcpcd command as above now trasmits:

    Vendor Class
        Option: Vendor Class (16)
        Length: 11
        Enterprise ID: Linkage Software Inc. (1234)
        vendor-class-data: Hello

And GDB showing:

(gdb) x/10bc arg
0x5824e0:	49 '1'	50 '2'	51 '3'	52 '4'	32 ' '	72 'H'	101 'e'	108 'l'
0x5824e8:	108 'l'	111 'o'
# The 5th byte is kept as whitespace rather than changed to \0 as before

(gdb) p bp
$1 = 0x5977a0 "1234"
(gdb) p arg
$2 = 0x5824e0 "1234 Hello"

@coderabbitai

coderabbitai Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 72c468c7-6db8-40b1-8b5a-baf8e8cca1fd
📥 Commits

Reviewing files that changed from the base of the PR and between 261851a and eb70f4d.

📒 Files selected for processing: 1
  • src/if-options.c

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

The vendor class option parser copies the enterprise-number prefix before parsing it. It leaves the original argument unchanged for later per-interface option replay.

Changes

Vendor class parsing

Layer / File(s) Summary
Non-destructive enterprise-number parsing
src/if-options.c
The parser copies the enterprise-number prefix into a temporary buffer, handles allocation failure, parses the copied value, and frees the buffer. The original argument remains unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: colinmcinnes

Merge Risk: ⚪ Minimal · up to eb70f

No actionable merge risk was identified in the vendor-class argument preservation change.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: preserving the IPv6 vendclass command-line argument while parsing its enterprise number.
Description check Passed The description directly explains the vendclass parsing bug, its cause, the proposed fix, and the resulting behavior.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

Comment thread src/if-options.c Outdated
nbromberg-dn added a commit to drivenets/dhcpcd that referenced this pull request Sep 8, 2026
The vendclass parser split the enterprise number from its class data by writing a NUL into optarg.
dhcpcd applies command-line options more than once: first while processing global options, then again when each interface is configured.
That made the first pass succeed because it kept a local pointer to the bytes after the separator, but it permanently shortened the shared argv string to the enterprise number for later passes.

Parse the enterprise number from a temporary NUL-terminated copy instead, leaving argv intact so the per-interface pass can still see and encode the vendor-class data.

(cherry picked from commit 261851a from NetworkConfiguration#725)
Comment thread src/if-options.c Outdated
The vendclass parser split the enterprise number from its class data by writing a NUL into optarg.
dhcpcd applies command-line options more than once: first while processing global options, then again when each interface is configured.
That made the first pass succeed because it kept a local pointer to the bytes after the separator, but it permanently shortened the shared argv string to the enterprise number for later passes.

Parse the enterprise number from a temporary NUL-terminated copy instead, leaving argv intact so the per-interface pass can still see and encode the vendor-class data.
@nbromberg-dn
nbromberg-dn force-pushed the nbromberg/fix-vendclass-cli-parsing branch from 261851a to eb70f4d Compare October 23, 2026 07:28
@nbromberg-dn

Copy link
Copy Markdown
Author

Thanks @ColinMcInnes for the approval.
@rsmarples may you please take a look?

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