0

Consider this C# program:

using System;
using System.Collections.Generic;
using System.Linq;

namespace SandboxApplication
{
    public class IntsOwner
    {
        private List<int> _ints;

        public IntsOwner (IEnumerable<int> ints)
        {
            _ints = ints.OrderBy(i => i).ToList(); // They must be in the correct order
        }

        public IEnumerable<int> Ints
            => _ints;

        public void CheckFirstTwoInts ()
        {
            if (_ints.Count < 2)
            {
                Console.WriteLine("You need to collect some more ints before trying this.");
            }
            else if (_ints[0] <= _ints[1])
            {
                Console.WriteLine("Your ints are in the correct order and you should stop worrying.");
            }
            else
            {
                Console.WriteLine("You've failed, your highness.");
            }
        }
    }

    class Program
    {
        static void Main (string[] args)
        {
            var intsOwner = new IntsOwner(new List<int> {1, 2, 3, 4, 5});

            var ienumerable = intsOwner.Ints;

            var list = (List<int>)ienumerable;

            intsOwner.CheckFirstTwoInts();

            list[0] = 6;

            intsOwner.CheckFirstTwoInts();

            Console.ReadLine();
        }
    }
}

If you run this, you will get two lines of output:

Your ints are in the correct order and you should stop worrying.
You've failed, your highness.

The original designer of the IntsOwner class wanted to make sure that a particular property (ordering of list elements) held for the private member _ints. But because a reference to the actual object is returned through the Ints property, a user of the class can modify the object so that this property no longer holds.

This sort of code doesn't appear very likely in practice, but it is still disconcerting that control of members intended to be private can "leak" in this way. How much, if at all, should programmers try to block this sort of thing off? For example, would it be reasonable or proportionate to change the Ints property's expression body to _ints.Select(i = i) and thereby close off this way of modifying the private member? Or would that be unnecessarily paranoid to the detriment of the code's readability?

Hammerite
  • 21,755
  • 6
  • 70
  • 91
  • The reason that `_ints` can be modified even though it is private is because a _reference_ to it is passed out when the public property get is called. To protect the private list the solution would seem to be to return a copy of this through the property, but then how would valid changes be controlled? Not an easy (or even necessary) problem to solve. – Martin Aug 31 '16 at 21:09
  • 1
    I think I wouldn't worry about it. Casting to `List` is explicit use of "internal" information - it's one step away from just grabbing the private member via reflection. You're clearly exposing only read-only information via the public interface of the class. – Blorgbeard Aug 31 '16 at 21:09
  • Incidentally, with reflection in .Net we can access all members, methods, properties, etc. regardless of their `private` scope, which renders such considerations obsolete anyway. – Martin Aug 31 '16 at 21:10
  • 1
    You can only protect people so much. Your duty is to provide an interface for them to use. People can always use reflection to get around whatever you try to do. If they try casting your `IEnumerable` to a `List`, they're going to manage it if they're determined enough. – itsme86 Aug 31 '16 at 21:12
  • Are you sending this data to the UI (perhaps web application) and then get the ids from client to do something on the back-end? Basically, I want to clarify if you are asking about secure software coding practices. – Sparrow Aug 31 '16 at 21:20

2 Answers2

2

I always add a call to AsReadOnly() after ToList

Immutability is key, not just principal of least knowledge in the return type

https://msdn.microsoft.com/en-us/library/e78dcd75(v=vs.110).aspx

Jeff
  • 35,755
  • 15
  • 108
  • 220
-3

You could do something like 

public IEnumerable<int> Ints => _ints.ToList<int>();

so you're not returning the reference to _ints, but only a copied list. Anyone modifying the return value then would only be modifying their own copy of it, not the privately stored one.

D. Stewart
  • 471
  • 4
  • 15
  • OP knows this, he mentions the similar solution `_ints.Select(i = i)`. He is asking whether it's necessary / a good idea. – Blorgbeard Aug 31 '16 at 21:18