[Samba] Bug in dhcp-dyndns.sh script, A_REC always singleton array

[Kasper Brandt]

On Fri, Aug 11, 2023, at 2:58 PM, Rowland Penny via samba wrote:
> On Fri, 11 Aug 2023 14:03:01 +0200
> Kasper Brandt via samba <samba at lists.samba.org> wrote:
> 
> > Hello
> > I was directed to discuss this issue here. As I understand the issue
> > with using the unquoted variable is that it expand globs unless
> > noglob is set. E.g.
> > 
> > root at dy3:/# test="b*"
> > root at dy3:/# a=($test)
> > root at dy3:/# echo ${a[0]}
> > bin
> > 
> > It does seem a bit hypothetical that the output of sambatool dns
> > query ... for an A record should contain a glob, but for the sake of
> > robustness it might still be a good idea to find a better solution.
> > 
> > For a single match the output looks like:
> > root at dy3:/# /bin/samba-tool dns query "dy3" "poizan.dk" "PoiKodi" A
> > "--use-kerberos=required" Name=, Records=1, Children=0
> >     A: 10.13.37.10 (flags=f0, serial=1392054, ttl=900)
> > 
> > For multiple A records the output looks like:
> > root at dy3:/# /bin/samba-tool dns query "dy3" "poizan.dk" "test" A
> > "--use-kerberos=required" Name=, Records=2, Children=0
> >     A: 10.13.37.100 (flags=f0, serial=1392062, ttl=3600)
> >     A: 10.13.37.101 (flags=f0, serial=1392063, ttl=3600)
> > 
> > With the "| grep 'A:' | awk '{print $2}' " filtering these becomes
> > respectively
> > 
> > 10.13.37.10
> > 
> > and
> > 
> > 10.13.37.100
> > 10.13.37.101
> > 
> > So just splitting by lines should work.
> > 
> > Unfortunately just using mapfile with A_REC variable does not quite
> > work as here strings always gets a newline at the end.
> > 
> > root at dy3:/# mapfile -t foo <<< ""
> > root at dy3:/# echo ${#foo[@]}
> > 1
> > 
> > I would then suggest instead redirecting the query command right away
> > with process substitution
> > 
> > `mapfile -t `A_REC `< <`($SAMBATOOL dns query "${Server}" "${domain}"
> > "${name}" A "$KTYPE" 2>/dev/null | grep 'A:' | awk '{print $2}') `
> > `This seems to make it work as intended:
> > 
> > root at dy3:/# mapfile -t A_REC < <(/bin/samba-tool dns query "dy3"
> > "poizan.dk" "doesnotexists" A "--use-kerberos=required" 2>/dev/null |
> > grep 'A:' | awk '{print $2}') root at dy3:/# echo ${#A_REC[@]} 0
> > root at dy3:/# mapfile -t A_REC < <(/bin/samba-tool dns query "dy3"
> > "poizan.dk" "PoiKodi" A "--use-kerberos=required" 2>/dev/null | grep
> > 'A:' | awk '{print $2}') root at dy3:/# echo ${#A_REC[@]} 1
> > root at dy3:/# echo ${A_REC[0]}
> > 10.13.37.10
> > root at dy3:/# mapfile -t A_REC < <(/bin/samba-tool dns query "dy3"
> > "poizan.dk" "test" A "--use-kerberos=required" 2>/dev/null | grep
> > 'A:' | awk '{print $2}') root at dy3:/# echo ${#A_REC[@]} 2
> > root at dy3:/# echo ${A_REC[0]}
> > 10.13.37.100
> > root at dy3:/# echo ${A_REC[1]}
> > 10.13.37.101
> > 
> > - Kasper Brandt
> > 
> > On Fri, Aug 11, 2023, at 1:14 PM, Rowland Penny wrote:
> > > On Fri, 11 Aug 2023 02:52:59 +0200
> > > Kasper Brandt via samba-team <samba-team at lists.samba.org> wrote:
> > > 
> > > > Hello
> > > > I encountered a bug in the dhcp-dyndns.sh script embedded in the
> > > > site at
> > > > https://wiki.samba.org/index.php/Configure_DHCP_to_update_DNS_records
> > > > 
> > > > Version 0.9.5 (introduced in this revision:
> > > > https://wiki.samba.org/index.php?title=Configure_DHCP_to_update_DNS_records&oldid=18255)
> > > > introduced the following
> > > > 
> > > > > # turn A_REC into an array
> > > > > A_REC=("$A_REC")
> > > > 
> > > > (a bash array wasn't used in the previous version)
> > > > This is however incorrect as this will always create an array of
> > > > one element due to the quotes. The branch immediately after that
> > > > should only be taken if the A record didn't exist is never taken.
> > > > Instead the branch that should handle 1 existing record gets
> > > > taken and something like
> > > > > /bin/samba-tool dns delete "dy3" "poizan.dk" "PoiKodi" A ""
> > > > > "--use-kerberos=required"
> > > > gets executed (note the empty data field), which fails with an
> > > > invalid parameter error.
> > > > 
> > > > The line should probably just have been
> > > > > A_REC=($A_REC)
> > > > which splits the string into array elements by whitespace.
> > > > 
> > > > Someone more familiar with the script might want to verify my
> > > > fix, or I can just make the change myself if I get wiki access if
> > > > that is acceptable? At least I am currently using the fix without
> > > > issue.
> > > > 
> > > > - Kasper Brandt
> > > 
> > > Problem is, whilst it may be a bug, the suggested fix doesn't
> > > appear to be the fix. If you do as Kasper suggests and remove the
> > > quotation marks and then run shellcheck against the script, you get
> > > this:
> > > 
> > > In dhcp-dyndns.sh.0.9.5 line 233:
> > > A_REC=($A_REC)
> > >                        ^-- SC2206: Quote to prevent word splitting,
> > > or split robustly with mapfile or read -a.
> > > 
> > > Which is why I probably took the easy option and added the quotation
> > > marks, looks like using a mapfile or read needs to be investigated.
> > > 
> > > Can I also point out that the last lines on that wikipage are these:
> > > 
> > > Any questions or problems, ask on the Samba mailing list.
> > > 
> > > Do not log a bug report on Samba bugzilla for any problems you have
> > > with this set up, ask on the samba mailing list.
> > > 
> > > Rowland
> > > 
> > > 
> > > 
> 
> Hi Kaspar, thanks for looking into this, when I said 'looks like using
> a mapfile or read needs to be investigated.', I really meant that I
> would look into it, so thanks for saving me the trouble :-)
> 
> It looks like the mapfile is the way to go, but have you run shellcheck
> on the revised script ? 
> 
> If you have, please update the script on the wikipage again.
Hi Rowland,
shellcheck runs with no complaints. I will update the wikipage.

- Kasper