Conversation
SharpHDiffPatch -> SharpHPatchZ
Attempt to implement a thread-safe Merged Stream Wrapper for Read/Write operations. Warning: Most of the Code for the reader are vibe-coded. So, need to check it further.
+ Struct type adjustment inside DirectoryPatchMetadata
+ Use UnmanagedArray<T> for some input-output ref lists instead of a direct array pointer
+ Fix CreateUnmanagedInt64List and CreateUnmanagedInt64As32List reading on backed numbers
+ Add extern functions:
- shpz_get_last_errorA
- shpz_get_last_errorW
- shpz_patch_from_filepath
- shpz_patch_from_FILE
+ Make some string arguments in extern functions auto-detect the encoding of the string
- shpz_read_header_signature_string
- shpz_init_from_filepath
- shpz_patch_from_filepath
+ Make the ExceptionHelper throw functions returns the Exception instead of throwing within
+ Add implicit cast for Utf16UnmanagedString and NativeStringW
+ Remove unnecessary type size check on UnmanagedArray<T>.GetSpan()
+ Use numbered error code on ExceptionHelper instead of flags
+ Add callback allocation check on ProgressCallback to avoit ExecutionEngineException due to attempt on calling null callback.
Use .NET Framework 4.7.2 for testing the lowest version of .NET to be supported
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| #if NET6_0_OR_GREATER | ||
| copyOverTcs.SetCanceled(token); | ||
| #else | ||
| copyOverTcs.SetCanceled(); | ||
| #endif |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
+ Also use native memory buffer on .NET 6 or above
| long newIndexNumber = -1; | ||
| for (int i = 0; i < count; i++) | ||
| { | ||
| oldIndexNumber += 1 + await reader.ReadLong7BitAsync(token); | ||
| newIndexNumber += 1 + await reader.ReadLong7BitAsync(token); | ||
| array[i] = new FileIndexPair | ||
| { | ||
| OldIndex = (int)oldIndexNumber, | ||
| NewIndex = (int)newIndexNumber | ||
| }; | ||
| } |
There was a problem hiding this comment.
Bug: The async implementation for reading index pairs is incompatible with the sync version. It reads data in the wrong order and incorrectly handles the signed delta encoding, causing corrupted patch metadata.
Severity: CRITICAL
Suggested Fix
The async implementation in CreateUnmanagedIndexPairListAsync must be updated to match the logic of the synchronous version. This includes reading newIndex before oldIndex and correctly implementing the signed delta decoding for oldIndex by checking the tag bit, as is done in the synchronous path.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/Extension/StreamExtension.cs#L302-L312
Potential issue: The asynchronous method for reading file index pairs is inconsistent
with its synchronous counterpart. The async version reads `oldIndex` and `newIndex`
values in a different order and, more importantly, fails to account for the signed delta
encoding used for `oldIndex`. The synchronous implementation correctly handles this
encoding by checking a tag bit for the sign. This discrepancy will lead to corrupted
metadata when creating directory patches via the async path, causing the patcher to
reference incorrect files, which can result in silent data corruption or out-of-bounds
access errors.
Also affects:
SharpHPatchZ/Extension/StreamExtension.cs:337~353
| return -1; | ||
| } | ||
|
|
||
| splitFirst = splitFirst.Slice(1, message.Length - 2); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| public static InvalidOperationException ThrowHDiffPatchPathNotAFile(string path) | ||
| => new($"[{Const.HDiffPatchPathNotAFile}] Path is not a file!: {path}"); | ||
| public static InvalidOperationException ThrowHDiffPatchInputFilesMismatched(long existingSize, long expectingSize) | ||
| => new($"[{Const.HDiffPatchInputFilesMismatched}] Input file size mismatched! Expecting: {expectingSize} bytes but got: {expectingSize} bytes instead."); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| while (totalRead < minimumBytes) | ||
| { | ||
| int read = stream.Read(buffer, totalRead, count - totalRead); | ||
| if (read == 0) |
There was a problem hiding this comment.
Bug: In ReadAtLeast, totalRead is incorrectly initialized to offset instead of 0, causing incorrect behavior when reading from a stream with a non-zero offset on netstandard2.0.
Severity: MEDIUM
Suggested Fix
Initialize totalRead to 0. The call to stream.Read should use offset + totalRead for the buffer position to ensure data is written to the correct location.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/Extension/StreamExtension.cs#L65
Potential issue: In the pre-NET6.0 implementation of `ReadAtLeast`, the `totalRead`
variable, which tracks the number of bytes read, is incorrectly initialized to the
`offset` parameter instead of 0. When `ReadExactly` is called with a non-zero `offset`,
this causes the loop condition `while (totalRead < minimumBytes)` to evaluate
incorrectly, potentially causing the read loop to terminate prematurely or not execute
at all. While current internal usage passes an offset of 0, this is a latent bug in the
public API for the `netstandard2.0` target framework that would affect external
consumers.
| if (encodedLength == long.MaxValue) | ||
| { | ||
| throw new InvalidDataException("An RLE run length exceeds the supported range."); | ||
| } |
There was a problem hiding this comment.
Bug: The RLE decoder in EnsureRun checks for long.MaxValue to detect an overflow, but the ReadLong7BitCore method it calls returns 0 on overflow, causing the check to fail.
Severity: MEDIUM
Suggested Fix
Update the overflow check in RleDecoder.EnsureRun to compare the encodedLength against 0 instead of long.MaxValue to correctly handle the sentinel value returned on overflow.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/Patch/HDiff13DerivedPatcher.CorePatcher.cs#L653-L656
Potential issue: The `ReadLong7BitCore` method returns `0` as a sentinel value to
indicate an integer overflow during decoding. However, its caller,
`RleDecoder.EnsureRun`, incorrectly checks for `long.MaxValue` to detect this overflow.
This mismatch means that if a malformed patch file triggers an overflow, the error is
not caught. The code then proceeds with an `encodedLength` of `0`, causing the RLE
decoder to process an incorrect number of bytes. This can lead to data corruption in the
output before a later validation step eventually fails.
+ Reenable PrefetchedReadStream for Zstd
| { | ||
| kvp.Value.Value.Dispose(); | ||
| } | ||
| } | ||
| _fileStreams.Clear(); | ||
| } | ||
| finally | ||
| { | ||
| _lifetimeLock.ExitWriteLock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The Dispose() method in RandomMergedStreamWrapper does not dispose its _lifetimeLock field, causing a ReaderWriterLockSlim kernel resource leak on every patch operation.
Severity: HIGH
Suggested Fix
In the Dispose() method of RandomMergedStreamWrapper, call _lifetimeLock.Dispose() to ensure the underlying kernel resources held by the ReaderWriterLockSlim are properly released. This should be done within a finally block after the write lock is released.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/IO/Reader/RandomMergedStreamWrapper.cs#L139-L162
Potential issue: The `Dispose()` method in the `RandomMergedStreamWrapper` class fails
to call `Dispose()` on its `_lifetimeLock` field, which is a `ReaderWriterLockSlim`
instance. Since `ReaderWriterLockSlim` holds underlying kernel resources, this omission
results in a resource leak. Each time a `RandomMergedStreamWrapper` is disposed, a
kernel handle is leaked. In long-running applications or batch processes that perform
many patching operations, this accumulation of leaked handles can exhaust system limits,
leading to an `OutOfResourceException` and application failure.
| { | ||
| _inputPosition += _outWindow.CopyStream(_inputStream, toProcess); | ||
| } | ||
| else if (_decoder.Code(_dictionarySize, _outWindow, _rangeDecoder) && _outputSize < 0) | ||
| else if (_decoder != null && _decoder.Code(_dictionarySize, _outWindow, _rangeDecoder) && _outputSize < 0) | ||
| { |
There was a problem hiding this comment.
Bug: A new null check in LzmaInputStream can mask errors from malformed streams, causing silent data corruption instead of throwing an exception.
Severity: MEDIUM
Suggested Fix
Remove the _decoder != null check from the _decoder.Code(...) call. The existing logic should already guarantee that _decoder is not null for valid streams, and throwing a NullReferenceException for malformed streams is the correct behavior to prevent silent data corruption.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/IO/Compression/Lzma/LzmaInputStream.cs#L163-L167
Potential issue: In `LzmaInputStream`, a null check was added before calling
`_decoder.Code(...)`. For well-formed LZMA2 streams, the `_decoder` is guaranteed to be
initialized. However, if a malformed or corrupted stream is processed, this new check
will cause the code to silently skip the decoding step instead of throwing an exception
as it would have previously. This behavior masks the data corruption error and can lead
to the generation of silently corrupted output, which is a more severe issue than an
explicit failure.
| => Task.Factory.StartNew(_ => StartPatch(inputPath, outputPath, token), | ||
| token, | ||
| TaskCreationOptions.LongRunning); |
There was a problem hiding this comment.
Bug: Task.Factory.StartNew is called with the CancellationToken passed as the state object, not as a cancellation token, and without specifying a TaskScheduler.
Severity: HIGH
Suggested Fix
Update the Task.Factory.StartNew call to use the correct overload that accepts a CancellationToken and explicitly specifies TaskScheduler.Default. This ensures the task runs on a background thread and correctly handles pre-cancellation. For example: Task.Factory.StartNew(() => StartPatch(inputPath, outputPath, token), token, TaskCreationOptions.LongRunning, TaskScheduler.Default);.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: SharpHPatchZ/Patch/HDiff13DerivedPatcher.cs#L102-L104
Potential issue: The `StartPatchAsync` method uses an incorrect overload of
`Task.Factory.StartNew`. The `CancellationToken` is passed as the `state` object instead
of being registered with the task factory. This bypasses pre-cancellation checks,
causing the task to be scheduled even if the token is already cancelled. Additionally,
no `TaskScheduler` is specified, so the task defaults to `TaskScheduler.Current`. If
this method is called from a UI thread, the long-running patch operation will execute on
that thread, freezing the user interface.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
For compatibility testing. As per Microsoft documentation, .NET Standard should support down to .NET Framework 4.6.1 https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0
+ HPatch.TryGetHDiff13PatchMetadata (Extern: shpz_util_get_hdiff13_patch_metadata) + HPatch.TryGetHDiff19DirectoryPatchMetadata (Extern: shpz_util_get_hdiff19_patch_metadata)
1 participant
TODO: Description
Benchmark Result
Test: Single file, LZMA2 compressed
Test: Directory, LZMA2 compressed
Test: Single file, LZMA2 compressed - Native (Result: 25 ms)
Test: Directory, LZMA compressed - Native (Result: 4.906 s)