On 2016-03-15 15:07:09, Tyler Hicks wrote:
> On 2016-03-14 21:09:07, Seth Arnold wrote:
> > On Mon, Mar 14, 2016 at 10:39:09PM -0500, Tyler Hicks wrote:
> > > -         yyerror(_("Unknown error while parsing label: %s\n"), label);
> > 
> > > +                 yyerror(_(err), label);
> > 
> > Does this change break the translator tools that gather strings to
> > translate? (It's a nice cleanup otherwise.)
> 
> It does. Nice catch.
> 
> I've identified a small change to fix it.
> 
> > 
> > >   if (!*_name) {
> > >           free(*_ns);
> > > -         yyerror(_("Memory allocation error."));
> > > +         goto alloc_fail;
> > >   }
> > > +
> > > + return true;
> > > +
> > > +alloc_fail:
> > > + err = "Memory allocation error.";
> > > + if (yyerr)
> > > +         yyerror(_(err));
> > > + else
> > > +         fprintf(stderr, "%s", _(err));
> > > +
> > > + return false;
> > 
> > This appears to be a behaviour change beyond a cleanup -- previously it'd
> > call yyerror() then return true. I suspect this is intentional but I
> > wanted to call it out as more than a cleanup...
> 
> yyerror() actually calls exit() so we're not changing behavior here:
> 
> void yyerror(const char *msg, ...)
> {
>         va_list arg;
> 
>         va_start(arg, msg);
>         vprintyyerror(msg, arg);
>         va_end(arg);
> 
>         exit(1);
> }
> 
> > 
> > > +         if (!kernel_supports_stacking) {
> > > +                 bool stack;
> > > +
> > > +                 if (!parse_label(&stack, &ns, &name,
> > > +                                  tbuf.c_str(), false)) {
> > > +                         return FALSE;
> > > +                 } else if (stack) {
> > 
> > I'm not a fan of this construct:
> > 
> > if (set_variable(&foo))
> >   /* branch A */
> > else if (foo)
> >   /* branch B */
> > 
> > This actually makes branch B dependant upon both the return value of the
> > function _and_ the variable but that's not obvious as the two are
> > separated by some distance. (It's not much, here, but it's enough to be
> > confusing.)
> 
> Do you prefer this instead?
> 
> if (set_variable(&foo))
>   /* branch A */
> 
> if (foo)
>   /* branch B */
> 

Here's the updated changes that I plan to fold into the greater patch.

* Changes:
  - Fixed regression in previous patch when the kernel doesn't support
    stacking
    + Only add the ns to the vec when ns is non-NULL
  - Adjust err message strings so that they're always declared inside
    of _(), which is gettext(), in order for them to show up in the
    generated pot file
  - Adjust flow of process_dfa_entry() conditionals to make them more
    readable.

Tyler

diff --git a/parser/parser.h b/parser/parser.h
index 81f211a..e7acda6 100644
--- a/parser/parser.h
+++ b/parser/parser.h
@@ -298,6 +298,7 @@ extern int kernel_supports_dbus;
 extern int kernel_supports_signal;
 extern int kernel_supports_ptrace;
 extern int kernel_supports_unix;
+extern int kernel_supports_stacking;
 extern int conf_verbose;
 extern int conf_quiet;
 extern int names_only;
@@ -386,7 +387,8 @@ extern char *process_var(const char *var);
 extern int parse_mode(const char *mode);
 extern int parse_X_mode(const char *X, int valid, const char *str_mode, int 
*mode, int fail);
 bool label_contains_ns(const char *label);
-void parse_label(bool *stack, char **ns, char **name, const char *label);
+bool parse_label(bool *_stack, char **_ns, char **_name,
+                const char *label, bool yyerr);
 extern struct cod_entry *new_entry(char *id, int mode, char *link_id);
 
 /* returns -1 if value != true or false, otherwise 0 == false, 1 == true */
diff --git a/parser/parser_common.c b/parser/parser_common.c
index ab36a53..4d5d814 100644
--- a/parser/parser_common.c
+++ b/parser/parser_common.c
@@ -73,6 +73,7 @@ int kernel_supports_dbus = 0;         /* kernel supports dbus 
rules */
 int kernel_supports_diff_encode = 0;   /* kernel supports diff_encode */
 int kernel_supports_signal = 0;                /* kernel supports signal rules 
*/
 int kernel_supports_ptrace = 0;                /* kernel supports ptrace rules 
*/
+int kernel_supports_stacking = 0;      /* kernel supports stacking */
 int conf_verbose = 0;
 int conf_quiet = 0;
 int names_only = 0;
diff --git a/parser/parser_main.c b/parser/parser_main.c
index dbf353d..bf3cf84 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -638,6 +638,8 @@ static void set_supported_features(void)
                                                       "policy/set_load");
        kernel_supports_diff_encode = aa_features_supports(features,
                                                           
"policy/diff_encode");
+       kernel_supports_stacking = aa_features_supports(features,
+                                                       "domain/stack");
 
        if (aa_features_supports(features, "policy/versions/v7"))
                kernel_abi_version = 7;
diff --git a/parser/parser_misc.c b/parser/parser_misc.c
index 22f6fef..4ec04e2 100644
--- a/parser/parser_misc.c
+++ b/parser/parser_misc.c
@@ -661,8 +661,10 @@ bool label_contains_ns(const char *label)
        return _parse_label(&stack, &ns, &ns_len, &name, &name_len, label) == 0 
&& ns;
 }
 
-void parse_label(bool *_stack, char **_ns, char **_name, const char *label)
+bool parse_label(bool *_stack, char **_ns, char **_name,
+                const char *label, bool yyerr)
 {
+       const char *err = NULL;
        char *ns = NULL;
        char *name = NULL;
        size_t ns_len = 0;
@@ -671,19 +673,28 @@ void parse_label(bool *_stack, char **_ns, char **_name, 
const char *label)
 
        res = _parse_label(_stack, &ns, &ns_len, &name, &name_len, label);
        if (res == 1) {
-               yyerror(_("Namespace not terminated: %s\n"), label);
+               err = _("Namespace not terminated: %s\n");
        } else if (res == 2) {
-               yyerror(_("Empty namespace: %s\n"), label);
+               err = _("Empty namespace: %s\n");
        } else if (res == 3) {
-               yyerror(_("Empty named transition profile name: %s\n"), label);
+               err = _("Empty named transition profile name: %s\n");
        } else if (res != 0) {
-               yyerror(_("Unknown error while parsing label: %s\n"), label);
+               err = _("Unknown error while parsing label: %s\n");
+       }
+
+       if (err) {
+               if (yyerr)
+                       yyerror(err, label);
+               else
+                       fprintf(stderr, err, label);
+
+               return false;
        }
 
        if (ns) {
                *_ns = strndup(ns, ns_len);
                if (!*_ns)
-                       yyerror(_("Memory allocation error."));
+                       goto alloc_fail;
        } else {
                *_ns = NULL;
        }
@@ -691,8 +702,19 @@ void parse_label(bool *_stack, char **_ns, char **_name, 
const char *label)
        *_name = strndup(name, name_len);
        if (!*_name) {
                free(*_ns);
-               yyerror(_("Memory allocation error."));
+               goto alloc_fail;
        }
+
+       return true;
+
+alloc_fail:
+       err = _("Memory allocation error.");
+       if (yyerr)
+               yyerror(err);
+       else
+               fprintf(stderr, "%s", err);
+
+       return false;
 }
 
 struct cod_entry *new_entry(char *id, int mode, char *link_id)
diff --git a/parser/parser_regex.c b/parser/parser_regex.c
index 9270076..8cc08c6 100644
--- a/parser/parser_regex.c
+++ b/parser/parser_regex.c
@@ -566,6 +566,8 @@ static int process_dfa_entry(aare_rules *dfarules, struct 
cod_entry *entry)
        if (entry->mode & AA_CHANGE_PROFILE) {
                const char *vec[3];
                std::string lbuf, xbuf;
+               autofree char *ns = NULL;
+               autofree char *name = NULL;
                int index = 1;
 
                if ((warnflags & WARN_RULE_DOWNGRADED) && entry->audit && 
warn_change_profile) {
@@ -585,7 +587,27 @@ static int process_dfa_entry(aare_rules *dfarules, struct 
cod_entry *entry)
                        /* allow change_profile for all execs */
                        vec[0] = "/[^/\\x00][^\\x00]*";
 
-               vec[index++] = tbuf.c_str();
+               if (!kernel_supports_stacking) {
+                       bool stack;
+
+                       if (!parse_label(&stack, &ns, &name,
+                                        tbuf.c_str(), false)) {
+                               return FALSE;
+                       }
+
+                       if (stack) {
+                               fprintf(stderr,
+                                       _("The current kernel does not support 
stacking of named transitions: %s\n"),
+                                       tbuf.c_str());
+                               return FALSE;
+                       }
+
+                       if (ns)
+                               vec[index++] = ns;
+                       vec[index++] = name;
+               } else {
+                       vec[index++] = tbuf.c_str();
+               }
 
                /* regular change_profile rule */
                if (!dfarules->add_rule_vec(entry->deny, AA_CHANGE_PROFILE | 
AA_ONEXEC, 0, index - 1, &vec[1], dfaflags))
diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
index 44793f7..2d95066 100644
--- a/parser/parser_yacc.y
+++ b/parser/parser_yacc.y
@@ -311,7 +311,7 @@ profile_base: TOK_ID opt_id_or_var flags TOK_OPEN rules 
TOK_CLOSE
                        yyerror(_("Memory allocation error."));
                }
 
-               parse_label(&self_stack, &prof->ns, &prof->name, $1);
+               parse_label(&self_stack, &prof->ns, &prof->name, $1, true);
                free($1);
 
                if (self_stack) {

Attachment: signature.asc
Description: PGP signature

-- 
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to