[v3,19/21] Warn if user-supplied regexes fail to compile.
Commit Message
There are not many calls to regex::compile in the code base. Two of
these pass internally-generated regexes and are followed by assertions
that the regexes compile.
The remaining cases deal with user-supplied regexes. If these fail to
compile they are currently silently ignored.
This patch makes sure failures now result in warning messages on
stderr, but otherwise does not change program behaviour.
* src/abg-corpus-priv.h
(corpus::exported_decls_builder::priv::compiled_regex_fns_suppress):
Emit a warning message if regex::compile fails.
(corpus::exported_decls_builder::priv::compiled_regex_fns_keep):
Ditto.
(corpus::exported_decls_builder::priv::compiled_regex_vars_suppress):
Ditto.
(corpus::exported_decls_builder::priv::compiled_regex_vars_keep):
Ditto.
* src/abg-suppression.cc (maybe_get_string_prop): Ditto.
(read_parameter_spec_from_string): Ditto.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
src/abg-corpus-priv.h | 10 ++++++++++
src/abg-suppression.cc | 4 ++++
2 files changed, 14 insertions(+)
Comments
On Fri, Apr 24, 2020 at 10:21:30AM +0100, Giuliano Procida wrote:
>There are not many calls to regex::compile in the code base. Two of
>these pass internally-generated regexes and are followed by assertions
>that the regexes compile.
>
>The remaining cases deal with user-supplied regexes. If these fail to
>compile they are currently silently ignored.
>
>This patch makes sure failures now result in warning messages on
>stderr, but otherwise does not change program behaviour.
>
> * src/abg-corpus-priv.h
> (corpus::exported_decls_builder::priv::compiled_regex_fns_suppress):
> Emit a warning message if regex::compile fails.
> (corpus::exported_decls_builder::priv::compiled_regex_fns_keep):
> Ditto.
> (corpus::exported_decls_builder::priv::compiled_regex_vars_suppress):
> Ditto.
> (corpus::exported_decls_builder::priv::compiled_regex_vars_keep):
> Ditto.
> * src/abg-suppression.cc (maybe_get_string_prop): Ditto.
> (read_parameter_spec_from_string): Ditto.
Is there an option to fail and terminate if such a configuration issue
is detected?
But reporting the regex is already great as such!
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> src/abg-corpus-priv.h | 10 ++++++++++
> src/abg-suppression.cc | 4 ++++
> 2 files changed, 14 insertions(+)
>
>diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
>index 2618e2d0..a06a1a23 100644
>--- a/src/abg-corpus-priv.h
>+++ b/src/abg-corpus-priv.h
>@@ -29,6 +29,8 @@
> #ifndef __ABG_CORPUS_PRIV_H__
> #define __ABG_CORPUS_PRIV_H__
>
>+#include <iostream>
>+
> #include "abg-sptr-utils.h"
> #include "abg-regex.h"
> #include "abg-internal.h"
>@@ -126,6 +128,8 @@ public:
> regex_t_sptr r = regex::compile(*i);
> if (r)
> compiled_fns_suppress_regexp_.push_back(r);
>+ else
>+ std::cerr << "warning: bad regex '" << *i << "'\n";
> }
> }
> return compiled_fns_suppress_regexp_;
>@@ -148,6 +152,8 @@ public:
> regex_t_sptr r = regex::compile(*i);
> if (r)
> compiled_fns_keep_regexps_.push_back(r);
>+ else
>+ std::cerr << "warning: bad regex '" << *i << "'\n";
> }
> }
> return compiled_fns_keep_regexps_;
>@@ -170,6 +176,8 @@ public:
> regex_t_sptr r = regex::compile(*i);
> if (r)
> compiled_vars_suppress_regexp_.push_back(r);
>+ else
>+ std::cerr << "warning: bad regex '" << *i << "'\n";
> }
> }
> return compiled_vars_suppress_regexp_;
>@@ -192,6 +200,8 @@ public:
> regex_t_sptr r = regex::compile(*i);
> if (r)
> compiled_vars_keep_regexps_.push_back(r);
>+ else
>+ std::cerr << "warning: bad regex '" << *i << "'\n";
> }
> }
> return compiled_vars_keep_regexps_;
>diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
>index 8b3631fd..e4b6554c 100644
>--- a/src/abg-suppression.cc
>+++ b/src/abg-suppression.cc
>@@ -1640,6 +1640,8 @@ maybe_get_regex_prop(const ini::config::section& section,
> if (!maybe_get_string_prop(section, name, str))
> return false;
> regex = regex::compile(str);
>+ if (!regex)
>+ std::cerr << "warning: bad regex '" << str << "'\n";
> return true;
> }
>
>@@ -3157,6 +3159,8 @@ read_parameter_spec_from_string(const string& str)
> if (is_regex)
> {
> type_name_regex = regex::compile(type_name);
>+ if (!type_name_regex)
>+ std::cerr << "warning: bad regex '" << type_name << "'\n";
> type_name.clear();
> }
> function_suppression::parameter_spec* p =
>--
>2.26.2.303.gf8c07b1a785-goog
>
Hi.
On Mon, 27 Apr 2020 at 16:36, Matthias Maennich <maennich@google.com> wrote:
>
> On Fri, Apr 24, 2020 at 10:21:30AM +0100, Giuliano Procida wrote:
> >There are not many calls to regex::compile in the code base. Two of
> >these pass internally-generated regexes and are followed by assertions
> >that the regexes compile.
> >
> >The remaining cases deal with user-supplied regexes. If these fail to
> >compile they are currently silently ignored.
> >
> >This patch makes sure failures now result in warning messages on
> >stderr, but otherwise does not change program behaviour.
> >
> > * src/abg-corpus-priv.h
> > (corpus::exported_decls_builder::priv::compiled_regex_fns_suppress):
> > Emit a warning message if regex::compile fails.
> > (corpus::exported_decls_builder::priv::compiled_regex_fns_keep):
> > Ditto.
> > (corpus::exported_decls_builder::priv::compiled_regex_vars_suppress):
> > Ditto.
> > (corpus::exported_decls_builder::priv::compiled_regex_vars_keep):
> > Ditto.
> > * src/abg-suppression.cc (maybe_get_string_prop): Ditto.
> > (read_parameter_spec_from_string): Ditto.
>
> Is there an option to fail and terminate if such a configuration issue
> is detected?
That is definitely an option (and one I'd personally prefer). However,
we should be mindful of making breaking changes. We could have all
parsing/regex compilation etc. problems cause error messages in the
next major release of libabigail and make the errors terminate the
tool in the one after, for example.
Giuliano.
> But reporting the regex is already great as such!
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
>
> Cheers,
> Matthias
>
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > src/abg-corpus-priv.h | 10 ++++++++++
> > src/abg-suppression.cc | 4 ++++
> > 2 files changed, 14 insertions(+)
> >
> >diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
> >index 2618e2d0..a06a1a23 100644
> >--- a/src/abg-corpus-priv.h
> >+++ b/src/abg-corpus-priv.h
> >@@ -29,6 +29,8 @@
> > #ifndef __ABG_CORPUS_PRIV_H__
> > #define __ABG_CORPUS_PRIV_H__
> >
> >+#include <iostream>
> >+
> > #include "abg-sptr-utils.h"
> > #include "abg-regex.h"
> > #include "abg-internal.h"
> >@@ -126,6 +128,8 @@ public:
> > regex_t_sptr r = regex::compile(*i);
> > if (r)
> > compiled_fns_suppress_regexp_.push_back(r);
> >+ else
> >+ std::cerr << "warning: bad regex '" << *i << "'\n";
> > }
> > }
> > return compiled_fns_suppress_regexp_;
> >@@ -148,6 +152,8 @@ public:
> > regex_t_sptr r = regex::compile(*i);
> > if (r)
> > compiled_fns_keep_regexps_.push_back(r);
> >+ else
> >+ std::cerr << "warning: bad regex '" << *i << "'\n";
> > }
> > }
> > return compiled_fns_keep_regexps_;
> >@@ -170,6 +176,8 @@ public:
> > regex_t_sptr r = regex::compile(*i);
> > if (r)
> > compiled_vars_suppress_regexp_.push_back(r);
> >+ else
> >+ std::cerr << "warning: bad regex '" << *i << "'\n";
> > }
> > }
> > return compiled_vars_suppress_regexp_;
> >@@ -192,6 +200,8 @@ public:
> > regex_t_sptr r = regex::compile(*i);
> > if (r)
> > compiled_vars_keep_regexps_.push_back(r);
> >+ else
> >+ std::cerr << "warning: bad regex '" << *i << "'\n";
> > }
> > }
> > return compiled_vars_keep_regexps_;
> >diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
> >index 8b3631fd..e4b6554c 100644
> >--- a/src/abg-suppression.cc
> >+++ b/src/abg-suppression.cc
> >@@ -1640,6 +1640,8 @@ maybe_get_regex_prop(const ini::config::section& section,
> > if (!maybe_get_string_prop(section, name, str))
> > return false;
> > regex = regex::compile(str);
> >+ if (!regex)
> >+ std::cerr << "warning: bad regex '" << str << "'\n";
> > return true;
> > }
> >
> >@@ -3157,6 +3159,8 @@ read_parameter_spec_from_string(const string& str)
> > if (is_regex)
> > {
> > type_name_regex = regex::compile(type_name);
> >+ if (!type_name_regex)
> >+ std::cerr << "warning: bad regex '" << type_name << "'\n";
> > type_name.clear();
> > }
> > function_suppression::parameter_spec* p =
> >--
> >2.26.2.303.gf8c07b1a785-goog
> >
@@ -29,6 +29,8 @@
#ifndef __ABG_CORPUS_PRIV_H__
#define __ABG_CORPUS_PRIV_H__
+#include <iostream>
+
#include "abg-sptr-utils.h"
#include "abg-regex.h"
#include "abg-internal.h"
@@ -126,6 +128,8 @@ public:
regex_t_sptr r = regex::compile(*i);
if (r)
compiled_fns_suppress_regexp_.push_back(r);
+ else
+ std::cerr << "warning: bad regex '" << *i << "'\n";
}
}
return compiled_fns_suppress_regexp_;
@@ -148,6 +152,8 @@ public:
regex_t_sptr r = regex::compile(*i);
if (r)
compiled_fns_keep_regexps_.push_back(r);
+ else
+ std::cerr << "warning: bad regex '" << *i << "'\n";
}
}
return compiled_fns_keep_regexps_;
@@ -170,6 +176,8 @@ public:
regex_t_sptr r = regex::compile(*i);
if (r)
compiled_vars_suppress_regexp_.push_back(r);
+ else
+ std::cerr << "warning: bad regex '" << *i << "'\n";
}
}
return compiled_vars_suppress_regexp_;
@@ -192,6 +200,8 @@ public:
regex_t_sptr r = regex::compile(*i);
if (r)
compiled_vars_keep_regexps_.push_back(r);
+ else
+ std::cerr << "warning: bad regex '" << *i << "'\n";
}
}
return compiled_vars_keep_regexps_;
@@ -1640,6 +1640,8 @@ maybe_get_regex_prop(const ini::config::section& section,
if (!maybe_get_string_prop(section, name, str))
return false;
regex = regex::compile(str);
+ if (!regex)
+ std::cerr << "warning: bad regex '" << str << "'\n";
return true;
}
@@ -3157,6 +3159,8 @@ read_parameter_spec_from_string(const string& str)
if (is_regex)
{
type_name_regex = regex::compile(type_name);
+ if (!type_name_regex)
+ std::cerr << "warning: bad regex '" << type_name << "'\n";
type_name.clear();
}
function_suppression::parameter_spec* p =