[apparmor] [patch] Split logparser.py add_event_to_tree() into multiple functions
Kshitij Gupta
kgupta8592 at gmail.com
Sun Aug 2 19:03:34 UTC 2015
Hello,
On Sat, Jul 18, 2015 at 2:23 AM, Christian Boltz <apparmor at cboltz.de> wrote:
> Hello,
>
> add_event_to_tree() is a hard-to-test function because it hands over its
> result to add_to_tree().
>
> This patch converts add_event_to_tree() to a simple wrapper function and
> moves the main code into parse_event_for_tree() and map_log_type(). These
> two new functions return their results and are therefore easier to test.
>
>
>
> [ 77-split-logparser-add_event_to_tree.diff ]
>
> diff -ru '--exclude=.bzr'
> ../HEAD-patches-applied/utils/apparmor/logparser.py
> ./utils/apparmor/logparser.py
> --- utils/apparmor/logparser.py 2015-07-17 22:43:21.977879320 +0200
> +++ ./utils/apparmor/logparser.py 2015-07-17 22:45:14.380287480 +0200
> @@ -180,23 +181,35 @@
> #print("log",self.log)
>
> def add_event_to_tree(self, e):
> - aamode = e.get('aamode', 'UNKNOWN')
> - if e.get('type', False):
> + e = self.parse_event_for_tree(e)
> + if e is not None:
> + (pid, parent, mode, details) = e
> + self.add_to_tree(pid, parent, mode, details)
> +
> + def map_log_type(self, type):
>
"type" is a keyword in Python so using it/overriding it in this scope is
not a good idea and we can probably avoid it. How about "log_type" instead?
I'm aware that "type" has been incorrectly used as a variable at other
places, but two wrongs don't make a right ;-)
- if re.search('(UNKNOWN\[1501\]|APPARMOR_AUDIT|1501)',
> e['type']):
> + if re.search('(UNKNOWN\[1501\]|APPARMOR_AUDIT|1501)', type):
> aamode = 'AUDIT'
> - elif re.search('(UNKNOWN\[1502\]|APPARMOR_ALLOWED|1502)',
> e['type']):
> + elif re.search('(UNKNOWN\[1502\]|APPARMOR_ALLOWED|1502)',
> type):
> aamode = 'PERMITTING'
> - elif re.search('(UNKNOWN\[1503\]|APPARMOR_DENIED|1503)',
> e['type']):
> + elif re.search('(UNKNOWN\[1503\]|APPARMOR_DENIED|1503)',
> type):
> aamode = 'REJECTING'
> - elif re.search('(UNKNOWN\[1504\]|APPARMOR_HINT|1504)',
> e['type']):
> + elif re.search('(UNKNOWN\[1504\]|APPARMOR_HINT|1504)', type):
> aamode = 'HINT'
> - elif re.search('(UNKNOWN\[1505\]|APPARMOR_STATUS|1505)',
> e['type']):
> + elif re.search('(UNKNOWN\[1505\]|APPARMOR_STATUS|1505)',
> type):
> aamode = 'STATUS'
> - elif re.search('(UNKNOWN\[1506\]|APPARMOR_ERROR|1506)',
> e['type']):
> + elif re.search('(UNKNOWN\[1506\]|APPARMOR_ERROR|1506)', type):
> aamode = 'ERROR'
> else:
> aamode = 'UNKNOWN'
>
> + return aamode
> +
> + def parse_event_for_tree(self, e):
> + aamode = e.get('aamode', 'UNKNOWN')
> +
> + if e.get('type', False):
> + aamode = self.map_log_type(e['type'])
> +
> if aamode in ['UNKNOWN', 'AUDIT', 'STATUS', 'ERROR']:
> return None
>
> @@ -240,13 +254,13 @@
> e['request_mask'], e['name2'] = log_str_to_mode(e['profile'],
> e['request_mask'], e['name2'])
>
> if e.get('info', False) and e['info'] == 'mandatory profile
> missing':
> - self.add_to_tree(e['pid'], e['parent'], 'exec',
> + return(e['pid'], e['parent'], 'exec',
> [profile, hat, aamode, 'PERMITTING',
> e['denied_mask'], e['name'], e['name2']])
> elif (e.get('name2', False) and '//null-' in e['name2']) or
> e.get('name', False):
> - self.add_to_tree(e['pid'], e['parent'], 'exec',
> + return(e['pid'], e['parent'], 'exec',
> [profile, hat, prog, aamode,
> e['denied_mask'], e['name'], ''])
> else:
> - self.debug_logger.debug('add_event_to_tree: dropped exec
> event in %s' % e['profile'])
> + self.debug_logger.debug('parse_event_for_tree: dropped
> exec event in %s' % e['profile'])
>
> elif ( e['operation'].startswith('file_') or
> e['operation'].startswith('inode_') or
> e['operation'] in ['open', 'truncate', 'mkdir', 'mknod',
> 'chmod', 'rename_src',
> @@ -286,14 +300,14 @@
> self.throw_away_next_log_entry()
>
> if is_domain_change:
> - self.add_to_tree(e['pid'], e['parent'], 'exec',
> + return(e['pid'], e['parent'], 'exec',
> [profile, hat, prog, aamode,
> e['denied_mask'], e['name'], e['name2']])
> else:
> - self.add_to_tree(e['pid'], e['parent'], 'path',
> + return(e['pid'], e['parent'], 'path',
> [profile, hat, prog, aamode,
> e['denied_mask'], e['name'], ''])
>
> elif e['operation'] == 'capable':
> - self.add_to_tree(e['pid'], e['parent'], 'capability',
> + return(e['pid'], e['parent'], 'capability',
> [profile, hat, prog, aamode, e['name'], ''])
>
> elif e['operation'] == 'clone':
> @@ -317,10 +331,10 @@
> # self.pid[child] = arrayref
>
> elif self.op_type(e['operation']) == 'net':
> - self.add_to_tree(e['pid'], e['parent'], 'netdomain',
> + return(e['pid'], e['parent'], 'netdomain',
> [profile, hat, prog, aamode, e['family'],
> e['sock_type'], e['protocol']])
> elif e['operation'] == 'change_hat':
> - self.add_to_tree(e['pid'], e['parent'], 'unknown_hat',
> + return(e['pid'], e['parent'], 'unknown_hat',
> [profile, hat, aamode, hat])
> else:
> self.debug_logger.debug('UNHANDLED: %s' % e)
>
>
> Looks fine to me. Haven't tested it though.
Thanks for the patch.
Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.
>
> Regards,
>
> Christian Boltz
> --
> A qualified candidate would display the following characteristics:
> [...] willing to apply the rules to everybody; primary goal is to
> safeguard quality, not friendship :) You're even allowed to
> decline coolo's request! [Craig Gardner in opensuse-packaging]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
--
Regards,
Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150803/13b32a62/attachment-0001.html>
More information about the AppArmor
mailing list