Fix crash in pillar if NTP FQDN is an empty string

[rucoder]

Under some conditions e.g. when set from Monitor TUI NTP FQDN can be an empty string which causes a crash

pillar;panic: runtime error: index out of range -1]

pillar;goroutine 170248 [running]:
pillar;github.com/lf-edge/eve/pkg/pillar/devicenetwork.ResolveWithSrcIP({0x0, 0x0}, {0xc002631950, 0x10, 0x10}, {0xc002631960, 0x10, 0x10}) pillar; /pillar/devicenetwork/dns.go:79 +0x4d4
pillar;github.com/lf-edge/eve/pkg/pillar/devicenetwork.ResolveCacheWrap.func1({0x0, 0x0}, {0xc002631950, 0x10, 0x10}, {0xc002631960, 0x10, 0x10}) pillar; /pillar/devicenetwork/dns.go:122 +0x1ca
pillar;github.com/lf-edge/eve/pkg/pillar/devicenetwork.ResolveWithPortsLambda.func1({0xc002631950, 0x10, 0x10}, {0xc002631960, 0x10, 0x10}) pillar; /pillar/devicenetwork/dns.go:198 +0x17b
pillar;created by github.com/lf-edge/eve/pkg/pillar/devicenetwork.ResolveWithPortsLambda pillar; /pillar/devicenetwork/dns.go:182 +0x98a

[OhmSpectator]

After discussing offline, @michel-zededa decided to add filtering out empty servers in the sanitize function:

eve/pkg/pillar/types/dpc.go

Line 333 in f2651ce

func (config *DevicePortConfig) DoSanitize(log *base.LogObject, args DPCSanitizeArgs) {

[rene]

After discussing offline, @michel-zededa decided to add filtering out empty servers in the sanitize function:

eve/pkg/pillar/types/dpc.go

Line 333 in f2651ce

func (config *DevicePortConfig) DoSanitize(log *base.LogObject, args DPCSanitizeArgs) {

@OhmSpectator , I think you mean @rucoder 😉

[christoph-zededa]

After discussing offline, @michel-zededa decided to add filtering out empty servers in the sanitize function:

eve/pkg/pillar/types/dpc.go

Line 333 in f2651ce

func (config *DevicePortConfig) DoSanitize(log *base.LogObject, args DPCSanitizeArgs) {

Why so complicated? The next one who is using ResolveWithSrcIP will again forget to use DoSanitize.
I thought something like this would be enough:

diff --git a/pkg/pillar/devicenetwork/dns.go b/pkg/pillar/devicenetwork/dns.go
index 698e90c15..61ab5eb88 100644
--- a/pkg/pillar/devicenetwork/dns.go
+++ b/pkg/pillar/devicenetwork/dns.go
@@ -76,7 +76,7 @@ func ResolveWithSrcIP(domain string, dnsServerIP net.IP, srcIP net.IP) ([]DNSRes
        dialer := net.Dialer{LocalAddr: &sourceUDPAddr}
        dnsClient := dns.Client{Dialer: &dialer}
        msg := dns.Msg{}
-       if domain[len(domain)-1] != '.' {
+       if !strings.HasSuffix(domain, ".") {
                domain = domain + "."
        }
        msg.SetQuestion(domain, dns.TypeA)
diff --git a/pkg/pillar/devicenetwork/dns_test.go b/pkg/pillar/devicenetwork/dns_test.go
index 715f4cd2f..72b08ed0d 100644
--- a/pkg/pillar/devicenetwork/dns_test.go
+++ b/pkg/pillar/devicenetwork/dns_test.go
@@ -285,3 +285,15 @@ func TestResolveCacheWrap(t *testing.T) {
                t.Fatalf("resolver func should have been called twice because different src IPs, but called=%d", called)
        }
 }
+
+func FuzzResolveWithSrcIP(f *testing.F) {
+       f.Fuzz(func(t *testing.T,
+               domain string,
+               dnsServer string,
+               src string,
+       ) {
+               dnsServerIP := net.ParseIP(dnsServer)
+               srcIP := net.ParseIP(src)
+               devicenetwork.ResolveWithSrcIP(domain, dnsServerIP, srcIP)
+       })
+}

[OhmSpectator]

Why so complicated?

@christoph-zededa, the point was to filter out these broken entries as soon as possible and never face them again. My understanding was that this function (DoSanitize) is called to handle all the DPC messages that come into the system. But @rucoder says I was wrong.

[eriknordmark]

Since you use TrimSpace about, don't you want to use the trimmed string here as well?

[christoph-zededa]

Why so complicated?

@christoph-zededa, the point was to filter out these broken entries as soon as possible and never face them again. My understanding was that this function (DoSanitize) is called to handle all the DPC messages that come into the system. But @rucoder says I was wrong.

I see.
Still I think ResolveWithSrcIP should not crash (defensive programming), therefore I guess we need both.

[OhmSpectator]

@rucoder are we abandoning the PR?

[rucoder]

@OhmSpectator no 😀

[OhmSpectator]

@OhmSpectator no 😀

okaaay