[cairo] [PATCH] Harden make-cairo-test-constructors.sh
ranma42 at gmail.com
Thu Mar 12 09:02:35 PDT 2015
On Wed, Mar 11, 2015 at 11:13 PM, Bryce Harrington <bryce at osg.samsung.com>
> 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)
> > - (cd $(srcdir) && sh ./make-cairo-test-constructors.sh
> $(test_sources)) > $@
> > + (cd $(srcdir) && sh ./make-cairo-test-constructors.sh
> $(test_sources)) > $@.tmp
> > + mv $@.tmp $@
> This will properly avoid overwriting $@ if the script fails, and make
> will bail at that point. Does $@.tmp get created in that case? If so,
> does anything clean that stray file up? Would mktemp be a cleaner
> solution maybe?
The $@.tmp 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
> > cairo_test_suite_SOURCES = \
> > $(cairo_test_suite_sources) \
> > diff --git a/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.
> > --
> > 1.9.5 (Apple Git-50.3)
> > --
> > cairo mailing list
> > cairo at cairographics.org
> > http://lists.cairographics.org/mailman/listinfo/cairo
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cairo