Fix TOCTOU race condition in XLiffBody.AddTransUnitRaw

CHANGELOG.md

@@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [L10NSharp.Windows.Forms] Restored project-local Resources support for `FallbackLanguagesDlgBase` button images (`Move`, `Move_up`, and `Move_down`).
 - [L10NSharp.Windows.Forms] Corrected resource manager base name to `L10NSharp.Windows.Forms.Properties.Resources`.
 - [L10NSharp.Windows.Forms.Tests] Corrected resource manager base name to `L10NSharp.Windows.Forms.Tests.Properties.Resources`.
+- [L10NSharp] Fixed race condition in `XLiffBody.AddTransUnitRaw` where two concurrent threads with the same translation-unit ID could both bypass the duplicate check and silently overwrite each other's entry. (#150)
+- [L10NSharp] Eliminated unnecessary cross-instance lock contention in `XLiffBody` by making `_transUnitIdLock` instance-level instead of static. (#150)
 - [L10NSharp] Fixed `FilenamesToAddToCache` yielding both the custom and installed XLIFF for the same language when `UseLanguageCodeFolders` is `true`, causing custom translations to be silently overwritten by installed ones. (#140)
 - [L10NSharp] Fixed `ExtractXliff` accumulating duplicate "Not found in static scan" notes on successive runs; the note is now replaced rather than appended, and removed when the string is subsequently found. (#113)
 - [L10NSharp] Fixed `LocalizationManager.GetString` silently falling back to English when called with a one-shot `IEnumerable<string>` for `preferredLanguageIds`; the sequence is now materialized before use. (#139)

src/L10NSharp/XLiffUtils/XLiffBody.cs

@@ -25,7 +25,7 @@ public class XLiffBody
 		// This is used when translation unit IDs are not found in the file (which seems to be
 		// the case with Lingobit XLiff files).
 		private int _transUnitId;
-		static SpinLock _transUnitIdLock;
+		SpinLock _transUnitIdLock;
 
 		private readonly ConcurrentDictionary<string, XLiffTransUnit> _transUnitDict =
 			new ConcurrentDictionary<string, XLiffTransUnit>();
@@ -152,19 +152,22 @@ internal bool AddTransUnitRaw(XLiffTransUnit tu)
 				key = tu.Id;
 				if (string.IsNullOrEmpty(key))
 				{
-					tu.Id = (System.Threading.Interlocked.Increment(ref _transUnitId)).ToString();
+					tu.Id = (++_transUnitId).ToString();
 					key = tu.Id;
 				}
+
+				// If a translation unit with the specified id already exists, then quit here.
+				// This check and the dictionary write must both happen inside the lock to avoid
+				// a TOCTOU race where two threads with the same ID both pass the check.
+				if (GetTransUnitForId(key) != null)
+					return false;
+				_transUnitDict[key] = tu;
 			}
 			finally
 			{
 				if (lockTaken) _transUnitIdLock.Exit(false);
 			}
 
-			// If a translation unit with the specified id already exists, then quit here.
-			if (GetTransUnitForId(tu.Id) != null)
-				return false;
-			_transUnitDict[key] = tu;
 			return true;
 		}
 		public bool AddTransUnit(XLiffTransUnit tu)
@@ -216,6 +219,8 @@ public void RemoveTransUnit(XLiffTransUnit tu)
 			if (tu == null || tu.Id == null)
 				return;
 
+			// No SpinLock needed: _transUnitIdLock serializes AddTransUnitRaw's check-then-add
+			// sequence; TryRemove is independently atomic.
 			_transUnitDict.TryRemove(tu.Id, out _);
 			TranslationsById.TryRemove(tu.Id, out _);
 		}