insomniacslk / dhcp

DHCPv6 and DHCPv4 packet library, client and server written in Go
BSD 3-Clause "New" or "Revised" License
685 stars 168 forks source link

fix_scope_shadowing_err_in_netboot_ConfigureInterface #538

Open youngifif opened 1 month ago

youngifif commented 1 month ago

Here is my long story:

I find that netlink doesn't allow to add the same address to one dev multiple time. TestConfigureInterface should fail to add 10.20.30.40 to eth0 in second time/second test case ,but it dosen't.

So,I think there is a bug in the func TestConfigureInterface located in netboot/netconf_integ_test.go And the reason TestConfigureInterface dosen't fail is that anonther bug located in func ConfigureInterface

To prove that, i use iproute2 tools in debian12 to illustrate the problem as follow: ip address flush dev enp0s8 To clean all address attached to dev enp0s8 ip address add 10.20.30.40 dev enp0s8 First time,to add 10.20.30.40 succesfully ip address add 10.20.30.40 dev enp0s8 Second time, an error return and terminal display RTNETLINK answers: File exists from iproute2.

Same reuslt in my test code which use rt.AddrAdd(iface,&addr) twice.


package iproute2

import (
    "net"
    "testing"

    "github.com/jsimonetti/rtnetlink/rtnl"
    "github.com/stretchr/testify/require"
)

var (

    ifname string = "enp0s8"//change ifname to  dev name in your machine
)

//before test,run  `sudo ip addr flush dev enp0s8`  to clean all addr on dev
func TestNetLinkAddrAdd(t *testing.T) {

    iface, err := net.InterfaceByName(ifname)
    require.NoError(t, err)

    rt, err := rtnl.Dial(nil)
    require.NoError(t, err)

    addr1 :=  net.IPNet{IP: net.ParseIP("10.20.30.40")}
    err1 := rt.AddrAdd(iface,&addr1)
    require.NoError(t, err1)//First Time,we succeed to add the addr to dev.

    addr2 :=  net.IPNet{IP: net.ParseIP("10.20.30.40")}
    err2 := rt.AddrAdd(iface,&addr2)
    require.Error(t, err2)//Second Time,we failed to add the addr to dev.
    // Error:       Received unexpected error:
    // netlink receive: file exists

}

After a further reaserch, i believe that the func netboot.ConfigureInterface has a bug, too. to be briefly,ConfigureInterface always return nil.

func ConfigureInterface(ifname string, netconf *NetConf) (err error) { 
    iface, err := net.InterfaceByName(ifname)
    if err != nil {
        return err
    }
    rt, err := rtnl.Dial(nil)
    if err != nil {
        return err
    }
    defer func() {
        if cerr := rt.Close(); err != nil {  //line1 
            err = cerr
        }
    }()
    for _, addr := range netconf.Addresses {
        if err := rt.AddrAdd(iface, &addr.IPNet); err != nil {  
            return fmt.Errorf("cannot configure %s on %s: %v", ifname, addr.IPNet, err) //line2

        }
    }
  //...
}

When line2 return err which is not nil ,the defer closure check the err != nil in line1, then reassign the err to cerr. But cerr := rt.Close() is nil in most case , which means that ConfigureInterface nearly always return nil even if line2 return err != nil inside the function . I think we should only reassign reutrn err to cerr,when return err is nil.

pmazzini commented 1 month ago

Integration tests are failing.