samba - Aug 2023 - Bug in dhcp-dyndns.sh script, A_REC always singleton array

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

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.

Rowand