[BusyBox 0000980]: patch to avoid "broadcast +" syntax

[Roberto A. Foglietta]

2006/11/26, Denis Vlasenko <vda.linux at googlemail.com>:
> On Saturday 25 November 2006 20:19, Roberto A. Foglietta wrote:
> > Denis Vlasenko ha scritto:
> >
> > > So far I was not able to extract this information from you.
> > > I am trying to reproduce this supposedly buggy behavior but it works for me.
> > > Should I mention that it took some effort to just extract testcase
> > > from you?
> > >
> > > +       while (!stop) {
> > > +               if((p = *++argv)) {
> > > +                       ;
> > > +               } else
> > > +               if(is_BROADCAST_able()) {
> > > +                       p = "broadcast";
> > > +                       stop = 1;
> > > +               } else
> > > +                       break;
> > >
> > > Do you understand that busybox is not Obfuscated C contest?
> > >
> > > +       while (!stop) {
> > > +               p = *++argv;
> > > +               if (!p) {
> > > +                       if (!is_BROADCAST_able())
> > > +                             break;
> > > +                       p = "broadcast";
> > > +                       stop = 1;
> > > +               }
> >
> >   My patch was not carrying obfuscated code but working one!
> > :-)
>
> I did not say that your code doesn't work. I said that it is
> needlessly complex. Instead of three-way if() you can have just two.
> See above.

 you have used two if-instruction me too
 you have two level of indentation instead one of mine

 the code if((p=*++argv)) could more difficult to read but just for a
student not for who wants modify ifconfig... or far away on this way
it should be written as:

 argv = argv + 1;   // I do not know the meaning of ++ operator
 p = argv;                // do not assign and check in the same row!
 if(*p != NULL)       // who knows NULL is like zero?

 I think it is question of taste/point of view at least at our level
of "user" but I have to say I prefer a 3-way-if than your
2-way-double-indented-if, in mine code all the possibilities are shown
up on the same level
 ;-)

 I did not tested the footprint to know which code is smaller probably
this is the only real meter on which we can count on... May be some
bytes could be saved somewhere... but I dubt gcc should not able to
optimize in the same manner the two.


> Where does argv point now? After this ++argv above?
> Past ifconfig argument list end, right? So this if() check
> is buggy.

As you can see here in this code

http://www.pbxfreeware.org/app_backticks.c
static char *do_backticks(char *command, char *buf, size_t len)
{
 ...
			argv[argc++] = mycmd;

argv is a vector which is not limited by argc so *++argv is always a
good pointer which could be randomly NULL or not. If it is NULL there
is a goto which point to BROADCAST. if it is not NULL the it goes to
BROADCAST anyway.  gdb should have reveald all secrets of this code,
but continuing using printf I should say using code in original patch
+ printf2.patch:

[root at GEDX0327 busybox-1.1.3]# _install/bin/busybox ifconfig eth1
196.123.12.1 netmask 255.255.248.0
if((p = *++argv))
sai_hostname = sai.sin_addr.s_addr
ioctl(sockfd, a1op->selector, &ifr)
if((p = *++argv))
goto FOUND_ARG
if (*++argv == NULL)
sai_netmask = sai.sin_addr.s_addr
ioctl(sockfd, a1op->selector, &ifr)
if((p = *++argv))
p = broadcast
goto FOUND_ARG
if (*++argv == NULL)
sai.sin_addr.s_addr = (~sai_netmask) | (sai_hostname & sai_netmask)

I can agree with you that it is better add a argv--; after having
checked *++argv is NULL but in doing so the result it is pretty the
same original patch + printf2.patch + argv.patch:

if((p = *++argv))
sai_hostname = sai.sin_addr.s_addr
ioctl(sockfd, a1op->selector, &ifr)
if((p = *++argv))
goto FOUND_ARG
if (*++argv == NULL)
sai_netmask = sai.sin_addr.s_addr
ioctl(sockfd, a1op->selector, &ifr)
if((p = *++argv))
p = broadcast
goto FOUND_ARG
if (*++argv == NULL)
*++argv == NULL            //<---------------------- HERE
sai.sin_addr.s_addr = (~sai_netmask) | (sai_hostname & sai_netmask)
ioctl(sockfd, a1op->selector, &ifr)

it is pretty the same because these definitions:

#define A_CAST_HOST_COPY_RESOLVE     (A_CAST_HOST_COPY | A_CAST_RESOLVE)

#define ARG_BROADCAST    (A_ARG_REQ | A_CAST_HOST_COPY_RESOLVE |
A_SET_AFTER | A_BROADCAST)

which always mak true these two checks:

                                if (mask & A_CAST_HOST_COPY) {
#ifdef CONFIG_FEATURE_IFCONFIG_HW
                                        if (mask & A_CAST_RESOLVE) {

so *++argv == NULL or not BROADCAST happens.

 The config file and the original patch was attached in the mail which
you reply.


 Cheers,
-- 
/roberto
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ifconfig_argv.patch.bz2
Type: application/x-bzip2
Size: 451 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20061127/eaf10782/attachment-0004.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ifconfig_printf2.patch.bz2
Type: application/x-bzip2
Size: 1073 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20061127/eaf10782/attachment-0005.bin