From 01c70371b03c74c57452d97a3a716764117abb54 Mon Sep 17 00:00:00 2001 From: CyrusNajmabadi Date: Thu, 8 Dec 2016 18:04:35 -0800 Subject: [PATCH] Add a version header to Object-Streams. This way we can tell, up front if the persistence version has changed, and if we can read the data properly or not. --- .../Portable/Syntax/CSharpSyntaxNode.cs | 8 +++- .../ObjectSerializationTests.cs | 19 +++++++-- .../CodeAnalysisResources.Designer.cs | 9 +++++ .../Core/Portable/CodeAnalysisResources.resx | 3 ++ .../Serialization/StreamObjectReader.cs | 40 +++++++++++++++++-- .../Serialization/StreamObjectWriter.cs | 8 ++++ .../Portable/Syntax/VisualBasicSyntaxNode.vb | 6 ++- .../TodoComment/TodoCommentState.cs | 36 ++++++----------- .../State/AbstractAnalyzerState.cs | 18 ++++++--- .../DesignerAttributeState.cs | 24 +++++------ .../VisualStudioDiagnosticAnalyzerExecutor.cs | 2 +- .../SemanticVersionTrackingService.cs | 38 ++++++++---------- .../CSharpSyntaxTreeFactoryService.cs | 4 +- .../Diagnostics/DiagnosticDataSerializer.cs | 12 +++--- .../SymbolTreeInfo_Serialization.cs | 18 ++++----- .../SyntaxTree/AbstractPersistableState.cs | 18 +++------ .../SnapshotService.JsonRpcAssetSource.cs | 2 +- 17 files changed, 158 insertions(+), 107 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs b/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs index b81d581b16a..829c2b0a3da 100644 --- a/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs +++ b/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs @@ -204,8 +204,14 @@ public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancel throw new InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom); } - using (var reader = new StreamObjectReader(stream, knownObjects: GetDeserializationObjectData(), binder: s_defaultBinder, cancellationToken: cancellationToken)) + + using (var reader = StreamObjectReader.TryGetReader(stream, knownObjects: GetDeserializationObjectData(), binder: s_defaultBinder, cancellationToken: cancellationToken)) { + if (reader == null) + { + throw new ArgumentException(CodeAnalysisResources.Stream_contains_invalid_data, nameof(stream)); + } + var root = (Syntax.InternalSyntax.CSharpSyntaxNode)reader.ReadValue(); return root.CreateRed(); } diff --git a/src/Compilers/Core/CodeAnalysisTest/ObjectSerializationTests.cs b/src/Compilers/Core/CodeAnalysisTest/ObjectSerializationTests.cs index dfb24ea1b0b..8c0c4d79b1d 100644 --- a/src/Compilers/Core/CodeAnalysisTest/ObjectSerializationTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/ObjectSerializationTests.cs @@ -12,6 +12,19 @@ namespace Microsoft.CodeAnalysis.UnitTests { public sealed class ObjectSerializationTests { + [Fact] + private void TestInvalidStreamVersion() + { + var stream = new MemoryStream(); + stream.WriteByte(0); + stream.WriteByte(0); + + stream.Position = 0; + + var reader = StreamObjectReader.TryGetReader(stream); + Assert.Null(reader); + } + private void RoundTrip(Action writeAction, Action readAction, bool recursive) { var stream = new MemoryStream(); @@ -25,7 +38,7 @@ private void RoundTrip(Action writeAction, Action re Assert.Equal(recursive, StreamObjectReader.IsRecursive(stream)); stream.Position = 0; - using (var reader = new StreamObjectReader(stream, binder: binder)) + using (var reader = StreamObjectReader.TryGetReader(stream, binder: binder)) { readAction(reader); } @@ -50,7 +63,7 @@ private T RoundTrip(T value, Action writeAction, Func + /// Looks up a localized string similar to Stream contains invalid data. + /// + internal static string Stream_contains_invalid_data { + get { + return ResourceManager.GetString("Stream_contains_invalid_data", resourceCulture); + } + } + /// /// Looks up a localized string similar to Stream is too long.. /// diff --git a/src/Compilers/Core/Portable/CodeAnalysisResources.resx b/src/Compilers/Core/Portable/CodeAnalysisResources.resx index dba6eb7f32d..efbb09b595a 100644 --- a/src/Compilers/Core/Portable/CodeAnalysisResources.resx +++ b/src/Compilers/Core/Portable/CodeAnalysisResources.resx @@ -588,4 +588,7 @@ Deserialization reader for '{0}' read incorrect number of values. + + Stream contains invalid data + \ No newline at end of file diff --git a/src/Compilers/Core/Portable/Serialization/StreamObjectReader.cs b/src/Compilers/Core/Portable/Serialization/StreamObjectReader.cs index 38fc7723b77..26886ac6492 100644 --- a/src/Compilers/Core/Portable/Serialization/StreamObjectReader.cs +++ b/src/Compilers/Core/Portable/Serialization/StreamObjectReader.cs @@ -27,6 +27,14 @@ namespace Roslyn.Utilities /// internal sealed partial class StreamObjectReader : ObjectReader, IDisposable { + /// + /// We start the version at something reasonably random. That way an older file, with + /// some random start-bytes, has little chance of matching our version. When incrementing + /// this version, just change VersionByte2. + /// + internal const byte VersionByte1 = 0b10101010; + internal const byte VersionByte2 = 0b00000001; + private readonly BinaryReader _reader; private readonly ObjectBinder _binder; private readonly bool _recursive; @@ -67,11 +75,11 @@ internal sealed partial class StreamObjectReader : ObjectReader, IDisposable /// An optional list of objects assumed known by the corresponding . /// A binder that provides object and type decoding. /// - public StreamObjectReader( + private StreamObjectReader( Stream stream, - ObjectData knownObjects = null, - ObjectBinder binder = null, - CancellationToken cancellationToken = default(CancellationToken)) + ObjectData knownObjects, + ObjectBinder binder, + CancellationToken cancellationToken) { // String serialization assumes both reader and writer to be of the same endianness. // It can be adjusted for BigEndian if needed. @@ -93,6 +101,30 @@ internal sealed partial class StreamObjectReader : ObjectReader, IDisposable } } + /// + /// Attempts to create a from the provided . + /// If the does not start with a valid header, then null will + /// be returned. + /// + public static StreamObjectReader TryGetReader( + Stream stream, + ObjectData knownObjects = null, + ObjectBinder binder = null, + CancellationToken cancellationToken = default(CancellationToken)) + { + if (stream == null) + { + return null; + } + + if (stream.ReadByte() != VersionByte1 && stream.ReadByte() != VersionByte2) + { + return null; + } + + return new StreamObjectReader(stream, knownObjects, binder, cancellationToken); + } + internal static bool IsRecursive(Stream stream) { var recursionKind = (EncodingKind)stream.ReadByte(); diff --git a/src/Compilers/Core/Portable/Serialization/StreamObjectWriter.cs b/src/Compilers/Core/Portable/Serialization/StreamObjectWriter.cs index a5c1dd04173..65b0b54db99 100644 --- a/src/Compilers/Core/Portable/Serialization/StreamObjectWriter.cs +++ b/src/Compilers/Core/Portable/Serialization/StreamObjectWriter.cs @@ -82,6 +82,8 @@ internal sealed partial class StreamObjectWriter : ObjectWriter, IDisposable _recursive = recursive; _cancellationToken = cancellationToken; + WriteVersion(); + if (_recursive) { _writer.Write((byte)EncodingKind.Recursive); @@ -95,6 +97,12 @@ internal sealed partial class StreamObjectWriter : ObjectWriter, IDisposable } } + private void WriteVersion() + { + _writer.Write(StreamObjectReader.VersionByte1); + _writer.Write(StreamObjectReader.VersionByte2); + } + public void Dispose() { _referenceMap.Dispose(); diff --git a/src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxNode.vb b/src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxNode.vb index 0d90dba495e..d5d6ed628a1 100644 --- a/src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxNode.vb +++ b/src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxNode.vb @@ -140,7 +140,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic Throw New InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom) End If - Using reader = New StreamObjectReader(stream, knownObjects:=GetDeserializationObjectData(), binder:=s_defaultBinder, cancellationToken:=cancellationToken) + Using reader = StreamObjectReader.TryGetReader(stream, knownObjects:=GetDeserializationObjectData(), binder:=s_defaultBinder, cancellationToken:=cancellationToken) + If reader Is Nothing Then + Throw New ArgumentException(CodeAnalysisResources.Stream_contains_invalid_data, NameOf(stream)) + End If + Return DirectCast(reader.ReadValue(), InternalSyntax.VisualBasicSyntaxNode).CreateRed(Nothing, 0) End Using End Function diff --git a/src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentState.cs b/src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentState.cs index 8748f747464..7e9433a02be 100644 --- a/src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentState.cs +++ b/src/EditorFeatures/Core/Implementation/TodoComment/TodoCommentState.cs @@ -1,7 +1,5 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.IO; using System.Threading; @@ -32,33 +30,25 @@ protected override int GetCount(Data data) protected override Data TryGetExistingData(Stream stream, Document value, CancellationToken cancellationToken) { - var list = SharedPools.Default>().AllocateAndClear(); - try + using (var reader = StreamObjectReader.TryGetReader(stream)) { - using (var reader = new StreamObjectReader(stream)) + if (reader != null) { var format = reader.ReadString(); - if (!string.Equals(format, FormatVersion)) + if (string.Equals(format, FormatVersion)) { - return null; - } - - var textVersion = VersionStamp.ReadFrom(reader); - var dataVersion = VersionStamp.ReadFrom(reader); + var textVersion = VersionStamp.ReadFrom(reader); + var dataVersion = VersionStamp.ReadFrom(reader); - AppendItems(reader, value, list, cancellationToken); + var list = ArrayBuilder.GetInstance(); + AppendItems(reader, value, list, cancellationToken); - return new Data(textVersion, dataVersion, list.ToImmutableArray()); + return new Data(textVersion, dataVersion, list.ToImmutableAndFree()); + } } } - catch (Exception) - { - return null; - } - finally - { - SharedPools.Default>().ClearAndFree(list); - } + + return null; } protected override void WriteTo(Stream stream, Data data, CancellationToken cancellationToken) @@ -104,7 +94,7 @@ public ImmutableArray GetItems_TestingOnly(DocumentId documentId) return ImmutableArray.Empty; } - private void AppendItems(ObjectReader reader, Document document, List list, CancellationToken cancellationToken) + private void AppendItems(ObjectReader reader, Document document, ArrayBuilder list, CancellationToken cancellationToken) { var count = reader.ReadInt32(); for (var i = 0; i < count; i++) @@ -130,4 +120,4 @@ private void AppendItems(ObjectReader reader, Document document, List } } } -} +} \ No newline at end of file diff --git a/src/Features/Core/Portable/SolutionCrawler/State/AbstractAnalyzerState.cs b/src/Features/Core/Portable/SolutionCrawler/State/AbstractAnalyzerState.cs index 70751a49e72..84cce6e9454 100644 --- a/src/Features/Core/Portable/SolutionCrawler/State/AbstractAnalyzerState.cs +++ b/src/Features/Core/Portable/SolutionCrawler/State/AbstractAnalyzerState.cs @@ -54,16 +54,22 @@ public async Task TryGetExistingDataAsync(TValue value, CancellationToken var solution = GetSolution(value); var persistService = solution.Workspace.Services.GetService(); - using (var storage = persistService.GetStorage(solution)) - using (var stream = await ReadStreamAsync(storage, value, cancellationToken).ConfigureAwait(false)) + try { - if (stream == null) + using (var storage = persistService.GetStorage(solution)) + using (var stream = await ReadStreamAsync(storage, value, cancellationToken).ConfigureAwait(false)) { - return default(TData); + if (stream != null) + { + return TryGetExistingData(stream, value, cancellationToken); + } } - - return TryGetExistingData(stream, value, cancellationToken); } + catch (IOException) + { + } + + return default(TData); } public async Task PersistAsync(TValue value, TData data, CancellationToken cancellationToken) diff --git a/src/VisualStudio/Core/Def/Implementation/DesignerAttribute/DesignerAttributeState.cs b/src/VisualStudio/Core/Def/Implementation/DesignerAttribute/DesignerAttributeState.cs index f2ff56e891c..2119d56af9f 100644 --- a/src/VisualStudio/Core/Def/Implementation/DesignerAttribute/DesignerAttributeState.cs +++ b/src/VisualStudio/Core/Def/Implementation/DesignerAttribute/DesignerAttributeState.cs @@ -31,27 +31,23 @@ protected override int GetCount(Data data) protected override Data TryGetExistingData(Stream stream, Document value, CancellationToken cancellationToken) { - try + using (var reader = StreamObjectReader.TryGetReader(stream)) { - using (var reader = new StreamObjectReader(stream)) + if (reader != null) { var format = reader.ReadString(); - if (!string.Equals(format, FormatVersion, StringComparison.InvariantCulture)) + if (string.Equals(format, FormatVersion, StringComparison.InvariantCulture)) { - return null; - } - - var textVersion = VersionStamp.ReadFrom(reader); - var dataVersion = VersionStamp.ReadFrom(reader); - var designerAttributeArgument = reader.ReadString(); + var textVersion = VersionStamp.ReadFrom(reader); + var dataVersion = VersionStamp.ReadFrom(reader); + var designerAttributeArgument = reader.ReadString(); - return new Data(textVersion, dataVersion, designerAttributeArgument); + return new Data(textVersion, dataVersion, designerAttributeArgument); + } } } - catch (Exception) - { - return null; - } + + return null; } protected override void WriteTo(Stream stream, Data data, CancellationToken cancellationToken) diff --git a/src/VisualStudio/Core/Def/Implementation/Diagnostics/VisualStudioDiagnosticAnalyzerExecutor.cs b/src/VisualStudio/Core/Def/Implementation/Diagnostics/VisualStudioDiagnosticAnalyzerExecutor.cs index 0f597529c54..e7f1bd8e9d1 100644 --- a/src/VisualStudio/Core/Def/Implementation/Diagnostics/VisualStudioDiagnosticAnalyzerExecutor.cs +++ b/src/VisualStudio/Core/Def/Implementation/Diagnostics/VisualStudioDiagnosticAnalyzerExecutor.cs @@ -139,7 +139,7 @@ private CompilationWithAnalyzers CreateAnalyzerDriver(CompilationWithAnalyzers a // handling of cancellation and exception var version = await DiagnosticIncrementalAnalyzer.GetDiagnosticVersionAsync(project, cancellationToken).ConfigureAwait(false); - using (var reader = new StreamObjectReader(stream)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { return DiagnosticResultSerializer.Deserialize(reader, analyzerMap, project, version, cancellationToken); } diff --git a/src/VisualStudio/Core/Def/Implementation/Versions/SemanticVersionTrackingService.cs b/src/VisualStudio/Core/Def/Implementation/Versions/SemanticVersionTrackingService.cs index 08e21af669b..4061571a123 100644 --- a/src/VisualStudio/Core/Def/Implementation/Versions/SemanticVersionTrackingService.cs +++ b/src/VisualStudio/Core/Def/Implementation/Versions/SemanticVersionTrackingService.cs @@ -2,6 +2,7 @@ using System; using System.Composition; +using System.IO; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -121,36 +122,31 @@ private static bool TryReadFrom(Project project, string keyName, out Versions ve return false; } - using (var storage = service.GetStorage(project.Solution)) - using (var stream = storage.ReadStreamAsync(keyName, CancellationToken.None).WaitAndGetResult(CancellationToken.None)) + try { - if (stream == null) - { - return false; - } - - try + using (var storage = service.GetStorage(project.Solution)) + using (var stream = storage.ReadStreamAsync(keyName, CancellationToken.None).WaitAndGetResult(CancellationToken.None)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { - using (var reader = new StreamObjectReader(stream)) + if (reader != null) { var formatVersion = reader.ReadInt32(); - if (formatVersion != SerializationFormat) + if (formatVersion == SerializationFormat) { - return false; - } - - var persistedProjectVersion = VersionStamp.ReadFrom(reader); - var persistedSemanticVersion = VersionStamp.ReadFrom(reader); + var persistedProjectVersion = VersionStamp.ReadFrom(reader); + var persistedSemanticVersion = VersionStamp.ReadFrom(reader); - versions = new Versions(persistedProjectVersion, persistedSemanticVersion); - return true; + versions = new Versions(persistedProjectVersion, persistedSemanticVersion); + return true; + } } } - catch (Exception) - { - return false; - } } + catch (IOException) + { + } + + return false; } public async Task RecordSemanticVersionsAsync(Project project, CancellationToken cancellationToken) diff --git a/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxTreeFactoryService.cs b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxTreeFactoryService.cs index 5282a6e960d..6be6380b0ea 100644 --- a/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxTreeFactoryService.cs +++ b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpSyntaxTreeFactoryService.cs @@ -44,9 +44,7 @@ public override SyntaxTree ParseSyntaxTree(string fileName, ParseOptions options } public override SyntaxNode DeserializeNodeFrom(Stream stream, CancellationToken cancellationToken) - { - return CSharpSyntaxNode.DeserializeFrom(stream, cancellationToken); - } + => CSharpSyntaxNode.DeserializeFrom(stream, cancellationToken); public override bool CanCreateRecoverableTree(SyntaxNode root) { diff --git a/src/Workspaces/Core/Portable/Diagnostics/DiagnosticDataSerializer.cs b/src/Workspaces/Core/Portable/Diagnostics/DiagnosticDataSerializer.cs index d3d7a295697..07c46143a2c 100644 --- a/src/Workspaces/Core/Portable/Diagnostics/DiagnosticDataSerializer.cs +++ b/src/Workspaces/Core/Portable/Diagnostics/DiagnosticDataSerializer.cs @@ -62,18 +62,16 @@ public async Task>> DeserializeAsync(ob using (var storage = persistService.GetStorage(solution)) using (var stream = await ReadStreamAsync(storage, key, documentOrProject, cancellationToken).ConfigureAwait(false)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { - if (stream == null) + if (reader == null) { return null; } - using (var reader = new StreamObjectReader(stream)) - { - // we return StrongBox rather than ImmutableArray due to task lib's issue with allocations - // when returning default(value type) - return ReadFrom(reader, documentOrProject, cancellationToken); - } + // we return StrongBox rather than ImmutableArray due to task lib's issue with allocations + // when returning default(value type) + return ReadFrom(reader, documentOrProject, cancellationToken); } } diff --git a/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Serialization.cs b/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Serialization.cs index 1830273d92f..ba73dc2536b 100644 --- a/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Serialization.cs +++ b/src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Serialization.cs @@ -95,19 +95,17 @@ internal partial class SymbolTreeInfo // Get the unique key to identify our data. var key = PrefixMetadataSymbolTreeInfo + prefix + keySuffix; using (var stream = await storage.ReadStreamAsync(key, cancellationToken).ConfigureAwait(false)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { - if (stream != null) + if (reader != null) { - using (var reader = new StreamObjectReader(stream)) + // We have some previously persisted data. Attempt to read it back. + // If we're able to, and the version of the persisted data matches + // our version, then we can reuse this instance. + result = readObject(reader); + if (result != null && VersionStamp.CanReusePersistedVersion(version, getVersion(result))) { - // We have some previously persisted data. Attempt to read it back. - // If we're able to, and the version of the persisted data matches - // our version, then we can reuse this instance. - result = readObject(reader); - if (result != null && VersionStamp.CanReusePersistedVersion(version, getVersion(result))) - { - return result; - } + return result; } } } diff --git a/src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/AbstractPersistableState.cs b/src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/AbstractPersistableState.cs index 82b86409d8e..e2da1415b71 100644 --- a/src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/AbstractPersistableState.cs +++ b/src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/AbstractPersistableState.cs @@ -47,13 +47,9 @@ private static bool TryReadVersion(ObjectReader reader, string formatVersion, ou // attempt to load from persisted state using (var storage = persistentStorageService.GetStorage(document.Project.Solution)) using (var stream = await storage.ReadStreamAsync(document, persistenceName, cancellationToken).ConfigureAwait(false)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { - if (stream == null) - { - return null; - } - - using (var reader = new StreamObjectReader(stream)) + if (reader != null) { if (TryReadVersion(reader, formatVersion, out var persistVersion) && document.CanReusePersistedSyntaxTreeVersion(syntaxVersion, persistVersion)) @@ -96,14 +92,12 @@ protected static async Task PrecalculatedAsync(Document document, string p // check whether we already have info for this document using (var storage = persistentStorageService.GetStorage(document.Project.Solution)) using (var stream = await storage.ReadStreamAsync(document, persistenceName, cancellationToken).ConfigureAwait(false)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { - if (stream != null) + if (reader != null) { - using (var reader = new StreamObjectReader(stream)) - { - return TryReadVersion(reader, formatVersion, out var persistVersion) && - document.CanReusePersistedSyntaxTreeVersion(syntaxVersion, persistVersion); - } + return TryReadVersion(reader, formatVersion, out var persistVersion) && + document.CanReusePersistedSyntaxTreeVersion(syntaxVersion, persistVersion); } } diff --git a/src/Workspaces/Remote/ServiceHub/Services/SnapshotService.JsonRpcAssetSource.cs b/src/Workspaces/Remote/ServiceHub/Services/SnapshotService.JsonRpcAssetSource.cs index 9dde7dc1e04..cdcfdfeac8e 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/SnapshotService.JsonRpcAssetSource.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/SnapshotService.JsonRpcAssetSource.cs @@ -55,7 +55,7 @@ private class JsonRpcAssetSource : AssetSource { var results = new List>(); - using (var reader = new StreamObjectReader(stream)) + using (var reader = StreamObjectReader.TryGetReader(stream)) { var responseSessionId = reader.ReadInt32(); Contract.ThrowIfFalse(sessionId == responseSessionId); -- GitLab