Re: [PATCH] Harden make-cairo-test-constructors.sh

On Wed, Mar 11, 2015 at 11:13 PM, Bryce Harrington <[email protected]>
wrote:

> On Wed, Mar 11, 2015 at 03:29:26PM +0100, Andrea Canciani wrote:
> > The make-cairo-test-constructors.sh script executes several commands
> > without checking their success. This can lead to undetected errors,
> > like those fixed in 86fad78fcd2bf987249890aea4eabcce02a58f45.
> >
> > Setting the '-e' flag and writing to a temporary file ensures that the
> > 'cairo-test-constructors.c' target completes succesfully only if no
> > error occurred.
> > ---
> >  test/Makefile.am                     | 3 ++-
> >  test/make-cairo-test-constructors.sh | 2 ++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/test/Makefile.am b/test/Makefile.am
> > index 950629b..a56e7ff 100644
> > --- a/test/Makefile.am
> > +++ b/test/Makefile.am
> > @@ -81,7 +81,8 @@ noinst_SCRIPTS = check-refs.sh
> >  TESTS += cairo-test-suite$(EXEEXT)
> >
> >  cairo-test-constructors.c: Makefile $(test_sources)
> make-cairo-test-constructors.sh
> > -     (cd $(srcdir) && sh ./make-cairo-test-constructors.sh
> $(test_sources)) > $@
> > +     (cd $(srcdir) && sh ./make-cairo-test-constructors.sh
> $(test_sources)) > [email protected]
> > +     mv [email protected] $@
>
> This will properly avoid overwriting $@ if the script fails, and make
> will bail at that point.  Does [email protected] get created in that case?  If so,
> does anything clean that stray file up?  Would mktemp be a cleaner
> solution maybe?
>

The [email protected] file is generated both if the make-cairo-test-constructors.sh
script succeeds and if it fails; in the first case it is renamed to $@, but
upon error it stays and it is not cleaned up.
AFAICT mktemp would not address the issue, as it would only generate a
unique filename, but it would not ensure that the file is eventually
removed.

>
>
> >  cairo_test_suite_SOURCES =           \
> >       $(cairo_test_suite_sources)     \
> > diff --git a/test/make-cairo-test-constructors.sh
> b/test/make-cairo-test-constructors.sh
> > index cb1391e..9b78eba 100644
> > --- a/test/make-cairo-test-constructors.sh
> > +++ b/test/make-cairo-test-constructors.sh
> > @@ -1,5 +1,7 @@
> >  #! /bin/sh
> >
> > +set -e
> > +
>
> Can you verify that your sed exits with non-zero exit code in the
> 86fad78f case?
>

Yes, I tested that.


>
> >  if test $# -eq 0; then
> >      echo "$0: no input files." >&2
> >      exit 0
>
> Shouldn't this exit 1 since it's another error condition?
>

Yes, I think that's much better.

I just sent in a revised patch that should address your comments.

Andrea


>
> > --
> > 1.9.5 (Apple Git-50.3)
> >
> > --
> > cairo mailing list
> > [email protected]
> > http://lists.cairographics.org/mailman/listinfo/cairo
>

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo