Detecting String Literals with FxCop 1.36

Question

Hello,

I will just describe my desired result: I need to be able to detect string literals in my project. I could simply just look for every "ldstr" instruction and report a problem, but I need to ignore a string literal if it's passed into a special method (e.g. "Strings.Ignore(System.String)" where the return type is "System.String"). I tried looking for every "ldstr" and then tracing the instructions to see if they eventually called the ignore method, but I think I made too many assumptions (or not enough). It was creating false positives, or not ignoring certain strings. I don't know IL or FxCop 1.36 too well, and I can't seem to find examples of this.

Just to add more information, I should be able to ignore the following "ldstr" instructions:

byte[] array = new byte[]{0,1,2,3,4,5,6,7,8};

string a = string.Format(Strings.Ignore("Format: {0}"), 5);
string b = Strings.Ignore("Ignore this");
string c = string.Format(Strings.Ignore("{0}{1}{2}"), m_Int, m_Bool, m_Float);
string d = Strings.Ignore("Array has: " + array.Length + " elements.");

Basically, anytime ANY string literal is passed into the Strings.Ignore method, it needs to be ignored. Otherwise, it needs to be detected and reported as a problem. If a string is stored inside a variable, and then that variable is passed into Strings.Ignore, then it isn't ignored. "ldstr" should only be ignored if they're passed to the ignore method.

Answer 1

Could you please let us know what you've got so far and where it's generating false positives or other problems?

Answer 2

Sure thing. Here is what I have so far:

public class StringLiteralsShouldBeInAResxOrIgnored : BaseIntrospectionRule
  {
    private int mID;
    private List<string> mIgnoredCalls;

    public StringLiteralsShouldBeInAResxOrIgnored () :
      base("StringLiteralsShouldBeInAResxOrIgnored ",
      "FxCopRules.Localization",
      typeof(StringLiteralsShouldBeInAResxOrIgnored ).Assembly)
    {

    }

    public override void BeforeAnalysis()
    {
      // initialize the id
      mID = 0;

      // Here, we can add just the namespaces, or specific classes, even specific methods.
      mIgnoredCalls = new List<string>()
      {
        "Strings.Ignore",
      };
    }

    public override ProblemCollection Check(Member member)
    {
      if (member is Method)
      {
        // we need to check the instructions of the method
        Method method = member as Method;
        if (!IsExcluded(method.Name.Name))
        {
          List<Instruction> instructions = method.Instructions.ToList();
          for (int i = 0; i < instructions.Count; ++i)
          {
            Instruction instr = instructions[i];
            if (instr.OpCode == OpCode.Ldstr)
            {
              // create an array of the instructions<br />              Instruction[] array = instructions.ToArray();

              // see if we're calling a forbidden method
              if (mIgnoredCalls.FindIndex(methodName => CallingMethod(array, i, methodName)) > -1)
                continue;

              // if we get here, then add a new problem
              this.Problems.Add(new Problem(GetResolution(instr.Value), mID.ToString()));

              // increment the ID
              ++mID;
            }
          }
        }
      }

      return this.Problems;
    }

    private bool CallingMethod(Instruction[] instructions, int currentIndex, string methodName)
    {
      // get a return value
      bool retval = false;

      // if the index is invalid, then return
      if (0 > currentIndex || currentIndex > (instructions.Length - 1))
        return false;

      // verify that the instruction is a load string instruction at the current index
      if (instructions[currentIndex].OpCode == OpCode.Ldstr)
      {
        // get the next instruction.
        Instruction next = instructions[++currentIndex];

        // so, we called Ldstr, if we next called load
        while ((IsLoading(next.OpCode) || IsSpecial(next.OpCode)) && (currentIndex + 1) < instructions.Length)
        {
          // so, get the next instruction since it's the one we care about
          next = instructions[++currentIndex];
        }

        // get the value of the next instruction as a string
        string name = GetMethodName(next.Value as Method);

        // okay, so, we called ldstr and possibly load. What are we doing next?
        while ((IsCalling(next.OpCode) || IsSpecial(next.OpCode)) && !name.Contains(methodName) && (currentIndex + 1) < instructions.Length)
        {
          // we called a method, but it isn't Strings.Ignore, so, get the next instruction
          next = instructions[++currentIndex];
          name = GetMethodName(next.Value as Method);
        }

        // if this next instruction's value ends with Strings.Ignore, then return true
        retval = name.Contains(methodName);
      }

      // return the retval
      return retval;
    }

    private string GetMethodName(Method method)
    {
      // the default value is string.Empty here so that we don't use a null string
      // of comparison
      string retval = string.Empty;
      if (method != null)
      {
        retval = method.ToString();
      }
      return retval;
    }

    private bool IsSpecial(OpCode opCode)
    {
      // if this is a for replacing an array element, or converting to an object, then we'll mark this as special.
      // TODO: We could see what type the Box instruction is converting to.
      return (opCode.ToString().ToLower().StartsWith("ste")) || (opCode == OpCode.Box);
    }

    private bool IsLoading(OpCode opCode)
    {
      // if this is a load instruction (starts with "ld"), then return true
      return opCode.ToString().ToLower().StartsWith("ld");
    }

    private bool IsCalling(OpCode opCode)
    {
      // if this is a call instruction (call, calli, callvirt), then return true
      return opCode.ToString().ToLower().StartsWith("call");
    }
  }

Here is the code I have. I didn't want to post this code because it doesn't work and I was hoping to start from scratch with something that did work, or at least have a point of reference.

Answer 3

Could you please also give examples of the problems (detections and false positives) mentioned in your first post?

Answer 4

The following examples aren't detected:  

private class Character
{
  public char GetLabel()
  {
    return 'c';
  }
}

private Character thisCharacter = new Character();
private int iInt = 32948;
private bool bBool = true;
private float fFloat = 2394.34f;

public void TestOne()    
{
 byte[] pictureData = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
 char c = 'A';

 Strings.Ignore("test string 1 = " + thisCharacter.GetLabel().ToString());
 Strings.Ignore(string.Format(CultureInfo.CurrentCulture, "test string 2:  {0}{1}{2}{3}{4}", iInt, bBool, fFloat, bBool, iInt));
 Strings.Ignore("test string 3 bytes length: " + pictureData.Length);
 Strings.Ignore("test string 4 (" + c.ToString() + ", " + iInt.ToString() + " test string 5)");
}

public void TestTwo(Exception ex, string str)
{
 string message =
        Strings.Ignore("test string 6 " +
        ex.GetType().Name + " test string 7 " + str + 
        "test string 8.\r\n") + ex.Message + Strings.Ignore("\r\n") +
        ex.StackTrace;
 Console.WriteLine(message);
}

There are a lot of other examples, but they pretty much follow that format. I don't have any examples of false postives because I mispoke (or mis-typed).

Answer 5

Umm...  Exactly what in that rather large sample should be detected but isn't?

Answer 6

Umm...  Exactly what in that rather large sample should be detected but isn't?

Everything. For every "ldstr" instruction, I report it as a problem (this.Problems.Add(...)) unless it's passed to the Strings.Ignore function in which I ignore it (hence the name of the function). But, the following tests (which I have above), still report a problem for the "ldstr" instruction. They're reported based on the FxCop 1.36 rule code (which I also have above).

You asked me to provide a sample of what I have, which I did, and to provide a sample of the problems, which I did.

I have the desired effect outlined in my original post:

"Basically, anytime ANY string literal is passed into the Strings.Ignore method, it needs to be ignored. Otherwise, it needs to be detected and reported as a problem. If a string is stored inside a variable, and then that variable is passed into Strings.Ignore, then it isn't ignored. "ldstr" should only be ignored if they're passed to the ignore method."

To describe it further, in my second post, I gave the exact code of the FxCop rule I'm using to detect all literal strings in my test project. In my previous post (the one before this), I gave the exact code of the class I'm using to test my FxCop rule. The class is only two functions, and an inline class and is only for testing my FxCop rule. However, my tests fail because not all of the literal strings passed into Strings.Ignore are ignored, and they should be.

So, back to my original post: Is there anything (samples, references, books...anything), that could help me create a rule which would ignore all "ldstr" instructions passed into my Strings.Ignore function?

Answer 7

So, back to my original post: Is there anything (samples, references, books...anything), that could help me create a rule which would ignore all "ldstr" instructions passed into my Strings.Ignore function?

AFAIK, the only available resources for this level of detail in custom rule development are this forum and use of Reflector.  (And, BTW, you never actually asked about resources in your original post, so I've been treating this as a request for coding help from the beginning.)

For every "ldstr" instruction, I report it as a problem (this.Problems.Add(...)) unless it's passed to the Strings.Ignore function in which I ignore it (hence the name of the function). But, the following tests (which I have above), still report a problem for the "ldstr" instruction.

That's what's known as a false positive (i.e.: reporting a rule violation where there is no actual problem).  Since you mentioned that you had no false positives, perhaps you can understand my confusion regarding the unexpected results...

Unfortunately, I'm still not clear on the exact rules regarding what qualifies as being "passed" to Strings.Ignore.  It seems like you have no objection to a literal string being concatenated with something else before being passed to Strings.Ignore, but what about being passed to a method other than String.Concat first?  e.g.:

Strings.Ignore(SomeMethod("abc"))

Incidentally, a quick look at your rule implementation shows that you're not screening out String.Concat calls, which might explain most of your false positives.

Answer 8

You are absolutely correct. I didn't ask for resources in my original post. I apologize. I also apologize about the false positives. I was thinking of an instance where the IL would look exactly like an example of passing a literal string to Strings.Ignore, but that's not how the code would execute it (so my rule code would ignore it). I'm not sure if that's the same thing as what you said above.

To be honest, I'd prefer coding help, but I'd settle with resources, or samples (which doesn't seem possible).

When I say "passed" to Strings.Ignore, I mean that the string isn't explicitly placed inside of a variable with a "store" instruction. So for example: string a = "Some literal string" or this.mField = "Some literal string", these strings are explicitly stored in a variable. If a "ldstr" is detected, but passed into Strings.Ignore, then it should be ignored; otherwise it should be reported.

Strings.Ignore(SomeMethod("abc")) would be ignored. A literal string ("abc") is loaded and eventually "passed" to Strings.Ignore and it isn't explicity stored inside a variable. However, something string like:

string str = "abc";
Strings.Ignore(SomeMethod(str))

...would be reported. Although we loaded a literally string, we're explicitly storing this inside a variable.

This of course has the possiblity to grow into something like:

Strings.Ignore(SomeOtherMethod(SomeMethod("abc") + GetSomeString() + strings.Format(ConstantStringFormat, 1, 2, 3, 4, mField)))).

I'm only interested in the "abc" since it would be the "ldstr" instruction. But, it isn't explicitly stored inside a variable and it's eventually "passed" to Strings.Ignore, so I'd ignore it.

Sorry about the confusion. I'd be happy to provide whatever is needed.

Answer 9

Here's something that should get you started.  A couple of things to note:

1.  I emphasized clarity over performance since the goal is to help you understand how the sample works.  You may need to do quite a bit of reworking for performance reasons once you have the rule logic working as you like.
2.  I excluded writing directly to compiler-generated locals in the extraction of "interesting" instructions, but writing to elements of compiler-generated arrays hasn't been done yet.  If you can't get that working, give a shout.
3.  There are probably some other categories of instructions that need exclusion from the "interesting" set.  Unfortunately, since your rule doesn't make much sense to me, I'm not having much luck identifying what those might be... ;)

		private IList<string> IgnoredCalls { get; set; }

		public override void BeforeAnalysis()
		{
			this.IgnoredCalls = new List<string>()
			  {
				"YourNamespace.Strings.Ignore(System.String)",
			  };
		}

		public override ProblemCollection Check(Member member)
		{
			Method method = member as Method;
			if (method != null)
			{
				var ldstrInstructions = from i in method.Instructions
										where i.OpCode == OpCode.Ldstr
										select i;

				if (ldstrInstructions.Any())
				{
					var interestingInstructions = from i in method.Instructions
												  where this.IsInteresting(i)
												  select i;

					var candidateInstructions = (from i in ldstrInstructions
												 select new CandidateInstruction(i, interestingInstructions)).ToList();

					var ignoredMethodCalls = from i in interestingInstructions
											 where OpCodeHelper.IsCall(i.OpCode) && this.IsIgnoredCall(i)
											 select i;
					if (ignoredMethodCalls.Any())
					{
						this.HandleIgnoredCalls(candidateInstructions);
					}

					this.VisitUnhandledInstructions(candidateInstructions, instruction =>
					{
						this.AddProblem(instruction);
					});
				}
			}

			return this.Problems;
		}

		private bool IsInteresting(Instruction i)
		{
			bool result;
			if ((i.OpCode == OpCode.Nop) || (i.OpCode == OpCode.Box))
			{
				result = false;
			}
			else if (i.OpCode.ToString().StartsWith("Ld", StringComparison.Ordinal))
			{
				result = false;
			}
			else if (i.OpCode.ToString().StartsWith("Conv", StringComparison.Ordinal))
			{
				result = false;
			}
			else if (i.OpCode.ToString().StartsWith("Stloc", StringComparison.Ordinal))
			{
				result = !RuleUtilities.IsCompilerGenerated((Local)i.Value);
			}
			else
			{
				result = true;
			}

			return result;
		}

		private bool IsIgnoredCall(Instruction instruction)
		{
			return this.IgnoredCalls.Contains(((Method)instruction.Value).FullName, StringComparer.Ordinal);
		}

		private void HandleIgnoredCalls(IList<CandidateInstruction> candidateInstructions)
		{
			this.VisitUnhandledInstructions(candidateInstructions, instruction =>
			{
				foreach (Instruction subsequentInstruction in instruction.SubsequentInstructions)
				{
					if (OpCodeHelper.IsCall(subsequentInstruction.OpCode))
					{
						if (this.IsIgnoredCall(subsequentInstruction))
						{
							instruction.Handled = true;
						}
					}
					else
					{
						break;
					}
				}
			});
		}

		private void VisitUnhandledInstructions(IList<CandidateInstruction> candidateInstructions, Action<CandidateInstruction> action)
		{
			foreach (CandidateInstruction instruction in candidateInstructions)
			{
				if (!instruction.Handled)
				{
					action(instruction);
				}
			}
		}

		private void AddProblem(CandidateInstruction instruction)
		{
			this.Problems.Add(new Problem(this.GetResolution(instruction.Instruction.Value), instruction.Instruction));
			instruction.Handled = true;
		}

		private sealed class CandidateInstruction
		{
			private readonly Instruction _instruction;
			private readonly IEnumerable<Instruction> _subsequentInstructions;

			internal CandidateInstruction(Instruction instruction, IEnumerable<Instruction> allInstructions)
			{
				this._instruction = instruction;
				this._subsequentInstructions = from i in allInstructions
											   where i.Offset > instruction.Offset
											   select i;
			}

			internal Instruction Instruction
			{
				get
				{
					return this._instruction;
				}
			}

			internal IEnumerable<Instruction> SubsequentInstructions
			{
				get
				{
					return this._subsequentInstructions;
				}
			}

			internal bool Handled { get; set; }
		}

		private static class OpCodeHelper
		{
			internal static bool IsAssignment(OpCode opCode)
			{
				return opCode.ToString().StartsWith("St", StringComparison.Ordinal);
			}

			internal static bool IsCall(OpCode opCode)
			{
				return opCode.ToString().StartsWith("Call", StringComparison.Ordinal);
			}
		}

Answer 10

Hello,

I wasn't able to respond over the weekend, and these past two days were kind of hectic. I got a chance to test the above code, however, it didn't pass all of my tests (it passed way more than my code did). However, Nicole did say that it would get me started (so I'm not going to "Unmark" it as an answer). I'm going to tweak this code, or possibly take pieces of it, and then post it here just incase anyone else would need help with this.

Just FYI, this is for Globalization of my project. I use FxCop to detect all literal strings so that I can put them inside a .resx (not including those on a control, because there is that "Localizable" designer property) if they're shown to the user, OR ignore them if they shouldn't be translated. Strings that are used with the resource manager, or strings that are used with dictionaries/hashtables, or strings used with XML objects don't need to be translated. But strings in message boxes, or in exception handlers, or dynamically setting the text of controls do need to be translated. When I first ran FxCop to detect all the literal strings, it reported about ~16000 strings that were just "ldstr" instructions. I needed a way to ignore some of those (because I didn't really care).

Like I said, I'm going to tweak this code, or start from scratch using the code as a base. I'll post what I come up with. Thanks for all of the help!

Answer 11

Here is what I've come up with. This has passed all of my tests so far. I'm not sure if anyone would need this. Thanks again for everything!

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.FxCop.Sdk;
using System.IO;
using System.Reflection;

// http://msdn.microsoft.com/en-us/library/system.reflection.emit.opcodes.stsfld.aspx

namespace Localization
{
  public class StringLiteralsShouldBeInResxOrIgnored : BaseIntrospectionRule
  {
    private int mID;
    private TypeNode mSystemVoid;
    private TypeNode mString;

    public StringLiteralsShouldBeInResxOrIgnored() :
      base("StringLiteralsShouldBeInResxOrIgnored",
      "Localization.LocalizationFile",
      typeof(StringLiteralsShouldBeInResxOrIgnored).Assembly)
    {

    }

    public override void BeforeAnalysis()
    {
      mID = 0;
      mSystemVoid = FrameworkAssemblies.Mscorlib.GetType(Identifier.For("System"), Identifier.For("Void"));
      mString = FrameworkAssemblies.Mscorlib.GetType(Identifier.For("System"), Identifier.For("String"));
    }

    public override ProblemCollection Check(Member member)
    {
      Method method = member as Method;
      if (method != null)
      {
        List<Instruction> instructions = method.Instructions.ToList();
        for (int i = 0; i < instructions.Count; ++i)
        {
          Instruction instruction = instructions[i];
          if (instruction.OpCode == OpCode.Ldstr)
          {
            // create a variable to say if we should ignore this call
            bool ignore = false;

            // we've just loaded a string. Make sure that the instructions that follow
            // are acceptable.
            for (int j = i + 1; !ignore && j < instructions.Count; ++j)
            {
              instruction = instructions[j];
              if (FxCopHelper.IsCalling(instruction.OpCode))
              {
                // if this is a calling instruction, then see if it's
                // the one we need.
                Method function = instruction.Value as Method;
                if (function != null)
                {
                  // get the full name of the function
                  string name = FxCopHelper.GetMethodName(function);
                  if (name.Contains("Strings.Ignore") || name.Contains("System.Diagnostics")) // TODO: Add a list/array to hold the methods
                  {
                    // we loaded a string and eventually passed it to a special method
                    ignore = true;
                  }
                  else if (function.ReturnType.Equals(mSystemVoid))
                  {
                    // the function returns void (which means this is breaking).
                    break;
                  }
                }
              }
              else if (!IsNonBreaking(instruction))
              {
                // the instruction isn't non-breaking (which means it is breaking).
                // so we break
                break;
              }
            }

            // if we're not supposed to ignore this, then add a problem
            if (!ignore)
            {
              // add a new problem
              this.Problems.Add(new Problem(GetResolution(instructions[i].Value), mID.ToString()));

              // increment the ID
              ++mID;
            }
          }
        }
      }

      return this.Problems;
    }

    private bool IsNonBreaking(Instruction instruction)
    {
      bool retval = false;
      switch (instruction.OpCode)
      {
        case OpCode.Box:
        case OpCode.Dup:
        case OpCode.Newarr:
        case OpCode.Newobj:
        case OpCode.Pop:
        case OpCode.Starg:
        case OpCode.Starg_S:
        case OpCode.Stelem_Ref:
          {
            retval = true;
            break;
          }
        case OpCode.Stelem:
          {
            retval = mString.Equals(instruction.Value);
            break;
          }
        case OpCode.Stloc:
        case OpCode.Stloc_0:
        case OpCode.Stloc_1:
        case OpCode.Stloc_2:
        case OpCode.Stloc_3:
        case OpCode.Stloc_S:
          {
            // get the local variable associated with this
            Local local = instruction.Value as Local;
            if (RuleUtilities.IsCompilerGenerated(local))
            {
              // if this is a compilier generated local, then we don't care. If it's compilier generated,
              // that means it isn't how it appears in code, so this store instruction is non breaking
              retval = true;
            }
            else
            {
              // this isn't compilier generated. As long as we're not storing to a local variable that's a string
              // then we're okay
              retval = !mString.Equals(local.Type);
            }
            break;
          }
      }

      // or the value with a load
      retval |= FxCopHelper.IsLoading(instruction.OpCode);

      // or the value with a convert
      retval |= FxCopHelper.IsConvert(instruction.OpCode);

      // return the retval
      return retval;
    }
  }
}

And here is the code for the FxCopHelper class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.FxCop.Sdk;

namespace Localization
{
  public static class FxCopHelper
  {
    public static string GetMethodName(Method method)
    {
      string retval = string.Empty;
      if (method != null)
      {
        retval = method.ToString();
      }
      return retval;
    }

    public static bool IsLoading(OpCode opCode)
    {
      return opCode.ToString().ToLower().StartsWith("ld");
    }

    public static bool IsCalling(OpCode opCode)
    {
      return opCode.ToString().ToLower().StartsWith("call");
    }

    public static bool IsGuid(string text)
    {
      bool retval = true;
      try { new Guid(text); }
      catch { retval = false; }
      return retval;
    }

    public static bool IsConvert(OpCode opCode)
    {
      return opCode.ToString().ToLower().StartsWith("conv");
    }
  }
}