Bug 18289

Summary: bad shell code: broken by certain alterator's configurations
Product: Branch 4.1 Reporter: Ivan Zakharyaschev <imz>
Component: etcnetAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: qa-4.1 <qa-4.1>
Severity: minor    
Priority: P2 CC: inger, pilot
Version: unspecified   
Hardware: all   
OS: Linux   
Bug Depends on:    
Bug Blocks: 18290, 18291    
Attachments:
Description Flags
/var/log/daemons none

Description Ivan Zakharyaschev 2008-12-22 04:33:54 MSK
Created attachment 3151 [details]
/var/log/daemons

etcnet-0.9.7-alt0.M41.1

How to reproduce: I booted the 4.1 Desktop LiveCD, started the ALT Linux control center, and selected Zeroconf for the interface eth0, and entered a hostname: "vaio.home"; pressed "Применить" ("Apply"). Then error messages can be seen:

Dec 21 21:43:30 localhost ifplugd(eth0)[19436]: client: /etc/net/scripts/ifup-ifplugd: line 22: [: /etc/net/ifaces/eth0@vaio.home: binary operator expected

and so on (attached).

It's not OK.

First, it's bad unsafe shell code.

Second, it affects further configuration of the net: Before this, DHCP configuration (selected here in alterator) worked. After this, DHCP doesn't work anymore (even if I delete the hostname). So, it's an important error.
Comment 1 Ivan Zakharyaschev 2008-12-22 04:42:22 MSK
(In reply to comment #0)

> Second, it affects further configuration of the net: Before this, DHCP configuration (selected here in alterator) worked. After this, DHCP
> doesn't work anymore (even if I delete the hostname). So, it's an important error.
> 

https://bugzilla.altlinux.org/show_bug.cgi?id=18290
Comment 2 Denis Ovsienko 2008-12-22 20:44:12 MSK
The error messages are most likely caused by space(s) in the hostname. It's easy to deduct, that the NETHOST etcnet option expands to more, than one word, regardless if alterator sets it or the user. The data input by user should be checked more thoroughly.
Comment 3 Ivan Zakharyaschev 2008-12-22 23:48:04 MSK
(In reply to comment #2)
> The error messages are most likely caused by space(s) in the hostname. It's easy to deduct, that the NETHOST etcnet option expands to more,

Isn't it a bad shell code which can be broken by such expansion of spaces (regardless of the errors of alterator or the user)?

Couldn't it use proper quoting (and bash arrays if neeed)?

> than one word, regardless if alterator sets it or the user. The data input by user should be checked more thoroughly.
> 

Comment 4 Ivan Zakharyaschev 2008-12-23 00:05:17 MSK
(In reply to comment #3)
> (In reply to comment #2)
> > The error messages are most likely caused by space(s) in the hostname. It's easy to deduct, that the NETHOST etcnet option expands to more,
> 
> Isn't it a bad shell code which can be broken by such expansion of spaces (regardless of the errors of alterator or the user)?
> 
> Couldn't it use proper quoting (and bash arrays if neeed)?

Instead of 

	CAND=$BASE${NETPROFILE:+#$NETPROFILE}${NETHOST:+@$NETHOST}
	if [ -e $CAND ]; then

write:

	CAND="$BASE${NETPROFILE:+#$NETPROFILE}${NETHOST:+@$NETHOST}"
	if [ -e "$CAND" ]; then

and so on.

You don't need multiword expansion here, do you? Then the shell code is much safer, more predictable.

Comment 5 Ivan Zakharyaschev 2008-12-23 00:22:30 MSK
(In reply to comment #4)

I see, I could be missing the idea: 

> Instead of 
> 
>         CAND=$BASE${NETPROFILE:+#$NETPROFILE}${NETHOST:+@$NETHOST}
>         if [ -e $CAND ]; then

some kind of expansion was meant to be active there (wildcards: *). 

I'm sorry, perhaps, it's safe enough to have such expansions; only the error message is not clear.

IMHO, at least [ -e $CAND ] would be nicer if it is rewritten in a way  which gurarantees that the expansion gives one word.

"for f in $CAND; do  .... "$f" ....; break; done" ?

With checks for the correctness of the format of $CAND.

> 
> write:
> 
>         CAND="$BASE${NETPROFILE:+#$NETPROFILE}${NETHOST:+@$NETHOST}"
>         if [ -e "$CAND" ]; then
> 
> and so on.
> 
> You don't need multiword expansion here, do you? Then the shell code is much safer, more predictable.
> 

Comment 6 Ivan Zakharyaschev 2008-12-23 00:44:42 MSK
(In reply to comment #2)
> The error messages are most likely caused by space(s) in the hostname. It's easy to deduct, that the NETHOST etcnet option expands to more,
> than one word, regardless if alterator sets it or the user. The data input by user should be checked more thoroughly.

After this action in alterator:

# diff -dur /etc/net{.orig,}
Только в /etc/net/ifaces: eth0
# cat /etc/HOSTNAME
dell.home
dell.home
# 

The two entries in /etc/HOSTNAME are without any spaces, but with a new line. 

I don't know whether two lines are allowed or this is an error.
Comment 7 Ivan Zakharyaschev 2008-12-23 00:46:43 MSK
(In reply to comment #6)

> # cat /etc/HOSTNAME
> dell.home
> dell.home
> # 
> 
> The two entries in /etc/HOSTNAME are without any spaces, but with a new line. 

So, check early for the well-formedness of the read hostname.
Comment 8 Ivan Zakharyaschev 2008-12-23 01:02:06 MSK
Also, the same error message on line 21 of /sbin/ifdown:

(In reply to comment #0)

> Dec 21 21:43:30 localhost ifplugd(eth0)[19436]: client: /etc/net/scripts/ifup-ifplugd: line 22: [: /etc/net/ifaces/eth0@vaio.home: binary
> operator expected

Comment 9 Denis Ovsienko 2008-12-23 14:14:09 MSK
[...]
> # cat /etc/HOSTNAME
> dell.home
> dell.home
> # 
> 
> The two entries in /etc/HOSTNAME are without any spaces, but with a new line. 
> 
> I don't know whether two lines are allowed or this is an error.

Could you cite the output of "hostname" to put the dots above i's? Two-lined hostname is a misconfiguration in any way. /etc/net neither detects nor fixes such a situation. The only way it's involved is that he hostname is set by /etc/rc.d/rc.sysinit from the HOSTNAME variable stored in /etc/sysconfig/network, it's a good idea to check it as well.
Comment 10 Ivan Zakharyaschev 2008-12-23 17:34:15 MSK
So, accordingly, it's just a minor code imperfectness.

(In reply to comment #5)

> I'm sorry, perhaps, it's safe enough to have such expansions; only the error message is not clear.
> 
> IMHO, at least [ -e $CAND ] would be nicer if it is rewritten in a way  which gurarantees that the expansion gives one word.
> 
> "for f in $CAND; do  .... "$f" ....; break; done" ?
> 
> With checks for the correctness of the format of $CAND.
Comment 11 Ivan Zakharyaschev 2008-12-23 17:35:08 MSK
(In reply to comment #10)
> So, accordingly, it's just a minor code imperfectness.

The real error must be on a a higher level: https://bugzilla.altlinux.org/show_bug.cgi?id=18313 .
Comment 12 Ivan Zakharyaschev 2008-12-23 17:48:00 MSK
(In reply to comment #9)

> Could you cite the output of "hostname" to put the dots above i's?

[root@vaio ~]# hostname 
vaio.home
vaio.home
[root@vaio ~]# cat /etc/HOSTNAME 
vaio.home
vaio.home
[root@vaio ~]# cat /etc/sysconfig/network
# When set to no, this may cause most daemons' initscripts skip starting.
NETWORKING=yes

# Used by hotplug/pcmcia/ifplugd scripts to detect current network config
# subsystem.
CONFMETHOD=etcnet

# Used by rc.sysinit to setup system hostname at boot.
HOSTNAME=vaio.home

# This is used by ALTLinux ppp-common to decide if we want to install
# nameserver lines into /etc/resolv.conf or not.
RESOLV_MODS=yes
NETWORKING=yes
FORWARD_IPV4=false
HOSTNAME=vaio.home
[root@vaio ~]# 

> Two-lined hostname is a misconfiguration in any way. /etc/net neither
> detects nor fixes such a situation. The only way it's involved is that he 

Even if it doesn't detect this situation, and is not to blame for it, the shell code of etcnet shouldn't break: it should be guaranteed that an expansion to many words doesn't happen in a place where only one word is allowed by the "type" of the operator. It'll make the code more predictable and reliable.

> hostname is set by /etc/rc.d/rc.sysinit from the HOSTNAME
> variable stored in /etc/sysconfig/network, it's a good idea to check it as well.
> 

Comment 13 Denis Ovsienko 2008-12-23 20:37:00 MSK
I would suggest fixing coreutils first and alterator then. However, I will consider adding a hostname check into /etc/net.
Comment 14 Michael Shigorin 2014-11-05 20:41:39 MSK
В 4.1/branch исправления не будут вноситься уже технически (заглушена очередь на сборку), поэтому прошу ошибки, актуальные для sisyphus/p7/t7, перевесить на текущие ветки или сизиф.