Concatenate multiple sub-options with the same code sent by the server

[spoljak-ent]

Apologies, a bit of a branch cleanup :)

Original pull request https://github.com/NetworkConfiguration/dhcpcd/pull/378 Same patch in issue #74

[perkelix]

Could this be rebased with master?

[spoljak-ent]

Could this be rebased with master?

I think this should be a bit more optimized as @evverx mentioned

[rsmarples]

After another review, I don't think we need this as we already concatenate the options via the get_option() function? https://github.com/NetworkConfiguration/dhcpcd/blob/master/src/dhcp.c#L240

Everyone agree or anyone disagree? @spoljak-ent @evverx

[evverx]

@rsmarples I looked at this patch because there was a request downstream where I am to backport it and I just fuzz stuff before looking at patches.

As far as I understand get_option concatenates options according to RFC 3396 and it works. This patch is supposed to also concatenate suboptions as well but I have to admit I don't remember the details.

[spoljak-ent]

@rsmarples I looked at this patch because there was a request downstream where I am to backport it and I just fuzz stuff before looking at patches.

As far as I understand get_option concatenates options according to RFC 3396 and it works. This patch is supposed to also concatenate suboptions as well but I have to admit I don't remember the details.

I don't think this one is related to RFC 3396

https://github.com/NetworkConfiguration/dhcpcd/issues/74

Basically, as it's outlined in the issue linked above, if we have 2 or more sub options in say, DHCPv4 Option 125 with the same sub-option number, we will concatenate them rather than use only the last one.

For example: Option 125: Sub-option 1 string1 Option 125: Sub-option 1 string2

Now, with this patch we'd have an option 125 environment variable string1string2 rather than just string2. This should also work for options 17, 43 and 125.

Last time I tested, it didn't work without this patch.

[evverx]

I don't think this one is related to RFC 3396

I don't think so either. I replied to the comment where https://github.com/NetworkConfiguration/dhcpcd/blob/master/src/dhcp.c#L240 was mentioned and it does what RFC 3396 says. I don't think there are RFCs saying how suboptions should be concatenated (or whether they should be concatenated at all). Personally I think generally it would be great if patches came along with tests showing what they do and making sure they work.

[evverx]

For the record I pulled this patch and sent \x0b\x0e\x0e\x0f\x06\x01\x01\x01\x01\x01\x02

    Option: (125) V-I Vendor-specific Information
        Length: 11
        Enterprise: Unknown (185470479)
            Length: 6
            Option 125 Suboption: 1
                Length: 1
                Data: 01
            Option 125 Suboption: 1
                Length: 1
                Data: 02

I haven't been able to get 0102 however hard I tried.

I also tried to specify that the length should be 2 and it led to

==2779==ERROR: AddressSanitizer: requested allocation size 0xffffffffffffffff (0x800 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
    #0 0x557dc45a5610 in realloc (/dhcpcd/src/dhcpcd+0x1c3610) (BuildId: d0a7682fe8c9ae6e53a35f0320853624f09f84a4)
    #1 0x557dc464b7c7 in dhcp_envoption /dhcpcd/src/dhcp-common.c:988:13
    #2 0x557dc46756c2 in dhcp_env /dhcpcd/src/dhcp.c:1504:3
    #3 0x557dc46532b5 in make_env /dhcpcd/src/script.c:480:7
    #4 0x557dc4651368 in script_runreason /dhcpcd/src/script.c:759:16
    #5 0x557dc467b726 in dhcp_bind /dhcpcd/src/dhcp.c:2447:3
    #6 0x557dc46a43fd in dhcp_handledhcp /dhcpcd/src/dhcp.c:3503:2
    #7 0x557dc468724e in dhcp_packet /dhcpcd/src/dhcp.c:3710:2
    #8 0x557dc46a8b59 in dhcp_readbpf /dhcpcd/src/dhcp.c:3735:3
    #9 0x557dc4611b2e in eloop_run_ppoll /dhcpcd/src/eloop.c:1106:5
    #10 0x557dc4611b2e in eloop_start /dhcpcd/src/eloop.c:1228:11
    #11 0x557dc46008d8 in main /dhcpcd/src/dhcpcd.c:2650:6
    #12 0x7f8a3c352ca7  (/lib/x86_64-linux-gnu/libc.so.6+0x29ca7) (BuildId: def5460e3cee00bfee25b429c97bcc4853e5b3a8)

With define 125 binhex yolo_125 in dhcpd.conf I got

new_yolo_125=0b0e0e0f06010101010102

All in all it doesn't seem this patch does what it's supposed to do (and introduces memory corruptions triggered by incoming packets) so to me it seems it can be safely closed and whatever https://github.com/NetworkConfiguration/dhcpcd/issues/74 is about can be discussed there probably.