Quantcast

[PATCH] small test for Bug 12558

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] small test for Bug 12558

Samba - samba-technical mailing list
Hi!

Review appreciated!

If someone know the shell quoting magic to get this under 80 columns,
I'll be happy to change this :-)

Thanks, Volker

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
On Sun, 9 Apr 2017 10:37:03 +0200, vl--- via samba-technical
<[hidden email]> wrote:

> If someone know the shell quoting magic to get this under 80 columns,
> I'll be happy to change this :-)

Not in the way it is currently done.  The eval is there to expand "$@",
which just seems crazy.  In other places the only reason the command is
assigned to a variable is so it can be printed before running it.
You're not doing that in this case so you could just put the command in
the sub-shell.

However, if you want to print the command then doing something like
the following seems like a reasonable approximation:

    out=$(set -x; \
          $SMBCLIENT "$@" \
                -U$USERNAME%$PASSWORD //$SERVER/msdfs-share \
                -I $SERVER_IP $ADDARGS -m nt1 -c dir 2>&1)

You get sane quoting (in fact you can sensibly quote whatever you like,
such as "$SMBCLIENT") and the "set -x" causes the command to be printed
with leading "+ ".

If you don't want the leading "+ " then you can set PS4="" in the
sub-shell before the "set -x":

    out=$(PS4=""; set -x; \
          $SMBCLIENT "$@" \
                -U$USERNAME%$PASSWORD //$SERVER/msdfs-share \
                -I $SERVER_IP $ADDARGS -m nt1 -c dir 2>&1)

PS4 could be set globally for the whole script... but that might
confuse anyone who runs the script with an explicit "sh -x ..." for
debugging.  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
Hi!

Before we enter a flamewar about 80 cols vs proper shell coding style
I think we could also opt to just not use this patch and cross fingers
that next time this bug does not happen.

Volker

On Mon, Apr 10, 2017 at 09:59:14PM +1000, Martin Schwenke via samba-technical wrote:

> On Sun, 9 Apr 2017 10:37:03 +0200, vl--- via samba-technical
> <[hidden email]> wrote:
>
> > If someone know the shell quoting magic to get this under 80 columns,
> > I'll be happy to change this :-)
>
> Not in the way it is currently done.  The eval is there to expand "$@",
> which just seems crazy.  In other places the only reason the command is
> assigned to a variable is so it can be printed before running it.
> You're not doing that in this case so you could just put the command in
> the sub-shell.
>
> However, if you want to print the command then doing something like
> the following seems like a reasonable approximation:
>
>     out=$(set -x; \
>  $SMBCLIENT "$@" \
> -U$USERNAME%$PASSWORD //$SERVER/msdfs-share \
> -I $SERVER_IP $ADDARGS -m nt1 -c dir 2>&1)
>
> You get sane quoting (in fact you can sensibly quote whatever you like,
> such as "$SMBCLIENT") and the "set -x" causes the command to be printed
> with leading "+ ".
>
> If you don't want the leading "+ " then you can set PS4="" in the
> sub-shell before the "set -x":
>
>     out=$(PS4=""; set -x; \
>  $SMBCLIENT "$@" \
> -U$USERNAME%$PASSWORD //$SERVER/msdfs-share \
> -I $SERVER_IP $ADDARGS -m nt1 -c dir 2>&1)
>
> PS4 could be set globally for the whole script... but that might
> confuse anyone who runs the script with an explicit "sh -x ..." for
> debugging.  :-)
>
> peace & happiness,
> martin
>

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Samba eXPerience 2017, Hotel Freizeit In
2nd-4th of May 2017, http://sambaXP.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 10 Apr 2017 21:59:14 +1000
Martin Schwenke via samba-technical <[hidden email]>
wrote:

> On Sun, 9 Apr 2017 10:37:03 +0200, vl--- via samba-technical
> <[hidden email]> wrote:
>
> > If someone know the shell quoting magic to get this under 80
> > columns, I'll be happy to change this :-)
>
> Not in the way it is currently done.  The eval is there to expand
> "$@", which just seems crazy.  In other places the only reason the
> command is assigned to a variable is so it can be printed before
> running it. You're not doing that in this case so you could just put
> the command in the sub-shell.
>

In an attempt to further understand and expand my 'bash' knowledge, I
have been examining the script in question and I cannot understand why
'$@' is being used in the way it is ???

My understanding is that it is an array of all the arguments passed to
a script, so why is it being used in the way it is, especially as line
25 is 'shift 11' which effectively deletes the contents of '$@'

Am I missing something obvious here, or is my understanding wrong ?

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Apr 10, 2017 at 02:40:23PM +0200, vl--- via samba-technical wrote:
> Hi!
>
> Before we enter a flamewar about 80 cols vs proper shell coding style
> I think we could also opt to just not use this patch and cross fingers
> that next time this bug does not happen.

80+ cols is a C code restriction, not a shell code one.

RB+. Pushed - thanks !

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
On Mon, Apr 10, 2017 at 9:54 AM, Jeremy Allison via samba-technical
<[hidden email]> wrote:
> On Mon, Apr 10, 2017 at 02:40:23PM +0200, vl--- via samba-technical wrote:
>> Hi!
>>
>> Before we enter a flamewar about 80 cols vs proper shell coding style
>> I think we could also opt to just not use this patch and cross fingers
>> that next time this bug does not happen.
>
> 80+ cols is a C code restriction, not a shell code one.

All animals are equal but some are more equal than others ...

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] small test for Bug 12558

Samba - samba-technical mailing list
On Mon, Apr 10, 2017 at 10:52:28AM -0700, Richard Sharpe via samba-technical wrote:

> On Mon, Apr 10, 2017 at 9:54 AM, Jeremy Allison via samba-technical
> <[hidden email]> wrote:
> > On Mon, Apr 10, 2017 at 02:40:23PM +0200, vl--- via samba-technical wrote:
> >> Hi!
> >>
> >> Before we enter a flamewar about 80 cols vs proper shell coding style
> >> I think we could also opt to just not use this patch and cross fingers
> >> that next time this bug does not happen.
> >
> > 80+ cols is a C code restriction, not a shell code one.
>
> All animals are equal but some are more equal than others ...

Rubbish Richard. C code is deep Samba logic, one mistake
in which can lead to a root-level-remote code execution
security CVE.

Shell code is test code.

One I really care about simplicity and clarity, the
other - so long as it works and tests the C code ... :-).

Loading...