Skip to content

Fix/dhcpv6 fixups#5759

Closed
ethan-thompson wants to merge 9 commits intoFreeRADIUS:masterfrom
ethan-thompson:fix/dhcpv6-fixups
Closed

Fix/dhcpv6 fixups#5759
ethan-thompson wants to merge 9 commits intoFreeRADIUS:masterfrom
ethan-thompson:fix/dhcpv6-fixups

Conversation

@ethan-thompson
Copy link
Contributor

Various DHCPv6 related fixes

…ind messages that include a Server Identifier option.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
…rmation-request messages IF the message includes a Server Identifier option AND the DUID in the option does not match the server's DUID, OR the message includes an IA option. So Server-ID is optional in this case and only if present should it be validated.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
… Instead, explicitly free vp before going to fail.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
…d to not reject them.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
…nteger.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>
@ethan-thompson ethan-thompson marked this pull request as ready for review March 4, 2026 20:51

static uint32_t instance_count = 0;
static bool instantiated = false;
static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
Copy link
Member

Choose a reason for hiding this comment

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

What was the justification for from claude for needing to make this thread safe? I don't think this is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, it didn't reference any call sites which actually needed this. Remove this commit and add a @note saying that init and deinit are only called by the main thred and do not need to be thread-safe. Ideally you'd add that to all init functions of the same type.

append = true;
}

slen = decode_vsa(vp, &vp->vp_group, da, data + 4, len, decode_ctx);
Copy link
Member

Choose a reason for hiding this comment

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

I think the correct thing here, is to do the same as we do in the raw label, but output the raw attribute within the VSA vp we already allocated, NOT free the vp. @alandekok @ndptech do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

there's a lot of similar issues elsewhere. I'll take a look.

@alandekok
Copy link
Member

I've grabbed some of the commit manually, and dropped others.

I've rewritten the "raw VSA" one, and hoisted the code out of the decode_vsa() function. It turns out to do it slightly better, we need to track both the VSA wrapper, and the vendor wrapper. I've also added a test.

It still has to do a better job of decoding unknown / invalid VSAs, but that can wait for a future day.

@alandekok alandekok closed this Mar 10, 2026
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.

3 participants