python/semanage/seobject.py: Fix undefined store check

Commit Message

Petr Lautrbach May 4, 2018, 11:51 a.m. UTC

From: Vit Mojzis <[e-mail]>

self.store is always a string (actual store name or "") because of
semanageRecords.__init__. Fix check for not defined store.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3

Signed-off-by: Vit Mojzis <[e-mail]>
---
 python/semanage/seobject.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephen Smalley May 4, 2018, 5:58 p.m. UTC | #1

On 05/04/2018 07:51 AM, Petr Lautrbach wrote:
> From: Vit Mojzis <[e-mail]>
> 
> self.store is always a string (actual store name or "") because of
> semanageRecords.__init__. Fix check for not defined store.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3
> 
> Signed-off-by: Vit Mojzis <[e-mail]>
> ---
>  python/semanage/seobject.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
> index ac310ea6..c76dce85 100644
> --- a/python/semanage/seobject.py
> +++ b/python/semanage/seobject.py
> @@ -2651,7 +2651,7 @@ class booleanRecords(semanageRecords):
>              self.current_booleans = []
>              ptype = None
>  
> -        if self.store is None or self.store == ptype:
> +        if self.store == "" or self.store == ptype:
>              self.modify_local = True
>          else:
>              self.modify_local = False
> 

Is there a reason you didn't use if not self.store here?

Petr Lautrbach May 4, 2018, 8:12 p.m. UTC | #2

On Fri, May 04, 2018 at 01:58:08PM -0400, Stephen Smalley wrote:
> On 05/04/2018 07:51 AM, Petr Lautrbach wrote:
> > From: Vit Mojzis <[e-mail]>
> > 
> > self.store is always a string (actual store name or "") because of
> > semanageRecords.__init__. Fix check for not defined store.
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3
> > 
> > Signed-off-by: Vit Mojzis <[e-mail]>
> > ---
> >  python/semanage/seobject.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
> > index ac310ea6..c76dce85 100644
> > --- a/python/semanage/seobject.py
> > +++ b/python/semanage/seobject.py
> > @@ -2651,7 +2651,7 @@ class booleanRecords(semanageRecords):
> >              self.current_booleans = []
> >              ptype = None
> >  
> > -        if self.store is None or self.store == ptype:
> > +        if self.store == "" or self.store == ptype:
> >              self.modify_local = True
> >          else:
> >              self.modify_local = False
> > 
> 
> Is there a reason you didn't use if not self.store here?
> 

There's a similar check on line 258 and this just follows the same pattern.

Stephen Smalley May 7, 2018, 1:58 p.m. UTC | #3

On 05/04/2018 04:12 PM, Petr Lautrbach wrote:
> On Fri, May 04, 2018 at 01:58:08PM -0400, Stephen Smalley wrote:
>> On 05/04/2018 07:51 AM, Petr Lautrbach wrote:
>>> From: Vit Mojzis <[e-mail]>
>>>
>>> self.store is always a string (actual store name or "") because of
>>> semanageRecords.__init__. Fix check for not defined store.
>>>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3
>>>
>>> Signed-off-by: Vit Mojzis <[e-mail]>
>>> ---
>>>  python/semanage/seobject.py | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
>>> index ac310ea6..c76dce85 100644
>>> --- a/python/semanage/seobject.py
>>> +++ b/python/semanage/seobject.py
>>> @@ -2651,7 +2651,7 @@ class booleanRecords(semanageRecords):
>>>              self.current_booleans = []
>>>              ptype = None
>>>  
>>> -        if self.store is None or self.store == ptype:
>>> +        if self.store == "" or self.store == ptype:
>>>              self.modify_local = True
>>>          else:
>>>              self.modify_local = False
>>>
>>
>> Is there a reason you didn't use if not self.store here?
>>
> 
> There's a similar check on line 258 and this just follows the same pattern.

Ok, I don't have a strong opinion on it either way, but noticed that it was recommended
to use not self.store in that bugzilla entry, comment #9, and was claimed to have been changed
in comment #10.  Up to you.

Petr Lautrbach May 7, 2018, 8 p.m. UTC | #4

On Mon, May 07, 2018 at 09:58:28AM -0400, Stephen Smalley wrote:
> On 05/04/2018 04:12 PM, Petr Lautrbach wrote:
> > On Fri, May 04, 2018 at 01:58:08PM -0400, Stephen Smalley wrote:
> >> On 05/04/2018 07:51 AM, Petr Lautrbach wrote:
> >>> From: Vit Mojzis <[e-mail]>
> >>>
> >>> self.store is always a string (actual store name or "") because of
> >>> semanageRecords.__init__. Fix check for not defined store.
> >>>
> >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3
> >>>
> >>> Signed-off-by: Vit Mojzis <[e-mail]>
> >>> ---
> >>>  python/semanage/seobject.py | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
> >>> index ac310ea6..c76dce85 100644
> >>> --- a/python/semanage/seobject.py
> >>> +++ b/python/semanage/seobject.py
> >>> @@ -2651,7 +2651,7 @@ class booleanRecords(semanageRecords):
> >>>              self.current_booleans = []
> >>>              ptype = None
> >>>  
> >>> -        if self.store is None or self.store == ptype:
> >>> +        if self.store == "" or self.store == ptype:
> >>>              self.modify_local = True
> >>>          else:
> >>>              self.modify_local = False
> >>>
> >>
> >> Is there a reason you didn't use if not self.store here?
> >>
> > 
> > There's a similar check on line 258 and this just follows the same pattern.
> 
> Ok, I don't have a strong opinion on it either way, but noticed that it was recommended
> to use not self.store in that bugzilla entry, comment #9, and was claimed to have been changed
> in comment #10.  Up to you.
> 

I think that the important part of the message is not use
`self.store is ""` as it has unpredictable behavior.

The check `not self.store` is already in __init__ on line 252:

 252        if not self.store:                                                                            
 253            self.store = getattr(args, "store", "")

If there's no objection, I'd leave as it is now.


FYI: I'll be offline most time of the week so I won't be able to
respond to emails during this time.

Stephen Smalley May 8, 2018, 12:15 p.m. UTC | #5

On 05/04/2018 07:51 AM, Petr Lautrbach wrote:
> From: Vit Mojzis <[e-mail]>
> 
> self.store is always a string (actual store name or "") because of
> semanageRecords.__init__. Fix check for not defined store.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559174#c3
> 
> Signed-off-by: Vit Mojzis <[e-mail]>
> ---
>  python/semanage/seobject.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
> index ac310ea6..c76dce85 100644
> --- a/python/semanage/seobject.py
> +++ b/python/semanage/seobject.py
> @@ -2651,7 +2651,7 @@ class booleanRecords(semanageRecords):
>              self.current_booleans = []
>              ptype = None
>  
> -        if self.store is None or self.store == ptype:
> +        if self.store == "" or self.store == ptype:
>              self.modify_local = True
>          else:
>              self.modify_local = False
> 

Applied.

Patch

diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
index ac310ea6..c76dce85 100644
--- a/python/semanage/seobject.py
+++ b/python/semanage/seobject.py
@@ -2651,7 +2651,7 @@  class booleanRecords(semanageRecords):
             self.current_booleans = []
             ptype = None
 
-        if self.store is None or self.store == ptype:
+        if self.store == "" or self.store == ptype:
             self.modify_local = True
         else:
             self.modify_local = False