glob in aa-genprof repeats same option

[Kshitij Gupta]

When using glob, the glob does not check if the entries mentioned previously is repeated or not. Using a simple check to match against the previous entry will solve this and prevent such long pointless lists.

| [(A)llow] / (D)eny / (G)lob / Glob w/(E)xt / (N)ew / Abo(r)t / (F)inish / (O)pts
|
| Profile: /usr/sbin/mtr
| Path: /etc/gai.conf
| Mode: r
| Severity: unknown
|
|
| 1 - #include <abstractions/apache2-common>
| 2 - #include <abstractions/nameservice>
| 3 - /etc/gai.conf
| 4 - /etc/*
| 5 - /**
| 6 - /**
| 7 - /**
| 8 - /**
| 9 - /**
| 10 - /**
| 11 - /**
| 12 - /**
| 13 - /**
| 14 - /**
| [15 - /**]
|
| [(A)llow] / (D)eny / (G)lob / Glob w/(E)xt / (N)ew / Abo(r)t / (F)inish / (O)pts

I'm assuming this is a problem with the AppArmor library based in Perl. I'm trying to work a way to fix it.
(BTW, my first bug report ever.)

[Kshitij Gupta (kgupta8592)]

The following patch fixes the above bug by adding the following line (after line 3883 in AppArmor.pm, which is located in /usr/lib/perl5/vendor_perl/5.16.2/Immunix ):
@options=uniq(@options);

I have also provided the updated file for anyone who doesn't want to manually fix the bug.
A perl script to insert the line can be done, but that would be an overkill.

[Kshitij Gupta (kgupta8592)]

The ordering of the options got messed up due to uniq() not preserving the order, hence another line has to be added to the code to make it work. Now the following two lines need to be present (after line 3883 in AppArmor.pm, which is located in /usr/lib/perl5/vendor_perl/5.16.2/Immunix ):

@options = uniq(@options);
@options = sort { length($b) <=> length($a) } @options;

Also I'm attaching the fixed AppArmor.pm .
Sorry for the previous fix.

[Christian Boltz (cboltz)]

Can you please upload a patch instead of the changed file? (You can create the patch with "diff -u AppArmor.pm_orig AppArmor.pm")

[Kshitij Gupta (kgupta8592)]

Yes, sure @cboltz.
Sorry, I wasn't aware how to do that. Here's the patch. Thanks

[Kshitij Gupta (kgupta8592)]

The previous modifications are unnecessary, the bug arose due to programmer matching the array index instead of element while inserting the new option to the list.
Replacing line 3882 with the following fixed the bug, avoid unnecessary use of uniq() and sort() :
if ($newpath ne $options[$selected]) {

The same has been incorporated in the new (and hopefully final) patch

[Kshitij Gupta (kgupta8592)]

I am removing the previous patches and updated files posted by me, since the final patch resolves everything.

[Christian Boltz (cboltz)]

Thanks for the updated patch, but there is a way to trick it ;-)

When you have

| 1 - #include <abstractions/apache2-common>
| 2 - #include <abstractions/nameservice>
| 3 - /etc/gai.conf
| 4 - /etc/*
| 5 - /**

press "4" to select /etc/* again, and then press "g" (glob). This will add "6 - /**" ;-)

I'm not sure if this corner case is worth fixing - I'd say do it only if it isn't too difficult.

It also seems you missed another place (I'd guess line 3899) where "Glob w/(E)xt" is handled because pressing "e" still adds unlimited "/**". You probably need the same fix there.

[Kshitij Gupta (kgupta8592)]

the previous patch I posted which used uniq() would very easily resolve this bug. it will be a matter of simply resorting the list of options. I removed that patch in favour of the solution I found which I believe does what the programmer intended.
I can fix this in no time. :)

Yes, indeed I did not notice that, will combine the solution to all 3 into a single patch. :)

[Kshitij Gupta (kgupta8592)]

There you go, same issue and hence the same code resolves the problem at both the places, first is the appropriate check and second ensures only unique options are displayed sorted by their length (this is one choice followed throughout the code so I didn't meddle with it).
Here's the patch. and I'm removing the previous patch files.

[Kshitij Gupta (kgupta8592)]

The above patch has a little blindspot due to the sorting function being used, so if we had paths longer than the abstraction name, they'll end up coming first, breaking the order of the list. So, I have used smart match for uniqueness and simply pushed the newpath into the options.

This seemed to be the right thing, without getting into how options should be displayed.

[Seth Arnold (seth-arnold)]

Can you explain a little further why one branch negates $newpath and the other branch does not?

+ if ($newpath ne $options[$selected]) {
+ if (not $newpath ~~ @options) {
vs
+ if ($newpath ne $options[$selected]) {
+ if (not !$newpath ~~ @options) {

Thanks

[Kshitij Gupta (kgupta8592)]

Now this patch looks better to me. :-)

[Seth Arnold (seth-arnold)]

I've checked this into trunk. I'm not sure about checking it into 2.8 though, since it introduces a requirement for Perl >= 5.10.1 via the ~~ operator: http://stackoverflow.com/a/3095066/377270

[Seth Arnold (seth-arnold)]

Here's the current Perl versions in Debian / Ubuntu world:

Ubuntu packages:

lucid: 5.10.1-8ubuntu2.3, Pocket: updates, Component: main
precise: 5.14.2-6ubuntu2.3, Pocket: updates, Component: main
quantal: 5.14.2-13ubuntu0.2, Pocket: updates, Component: main
raring: 5.14.2-21, Pocket: release, Component: main
saucy: 5.14.2-21, Pocket: release, Component: main

Other packages:

etch: 5.8.8-7etch6, Pocket: release, Component: main
sarge: 5.8.4-8sarge6, Pocket: release, Component: main
squeeze: 5.10.1-17squeeze6, Pocket: updates, Component: main
unstable: 5.14.2-21, Pocket: release, Component: main

[Christian Boltz (cboltz)]

This patch was commited to 2.8 branch and trunk, and later changed to use grep instead of ~~~.

AppArmor 2.8.3 contains the fix.