search

aalhour / C-Sharp-Algorithms

:books: :chart_with_upwards_trend: Plug-and-play class-library project of standard Data Structures and Algorithms in C#
MIT License
5.96k stars 1.4k forks source link

Primes List Should Be Immutable When Exposed to Callers #129

Closed VaslD closed 4 years ago

VaslD commented 4 years ago

Describe the bug

In PrimesList.GetAll() the underlying list of primes is returned as a mutable List<int> which makes it susceptible to modifications outside the library’s control.

Which in turn may interfere with (change behaviors of) methods called later because they rely heavily on the predefined list.

Expected behavior

Although it’s a developer’s job to pay attention and never modify library resources, the library itself should use some protection to make exposed resources secure.

List<T>.AsReadOnly() is the best option to return a read-only wrapper over the original list. It’s O(1) (no-copy) and syncs with the original list when modified. The return type will have to be changed to either IList<int> or IReadOnlyCollection<int> (from currently List<int>).

Notes

There may be other occurrences that a private/internal collection is returned without additional consideration. I just happen to bump into the one in PrimesList. If said collections are not meant to be modified outside the library, they should all be wrapped under a read only collection to prevent contamination.

github-actions[bot] commented 4 years ago

Thanks for supporting the development of C# Algorithms with your first issue! We look forward to handling it.

RyanGrange commented 4 years ago

Created pull https://github.com/aalhour/C-Sharp-Algorithms/pull/134 to return primes list as a read-only IList. Added a unit test to verify the list returned by GetAll can't be altered as-is. The readonly attribute on the underlying List only protected the List reference, not any of the list's member values.