Skip to content

Commit 43bab3a

Browse files
committed
fix: prevent race condition in PowertoolsLoggingSerializer options initialization
BuildJsonSerializerOptions was assigning _jsonOptions before fully configuring it. A concurrent thread doing the lock-free null check in GetSerializerOptions() could see the partially-configured instance, use it for serialization (making JsonSerializerOptions read-only), causing AddConverters() to throw InvalidOperationException. Fix: - Build into a local variable and publish to _jsonOptions only after full config - Mark _jsonOptions as volatile for correct double-checked locking - ConfigureNamingPolicy now nulls _jsonOptions to force rebuild instead of mutating options that may already be read-only
1 parent 09d4da6 commit 43bab3a

1 file changed

Lines changed: 46 additions & 43 deletions

File tree

libraries/src/AWS.Lambda.Powertools.Logging/Serializers/PowertoolsLoggingSerializer.cs

Lines changed: 46 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ internal class PowertoolsLoggingSerializer
2020
{
2121
private JsonSerializerOptions _currentOptions;
2222
private LoggerOutputCase _currentOutputCase;
23-
private JsonSerializerOptions _jsonOptions;
23+
private volatile JsonSerializerOptions _jsonOptions;
2424
private readonly object _lock = new();
2525

2626
private readonly ConcurrentBag<JsonSerializerContext> _additionalContexts = new();
@@ -59,11 +59,9 @@ internal void ConfigureNamingPolicy(LoggerOutputCase loggerOutputCase)
5959
{
6060
_currentOutputCase = loggerOutputCase;
6161

62-
// Only rebuild options if they already exist
63-
if (_jsonOptions != null)
64-
{
65-
SetOutputCase();
66-
}
62+
// Force a full rebuild on next access instead of mutating existing options,
63+
// because JsonSerializerOptions becomes read-only after first serialization.
64+
_jsonOptions = null;
6765
}
6866
}
6967
}
@@ -175,27 +173,29 @@ private void BuildJsonSerializerOptions(JsonSerializerOptions options = null)
175173
{
176174
lock (_lock)
177175
{
178-
// Create a completely new options instance regardless
179-
_jsonOptions = new JsonSerializerOptions();
176+
// Build into a local variable so _jsonOptions is never visible in a partially-configured state.
177+
// Another thread doing a lock-free read in GetSerializerOptions() could see a non-null _jsonOptions,
178+
// use it for serialization (making it read-only), and then subsequent mutations here would throw.
179+
var newOptions = new JsonSerializerOptions();
180180

181181
// Copy any properties from the original options if provided
182182
if (options != null)
183183
{
184184
// Copy standard properties
185-
_jsonOptions.DefaultIgnoreCondition = options.DefaultIgnoreCondition;
186-
_jsonOptions.PropertyNameCaseInsensitive = options.PropertyNameCaseInsensitive;
187-
_jsonOptions.PropertyNamingPolicy = options.PropertyNamingPolicy;
188-
_jsonOptions.DictionaryKeyPolicy = options.DictionaryKeyPolicy;
189-
_jsonOptions.WriteIndented = options.WriteIndented;
190-
_jsonOptions.ReferenceHandler = options.ReferenceHandler;
191-
_jsonOptions.MaxDepth = options.MaxDepth;
192-
_jsonOptions.IgnoreReadOnlyFields = options.IgnoreReadOnlyFields;
193-
_jsonOptions.IgnoreReadOnlyProperties = options.IgnoreReadOnlyProperties;
194-
_jsonOptions.IncludeFields = options.IncludeFields;
195-
_jsonOptions.NumberHandling = options.NumberHandling;
196-
_jsonOptions.ReadCommentHandling = options.ReadCommentHandling;
197-
_jsonOptions.UnknownTypeHandling = options.UnknownTypeHandling;
198-
_jsonOptions.AllowTrailingCommas = options.AllowTrailingCommas;
185+
newOptions.DefaultIgnoreCondition = options.DefaultIgnoreCondition;
186+
newOptions.PropertyNameCaseInsensitive = options.PropertyNameCaseInsensitive;
187+
newOptions.PropertyNamingPolicy = options.PropertyNamingPolicy;
188+
newOptions.DictionaryKeyPolicy = options.DictionaryKeyPolicy;
189+
newOptions.WriteIndented = options.WriteIndented;
190+
newOptions.ReferenceHandler = options.ReferenceHandler;
191+
newOptions.MaxDepth = options.MaxDepth;
192+
newOptions.IgnoreReadOnlyFields = options.IgnoreReadOnlyFields;
193+
newOptions.IgnoreReadOnlyProperties = options.IgnoreReadOnlyProperties;
194+
newOptions.IncludeFields = options.IncludeFields;
195+
newOptions.NumberHandling = options.NumberHandling;
196+
newOptions.ReadCommentHandling = options.ReadCommentHandling;
197+
newOptions.UnknownTypeHandling = options.UnknownTypeHandling;
198+
newOptions.AllowTrailingCommas = options.AllowTrailingCommas;
199199

200200
// Handle type resolver extraction without setting it yet
201201
if (options.TypeInfoResolver != null)
@@ -211,49 +211,52 @@ private void BuildJsonSerializerOptions(JsonSerializerOptions options = null)
211211
}
212212

213213
// Set output case and other properties
214-
SetOutputCase();
215-
AddConverters();
216-
_jsonOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping;
217-
_jsonOptions.PropertyNameCaseInsensitive = true;
214+
SetOutputCase(newOptions);
215+
AddConverters(newOptions);
216+
newOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping;
217+
newOptions.PropertyNameCaseInsensitive = true;
218218

219219
// Set TypeInfoResolver last, as this makes options read-only
220220
if (!RuntimeFeatureWrapper.IsDynamicCodeSupported)
221221
{
222-
_jsonOptions.TypeInfoResolver = GetCompositeResolver();
222+
newOptions.TypeInfoResolver = GetCompositeResolver();
223223
}
224+
225+
// Publish fully-configured options in a single atomic assignment
226+
_jsonOptions = newOptions;
224227
}
225228
}
226229

227-
internal void SetOutputCase()
230+
internal void SetOutputCase(JsonSerializerOptions target)
228231
{
229232
switch (_currentOutputCase)
230233
{
231234
case LoggerOutputCase.CamelCase:
232-
_jsonOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
233-
_jsonOptions.DictionaryKeyPolicy = JsonNamingPolicy.CamelCase;
235+
target.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
236+
target.DictionaryKeyPolicy = JsonNamingPolicy.CamelCase;
234237
break;
235238
case LoggerOutputCase.PascalCase:
236-
_jsonOptions.PropertyNamingPolicy = PascalCaseNamingPolicy.Instance;
237-
_jsonOptions.DictionaryKeyPolicy = PascalCaseNamingPolicy.Instance;
239+
target.PropertyNamingPolicy = PascalCaseNamingPolicy.Instance;
240+
target.DictionaryKeyPolicy = PascalCaseNamingPolicy.Instance;
238241
break;
239242
default: // Snake case
240243
// If is default (Not Set) and JsonOptions provided with DictionaryKeyPolicy or PropertyNamingPolicy, use it
241-
_jsonOptions.DictionaryKeyPolicy ??= JsonNamingPolicy.SnakeCaseLower;
242-
_jsonOptions.PropertyNamingPolicy ??= JsonNamingPolicy.SnakeCaseLower;
244+
target.DictionaryKeyPolicy ??= JsonNamingPolicy.SnakeCaseLower;
245+
target.PropertyNamingPolicy ??= JsonNamingPolicy.SnakeCaseLower;
243246
break;
244247
}
245248
}
246249

247-
private void AddConverters()
250+
private static void AddConverters(JsonSerializerOptions target)
248251
{
249-
_jsonOptions.Converters.Add(new ByteArrayConverter());
250-
_jsonOptions.Converters.Add(new ExceptionConverter());
251-
_jsonOptions.Converters.Add(new MemoryStreamConverter());
252-
_jsonOptions.Converters.Add(new ConstantClassConverter());
253-
_jsonOptions.Converters.Add(new DateOnlyConverter());
254-
_jsonOptions.Converters.Add(new TimeOnlyConverter());
255-
256-
_jsonOptions.Converters.Add(new LogLevelJsonConverter());
252+
target.Converters.Add(new ByteArrayConverter());
253+
target.Converters.Add(new ExceptionConverter());
254+
target.Converters.Add(new MemoryStreamConverter());
255+
target.Converters.Add(new ConstantClassConverter());
256+
target.Converters.Add(new DateOnlyConverter());
257+
target.Converters.Add(new TimeOnlyConverter());
258+
259+
target.Converters.Add(new LogLevelJsonConverter());
257260
}
258261

259262
internal void SetOptions(JsonSerializerOptions options)

0 commit comments

Comments
 (0)