Rowland Penny
2023-Aug-11 12:58 UTC
[Samba] Bug in dhcp-dyndns.sh script, A_REC always singleton array
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
Kasper Brandt
2023-Aug-11 13:06 UTC
[Samba] Bug in dhcp-dyndns.sh script, A_REC always singleton array
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