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

Fri Aug 11 15:33:15 UTC 2023

On 8/11/2023 3:06 PM, Kasper Brandt wrote:
  > 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

I have updated the wikipage (and bumped the version number to 0.9.6)

PS. It seems that fastmail's web interface mangles the plaintext version 
of the email with backticks whenever there is a change in format. Here 
is what the last part was supposed to say:

--------------------------------------------------------------------------------
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