Still can not obfuscate based on public/private (one line fix, patch avail)

[GoogleCodeExporter]

What steps will reproduce the problem?
1. use attrib="protected" (or public)
2. view output with monodis or other disassembler

Methods and fields are not obfuscated based on visibility, and only the
rx/name/type patterns are used.

This appears in release version 1.5.0, and svn rev 62 (latest).

MethodTester::CheckMethodVisibility always returns true. Change its last
line to return false, and the program works as expected.

Index: Obfuscar/MethodTester.cs
===================================================================
--- Obfuscar/MethodTester.cs    (revision 62)
+++ Obfuscar/MethodTester.cs    (working copy)
@@ -92,7 +92,7 @@
                                else
                                        throw new
ApplicationException(string.Format("'{0}' is not valid for the 'attrib'
value of skip elements. Only 'public' and 'protected' are supported by
now.", attribute));
                        }
-                       return true;
+                       return false; // does not match, is
private/internal/etc
                }
        }
 }

Original issue reported on code.google.com by tekh...@gmail.com on 9 Nov 2009 at 9:52

[GoogleCodeExporter]

This behavior actually is by design.
Each of the SkipXXX rules determine which things are not to be obfuscated based 
on
the type and member name. When an additional attrib value of 'public' or 
'protected'
exists, then this skip rule only applies to 'public' (and 'protected') members.
Maybe I am wrong, but I think the attrib feature is mainly for having an 
"public" (in
the classical meaning) programming interface and still hide/obfuscate the 
"private"
internals.

Original comment by webbi...@gmail.com on 12 Nov 2009 at 2:10

[GoogleCodeExporter]

In the shipping version of the code, CheckMethodVisibility *always* returns 
true.
This is clearly incorrect. :) 

Based on the method name, I am assuming that it should return true if the 
method is
visible, and false otherwise. In the absence of other documentation or 
comments, this
is what I have to go on. 

The end result is that this means if you supply attrib="public", non-public 
methods
that match the Type= pattern are *never* skipped, and other methods are 
obfuscated
based on the name/rx pattern. 

I would guess that the intended behavior is this:

  skip only methods that match both the attrib and the name/rx pattern. 

This would allow you to skip only private methods that start with "m". In the 
current
implementation the name/rx pattern is ignored when attrib= is supplied, which is
unintuitive.

I can implement this if you like, by modifying the relevant Test() methods. 

attrib= sort of works with the change I provide. It definitely does not with the
shipping version. I checked!

Original comment by tekh...@gmail.com on 12 Nov 2009 at 6:56

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Sorry, when I say "the end result is that this means", I mean this is the 
result if
you use my one-line patch. I am also wrong...(sorry)

IF you use my one-line patch, the behavior is this: if you specify 
attrib="public",
public methods that match the Type= pattern are *always* skipped, and private 
methods
are skipped only if they match the pattern in name/rx.

I get confused by the logic. If Test() returns true, the element is /skipped/. 
Hard
to keep that straight in my head. :) This inversion of the logic is also why the
behavior is not what you would expect, I'm sure. :)

Original comment by tekh...@gmail.com on 12 Nov 2009 at 7:06

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I deleted my last two comments. My change made no change. I'll look at this 
again
after a nap and some tea.

It really should be trivial to implement just about any behavior here, the real
question is: when you specify a skip pattern with attrib= and rx=, what exactly
should be skipped? I would assume it should skip only things that match both 
criteria!

Original comment by tekh...@gmail.com on 12 Nov 2009 at 7:20

[GoogleCodeExporter]

Sorry, you are completely right.
The CheckMethodVisibility() always returns true. This is obviously wrong.
The inversion of logic seems really to have confused me. (I tested the code 
twice,
but I seem to have mixed up the colors and icons for the different member types 
in
Reflector.)

Rev 64 contains the fix. I hopefully got it right this time.

The logic is (at least should be :)) the following: A member is NOT obfuscated 
when:
the type matches AND the member name matches AND (attrib is empty/non-existent 
OR
member accessibility is public/protected).

Could you please test if this now works correctly for you? I will make a 1.5.1
release then.

Original comment by webbi...@gmail.com on 13 Nov 2009 at 8:55

[GoogleCodeExporter]

Yes I will test it now. (Sorry so long to reply, I've been dealing with a 
failing
hard drive.)

Original comment by tekh...@gmail.com on 28 Nov 2009 at 8:07

[GoogleCodeExporter]

The code works fine, but I think the comments are backwards. :) If
CheckMethodVisibilty returns *true*, the method is *skipped*.

So, the comment on the last line of the file (if "no attrib is supplied") is 
misleading:

return true; // No attrib value given: we obfuscate always

In fact, returning true means we "check the member regular expressions to see 
whether
we should skip", and returning false means "always skip".

The current logic appears to be this:

A member is *skipped* when:

  the type matches
  AND 
  (
    ( attrib value is supplied AND member is public/protected )
    OR
    the member name matches
  )

This works for me, but differs from what is desired. Apparently. :)

Original comment by tekh...@gmail.com on 28 Nov 2009 at 8:44

[GoogleCodeExporter]

Correction, if CheckMethodVisibility returns *false* the member is never 
skipped. If
it returns true, the member pattern is checked. Or is it? 

OK this logic is mind numbing.

Original comment by tekh...@gmail.com on 28 Nov 2009 at 8:46

[GoogleCodeExporter]

OK, this patch should implement the logic flow you describe. It seems to work 
for me.
Sorry about the space-indents, I need to start using a project-specific 
environment
if I'm going to make sweeping changes like this. :)

Original comment by tekh...@gmail.com on 28 Nov 2009 at 9:32

Attachments:

• test-logic-rewrite.diff

[GoogleCodeExporter]

augh, and true to form, there's a wrong comment in the patch. //visible, and 
skipped
should be // not visible, not skipped. Guess I could fix those tabs too. 

Original comment by tekh...@gmail.com on 28 Nov 2009 at 9:35

[GoogleCodeExporter]

Again, with comments and tabs fixed

Original comment by tekh...@gmail.com on 28 Nov 2009 at 9:41

Attachments:

• test-logic-retabbed.diff

[GoogleCodeExporter]

Third time's a charm. In order for the logic to work when no attrib= value is
supplied, this code has to assume that it is *visible*. I think this version 
actually
works as intended.

Original comment by tekh...@gmail.com on 28 Nov 2009 at 9:59

Attachments:

• third-times-a-charm.diff

[GoogleCodeExporter]

Thanks for testing.
My comments in the code are indeed the opposite way around.
(The whole skip logic should be simple, but seemingly has it's own challenges.)

I just checked some test cases and I think my code works correctly. I also 
analyzed
your code, and I think it implements the same logic only in another way.

Maybe it's better to look at the whole attrib-logic from another point of view:
When an attrib value exists, the Skip* element in the configuration is not 
applied to
private members. (I only look at the attrib="protected" case here to simplify 
things)

The typical test pattern in the current code is the following (returning true 
means
the member should not be obfuscated):

if ( Helper.CompareOptionalRegex(prop.TypeKey.Fullname, type) &&
MethodTester.CheckMethodVisibility(attrib,prop.GetterMethodAttributes) )
{
    if ( name != null )
      return Helper.CompareOptionalRegex(prop.Name, name);
    else
      return nameRx.IsMatch( prop.Name );
}
return false;

CheckMethodVisibility() returns false when an attrib value exists and the 
member is
private. In this case the whole test pattern returns false => The member is 
obfuscated.
Checking the member name is not necessary in this case, because it is 
obfuscated in
any case.

I hope I am not totally wrong on this.

Original comment by webbi...@gmail.com on 30 Nov 2009 at 9:33

[GoogleCodeExporter]

I never combine attrib with a pattern, so it's a moot point to me.

However, the current code in svn does not implement the logic that you described
above. You might want to add a comment to the (several) functions that 
implement this
same logic describing what the intended behavior is, as it is not really 
obvious a)
what it does or b) what it is supposed to do, and therefore c) whether it is 
correct. :)

Code in SVN seems to implement:

Skip if:
  ( the type name matches 
    AND 
    ( attrib is not supplied 
      OR
      the member is public
    )
  )
  AND
  the name or rx matches

So, this means it skips members that are ... "visible" *and* match the pattern
("visible" is defined as "match the attrib= value if supplied, true otherwise")

Works for me.

Original comment by tekh...@gmail.com on 30 Nov 2009 at 8:29

[GoogleCodeExporter]

> So, this means it skips members that are ... "visible" *and* match the pattern
> ("visible" is defined as "match the attrib= value if supplied, true 
otherwise")

This describes exactly what I intended and what I think most users expect. The 
attrib
value is just another rule besides the type and member pattern which all three 
must
be true for obfuscation to be skipped.

The code works now and refactoring it with a common test function is certainly
desirable but would require additional work and testing.

> Works for me.

Ok. I will change the state of this issue to "fixed".

Original comment by webbi...@gmail.com on 1 Dec 2009 at 9:45

• Changed state: Fixed