Re: [PATCH] id: fix: check return value of smack_new_label_from_path() < 0

[Jarkko Sakkinen]

On Tue, Jun 4, 2013, at 22:39, Jarkko Sakkinen wrote:
> 
> 
> On Tue, Jun 4, 2013, at 2:23, Pádraig Brady wrote:
> > On 06/03/2013 06:15 PM, Jarkko Sakkinen wrote:
> > > Check that smack_new_label_from_path() < 0 and not just non-zero.
> > > There was slight change to libsmack such that positive values are
> > > reserved for returning length of the label.
> > > ---
> > >  src/id.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/id.c b/src/id.c
> > > index 937b723..c91dbcd 100644
> > > --- a/src/id.c
> > > +++ b/src/id.c
> > > @@ -210,7 +210,8 @@ main (int argc, char **argv)
> > >        if (selinux_enabled && getcon (&context) && just_context)
> > >          error (EXIT_FAILURE, 0, _("can't get process context"));
> > >  #ifdef HAVE_SMACK
> > > -      else if (smack_enabled && smack_new_label_from_self ((char **) 
> > > &context))
> > > +      else if (smack_enabled
> > > +               && smack_new_label_from_self ((char **) &context) < 0)
> > >          error (EXIT_FAILURE, 0, _("can't get process context"));
> > >  #endif
> > >      }
> > 
> > That's not a slight change, it seems to be a backwards incompat
> > change that would break all existing users of
> > smack_new_label_from_self().
> > Are you assuming that one is always using the latest libsmack?
> > Should there be checks in m4 etc. to enforce any of this?
> 
> We are planning to freeze and tag 1.0 version of libsmack after changes
> for 'id' and 'ls are accepted. This is last API change before doing
> that.
> 
> I think there should be checks in m4 to check both
> smack_new_label_from_self() and smack_label_from_path() so I will put
> effort to get those checks in.

That said this patch had error in subject line (should have 
smack_new_label_from_self). I'm planning to freeze and tag 1.0 
version of libsmack in the latter part of next week (probably on
Thu) regardless of whether 'ls' changes have landed before that
so you don't have to worry about moving target.

> 
> Other thing I've been thinking should I also update man pages? They
> speak now specifically about SELinux. Should they talk about LSM or
> security context instead?
> 
> > 
> > thanks,
> > Pádraig.
> > 
> 
> /Jarkko

/Jarkko