dtrace discuss - Nov 2008 - Round three: Re: code review req: 6750659 drti.o crashes app due to corrupt environment

I''ve posted an updated webrev at:

http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/

Changes since the previous (11-15) webrev include the following
changes to the test case.

- Move the test case into the usdt directory
- Fix packaging for the test case in SUNWdtrt
- Remove leading tab for "all" target in generated Makefile

There have been no changes to the code in drti.c since the first code
review request was sent.

In the course of testing the test case, I ran across a problem with
tst.resize1.d that prevents me from running the entire test suite.  I
am, however, able to run the new test.  I believe that this is not
related to the change I made, as I am able to reproduce the problem on
a plain snv_101 install using tst.resize1.d from a snv_102 build.  I
have logged a separate bug on this but do not yet have the bug ID.

Mike

On Sun, Nov 16, 2008 at 7:43 AM, Mike Gerdts <mgerdts at gmail.com>
wrote:
> I''ve posted an updated code review that adds a test case. 
I''ve
> verified a full clobber build, that bfu delivers the new test case,
> and that the test case fails without the fix and succeeds with the
> fix.
>
> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-15/
>
> Mike
>
>
> On Wed, Nov 12, 2008 at 9:50 PM, Mike Gerdts <mgerdts at gmail.com>
wrote:
>> I believe that I have a fix for 6750659 ready to go.  Can I get a few
eyes on:
>>
>> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-12/
>>
>> There are only 5 lines of meaningful change.  Some background info is
>> available at
>>
>>
http://mail.opensolaris.org/pipermail/dtrace-discuss/2008-September/006637.html
>>
>> Jon Haslam has offered to sponsor this change.
>>
>> --
>> Mike Gerdts
>> http://mgerdts.blogspot.com/
>>
>
>
>
> --
> Mike Gerdts
> http://mgerdts.blogspot.com/
>


-- 
Mike Gerdts
http://mgerdts.blogspot.com/

Mike Gerdts wrote:
> 
> I''ve posted an updated webrev at:
> 
> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/
> 
> Changes since the previous (11-15) webrev include the following
> changes to the test case.
> 
> - Move the test case into the usdt directory
> - Fix packaging for the test case in SUNWdtrt
> - Remove leading tab for "all" target in generated Makefile
Quick race over
http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh.patch
(patch code is quoted with "> "):
-- snip --
> --- /dev/null	Sun Nov 16 15:39:56 2008
> +++ new/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh	Sun Nov
16 15:39:55 2008
> @@ -0,0 +1,107 @@
> +#!/bin/ksh -p
1. Assuming your target is is only Solaris >= 10 the "-p" option is
not
needed anymore.
2. Please use /usr/bin/, not /bin/ (saves a softlink lookup)
[snip]
> +
> +PATH=/usr/bin:/usr/sbin:$PATH
> +
> +if [[ $# != 1 ]]; then
Please use (( $# != 1 )) in this case (see
http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions
- $# and $? are integer variables and using a string operator (e.g.
"!="
in [[ expr ]]) causes an extra integer--->string--->integer conversion)
> +	print -u2 ''expected one argument: <dtrace-path>''
> +	exit 2
> +fi
> +
> +#
> +# jdtrace does not implement the -h option that is required to generate
> +# C header files.
> +#
> +if [[ "$1" = */jdtrace ]] ; then
Please replace "=" with "==" - the "=" operator is
depreciated since a
while.
> +	exit 0
> +fi
> +
> +dtrace=$1
> +startdir=$(pwd)
> +dir=$(mktemp -td drtiXXXXXX)
> +if [[ $? != 0 ]] ; then
http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math
> +	print -u2 ''Could not create safe temporary directory''
> +	exit 2
> +fi
> +
> +cd $dir
Please add double-quotes around "$dir" ...
> +
> +cat > Makefile <<EOF
> +all: main
> +
> +main: main.o prov.o
> +	\$(CC) -o main main.o prov.o
> +
> +main.o: main.c prov.h
> +	\$(CC) -c main.c
> +
> +prov.h: prov.d
> +	$dtrace -h -s prov.d
> +
> +prov.o: prov.d main.o
> +	$dtrace -G -32 -s prov.d main.o
> +EOF
> +
> +cat > prov.d <<EOF
> +provider tester {
> +	probe entry();
> +};
> +EOF
> +
> +cat > main.c <<EOF
> +#include <stdlib.h>
> +#include <sys/sdt.h>
> +#include "prov.h"
> +
> +int
> +main(int argc, char **argv, char **envp)
> +{
> +	envp[0] = (char*)0xff;
> +	TESTER_ENTRY();
> +	exit(0);
Erm... AFAIK this should be |return 0;|
> +}
> +EOF
> +
> +make > /dev/null
> +status=$?
> +if [ $status -ne 0 ]; then
Please change this to:
-- snip --
if (( status != 0 )); then
-- snip --
> +	print -u2 "failed to build"
> +else
> +	./main
> +	status=$?
> +fi
> +
> +cd $startdir
Please add double-quotes.
> +rm -rf $dir
> +
> +exit $status
-- snip --

In general it may be nice to check whether the script passes $ ksh93 -n
<scriptname> # (there is at least one warning) ...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

Roland Mainz wrote:
> Mike Gerdts wrote:
> > I''ve posted an updated webrev at:
> >
> > http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/
[snip]
> Quick race over
>
http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh.patch
> (patch code is quoted with "> "):
[snip]> 
> > +     exit 0
> > +fi
> > +
> > +dtrace=$1
> > +startdir=$(pwd)
I missed this issue - AFAIK this should be:
-- snip --
startdir="${PWD}"
-- snip --
... if you want an explicit conversion of physical<--->logical paths
please spell the matching "pwd" option out, e.g. either...
-- snip --
startdir=$(pwd -L)
-- snip --
... or ...
-- snip --
startdir=$(pwd -P)
-- snip --
(note this may require ksh93... I am not sure).

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

Thanks for taking the time to review.  There are two areas of concern
with the changes you recommend:

- Portability.  This test suite is intended to run on all platforms
that Dtrace has been ported to.
- Style.  I started by copying another script in the same directory.
Is the shell style document you reference the accepted standard such
that it trumps consistency with other scripts in the directory?

On Sun, Nov 16, 2008 at 5:08 PM, Roland Mainz <roland.mainz at
nrubsig.org> wrote:
> Mike Gerdts wrote:
>>
>> I''ve posted an updated webrev at:
>>
>> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/
>>
>> Changes since the previous (11-15) webrev include the following
>> changes to the test case.
>>
>> - Move the test case into the usdt directory
>> - Fix packaging for the test case in SUNWdtrt
>> - Remove leading tab for "all" target in generated Makefile
>
> Quick race over
>
http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh.patch
> (patch code is quoted with "> "):
> -- snip --
>> --- /dev/null Sun Nov 16 15:39:56 2008
>> +++ new/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh   
Sun Nov 16 15:39:55 2008
>> @@ -0,0 +1,107 @@
>> +#!/bin/ksh -p
>
> 1. Assuming your target is is only Solaris >= 10 the "-p"
option is not
> needed anymore.
ok, but irrelevant.  :) The script is not even delivered executable -
/opt/SUNWdtrt/bin/dtest calls the script as an argument to ksh and as
such the sh-bang line is really needed at all.
> 2. Please use /usr/bin/, not /bin/ (saves a softlink lookup)
> [snip]
In the event that someone does chmod +x on the file and tries to run
it on BSD or MacOS, will ksh be found in /bin, /usr/bin, or both?
>> +
>> +PATH=/usr/bin:/usr/sbin:$PATH
>> +
>> +if [[ $# != 1 ]]; then
>
> Please use (( $# != 1 )) in this case (see
>
http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions
> - $# and $? are integer variables and using a string operator (e.g.
"!="
> in [[ expr ]]) causes an extra integer--->string--->integer
conversion)
ok
>> +     print -u2 ''expected one argument:
<dtrace-path>''
>> +     exit 2
>> +fi
>> +
>> +#
>> +# jdtrace does not implement the -h option that is required to
generate
>> +# C header files.
>> +#
>> +if [[ "$1" = */jdtrace ]] ; then
>
> Please replace "=" with "==" - the "="
operator is depreciated since a
> while.
Does this apply to the ksh implementation in other supported OS''s
(S10, BSD, MacOS)?  ksh(1) and test(1) could use an update.
>> +     exit 0
>> +fi
>> +
>> +dtrace=$1
>> +startdir=$(pwd)
>> +dir=$(mktemp -td drtiXXXXXX)
>> +if [[ $? != 0 ]] ; then
>
>
http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math
ok
>> +     print -u2 ''Could not create safe temporary
directory''
>> +     exit 2
>> +fi
>> +
>> +cd $dir
>
> Please add double-quotes around "$dir" ...
ok
>> +cat > Makefile <<EOF
>> +all: main
>> +
>> +main: main.o prov.o
>> +     \$(CC) -o main main.o prov.o
>> +
>> +main.o: main.c prov.h
>> +     \$(CC) -c main.c
>> +
>> +prov.h: prov.d
>> +     $dtrace -h -s prov.d
>> +
>> +prov.o: prov.d main.o
>> +     $dtrace -G -32 -s prov.d main.o
>> +EOF
>> +
>> +cat > prov.d <<EOF
>> +provider tester {
>> +     probe entry();
>> +};
>> +EOF
>> +
>> +cat > main.c <<EOF
>> +#include <stdlib.h>
>> +#include <sys/sdt.h>
>> +#include "prov.h"
>> +
>> +int
>> +main(int argc, char **argv, char **envp)
>> +{
>> +     envp[0] = (char*)0xff;
>> +     TESTER_ENTRY();
>> +     exit(0);
>
> Erm... AFAIK this should be |return 0;|
ok
>
>> +}
>> +EOF
>> +
>> +make > /dev/null
>> +status=$?
>> +if [ $status -ne 0 ]; then
>
> Please change this to:
> -- snip --
> if (( status != 0 )); then
> -- snip --
ok
>
>> +     print -u2 "failed to build"
>> +else
>> +     ./main
>> +     status=$?
>> +fi
>> +
>> +cd $startdir
>
> Please add double-quotes.
ok
>
>> +rm -rf $dir
>> +
>> +exit $status
> -- snip --
>
> In general it may be nice to check whether the script passes $ ksh93 -n
> <scriptname> # (there is at least one warning) ...
fair enough.  This would probably be a good check to get integrated
into a lint or similar build pass.

What variant of ksh should I expect to see on other platforms that
support dtrace?  Is the test suite something that needs to be portable
back to S10?

Again, thanks for taking the time to review.

-- 
Mike Gerdts
http://mgerdts.blogspot.com/

Mike Gerdts wrote:
> 
> Thanks for taking the time to review.  There are two areas of concern
> with the changes you recommend:
> 
> - Portability.  This test suite is intended to run on all platforms
> that Dtrace has been ported to.
Right... see below. Technically most platforms already have ksh93 as
/usr/bin/ksh (e.g. Linux, FreeBSD, MacOS X, OpenSolaris/Indiana etc.,
Solaris <= 10 uses ksh88) but there may be still be some platforms with
hacked pdksh clones around (but I doubt that those who have ported
Dtrace fall into this category since "pdksh" is unmaintained since
many
years, leaving "ksh93" as the only maintained and
actively-under-development Korn Shell version around) ... ;-(
> - Style.  I started by copying another script in the same directory.
> Is the shell style document you reference the accepted standard such
> that it trumps consistency with other scripts in the directory?
Erm... please define "accepted standard" ...

(Note that I started the shell style guide document out of desperation
how ksh features are "abused" (which usually leads to bad performace
(something which is unneccesary since even complex applications executed
by ksh93 version ''t'' can reach the same level of performance
of
equivalent JAVA apps)) and not about stuff like whether functions are
intended with one tab or four spaces (maybe we should rename the
document from "style guide" to "coding guidelines" some
day...)).
> On Sun, Nov 16, 2008 at 5:08 PM, Roland Mainz <roland.mainz at
nrubsig.org> wrote:
> > Mike Gerdts wrote:
> >>
> >> I''ve posted an updated webrev at:
> >>
> >> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/
> >>
> >> Changes since the previous (11-15) webrev include the following
> >> changes to the test case.
> >>
> >> - Move the test case into the usdt directory
> >> - Fix packaging for the test case in SUNWdtrt
> >> - Remove leading tab for "all" target in generated
Makefile
> >
> > Quick race over
> >
http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh.patch
> > (patch code is quoted with "> "):
> > -- snip --
> >> --- /dev/null Sun Nov 16 15:39:56 2008
> >> +++ new/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh
Sun Nov 16 15:39:55 2008
> >> @@ -0,0 +1,107 @@
> >> +#!/bin/ksh -p
> >
> > 1. Assuming your target is is only Solaris >= 10 the "-p"
option is not
> > needed anymore.
> 
> ok, but irrelevant.  :)
Ok...
> The script is not even delivered executable -
> /opt/SUNWdtrt/bin/dtest calls the script as an argument to ksh and as
> such the sh-bang line is really needed at all.
Mhhh... what about changing the "sh-bang"-line to
"#!/opt/SUNWdtrt/bin/dtest -s" then where "-s" means
"interpret the
given test script" ?
> > 2. Please use /usr/bin/, not /bin/ (saves a softlink lookup)
> > [snip]
> 
> In the event that someone does chmod +x on the file and tries to run
> it on BSD or MacOS, will ksh be found in /bin, /usr/bin, or both?
I am not sure. At least Linux has seperate /usr/bin/ and /bin/
directories while Solaris''s /bin/ is a softlink to /usr/bin/ ... but I
don''t know how FreeBSD/NetBSD/DragonFly''s filesytem layout
looks like.

[snip]
> >> +# jdtrace does not implement the -h option that is required to
generate
> >> +# C header files.
> >> +#
> >> +if [[ "$1" = */jdtrace ]] ; then
> >
> > Please replace "=" with "==" - the "="
operator is depreciated since a
> > while.
> 
> Does this apply to the ksh implementation in other supported OS''s
> (S10, BSD, MacOS)?
The "==" operator was added in ksh88 version ''g'',
unfortunately the
ksh(1) manual page was never updated to reflect this (it appears to be
at ksh88 version ''e'' level while the binary is a hacked ksh88
version
''i''). See
http://mail.opensolaris.org/pipermail/ksh93-integration-discuss/2008-July/006333.html
for a short evaluation.
> ksh(1) and test(1) could use an update.
Known problem - http://bugs.opensolaris.org/view_bug.do?bug_id=6753139
(''ksh(1) does not document the "==" operator in [[
]]'') was filed to
address this documentation problem but currently the bug is "stuck" in
"4-Defer:No Plan to Fix" (somehow I feel someone thought this is a
ksh88
code bug and not a documentation issue and did the wrong bug triage in
this case... ;-( ).

[snip]
> >> +rm -rf $dir
> >> +
> >> +exit $status
> > -- snip --
> >
> > In general it may be nice to check whether the script passes $ ksh93
-n
> > <scriptname> # (there is at least one warning) ...
> 
> fair enough.
BTW: I forgot to note that $ ksh83 -n <scriptname> # will primarity
complain about POSIX shell standard violations (e.g. "depreciated"
constructs), syntax errors and starting with ksh93 update1 some
statements with "undefined behaviour", e.g. it will not enforce
ksh93-specific features and not complain about stuff like "(( math >= 6
))" vs. "(( $math >= 6 ))" (both forms are valid statements,
however the
2nd form is slower (because ${math} will force a
number-->string--->number conversion cycle) and causes
base2--->base10--->base2 conversion rounding errors for floating-point
values unless you use the C99 "hexfloat" format or use the (future)
decimal floating-point datatype (not supported yet, we have to wait
until Sun Studio implements IEEE754r))).
> This would probably be a good check to get integrated
> into a lint or similar build pass.
Well, we already have the wrapper and infrastructure ready... the
problem is that I still have no sponsor for the bug... ;-(
> What variant of ksh should I expect to see on other platforms that
> support dtrace?
I don''t have a full overview about all platforms which implement
Dtrace.
See above which platforms have ksh93 as /usr/bin/ksh, Solaris 10 still
uses ksh88 as /usr/bin/ksh but we _may_ get ksh93 as /usr/bin/ksh93 in
Solaris 10 at some point (short: the ksh93-integration codebase can
easily be moved to Solaris 10 - the problem is to find a "paying
customer" who throws some $$$$ into Sun''s direction... ;-/ ).
> Is the test suite something that needs to be portable
> back to S10?
Uhm... I don''t know. If you need to be portable across multiple
platforms with different Bourne-shell compatible shells the best option
may be to stick with the feature set defined in the POSIX shell standard
(the shell style guide has "labels" which give hints which fetures are
shell specific - anything marked with "ksh" should work with the POSIX
shell standard while stuff marked with "ksh93" is currently
ksh93-specific).

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

Roland Mainz writes:
> > ksh(1) and test(1) could use an update.
> 
> Known problem - http://bugs.opensolaris.org/view_bug.do?bug_id=6753139
> (''ksh(1) does not document the "==" operator in [[
]]'') was filed to
> address this documentation problem but currently the bug is
"stuck" in
> "4-Defer:No Plan to Fix" (somehow I feel someone thought this is
a ksh88
> code bug and not a documentation issue and did the wrong bug triage in
> this case... ;-( ).
Is this just a man page issue?  If so, I''ll recategorize there.

(Note that "defer; no plan to fix" just means that the manager who
"owns" that subcategory has no plan to assign anyone to the bug.  It
doesn''t mean that the bug can''t be picked up by someone else. 
Indeed,
it means the exact opposite: read it as "someone please come fix me.")

-- 
James Carlson, Solaris Networking              <james.d.carlson at
sun.com>
Sun Microsystems / 35 Network Drive        71.232W   Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757   42.496N   Fax +1 781 442 1677

James Carlson wrote:
> Roland Mainz writes:
> > > ksh(1) and test(1) could use an update.
> >
> > Known problem - http://bugs.opensolaris.org/view_bug.do?bug_id=6753139
> > (''ksh(1) does not document the "==" operator in [[
]]'') was filed to
> > address this documentation problem but currently the bug is
"stuck" in
> > "4-Defer:No Plan to Fix" (somehow I feel someone thought
this is a ksh88
> > code bug and not a documentation issue and did the wrong bug triage in
> > this case... ;-( ).
> 
> Is this just a man page issue?
Yes, IMO this is just a manpage issue. Technically the whole manual page
_may_ need an update to ksh88 version ''i'' since (at least)
Solaris >2.6.
> If so, I''ll recategorize there.
Thanks! ... :-)
... but please keep ksh93-integration-discuss at opensolaris.org in the
bugster "interests" field...

----

Bye,
Roland

P.S.: Reply-To: set to ksh93-integration-discuss
<ksh93-integration-discuss at opensolaris.org>

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)