-
-
Notifications
You must be signed in to change notification settings - Fork 366
refactor: Migrate SentryMsgPackSerializer from Objective-C to Swift #6143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a121c88
6eb1e43
3021956
810f132
b73f131
8d45ba3
9b91dac
7726ae1
2307fe1
5181e9e
568d287
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
@testable import Sentry | ||
|
||
private class ErrorInputStream: InputStream { | ||
override var hasBytesAvailable: Bool { | ||
return true | ||
} | ||
|
||
override func read(_ buffer: UnsafeMutablePointer<UInt8>, maxLength len: Int) -> Int { | ||
return -1 // Simulate read error | ||
} | ||
|
||
override func open() { | ||
// No-op | ||
} | ||
|
||
override func close() { | ||
// No-op | ||
} | ||
} | ||
|
||
public class TestStreamableObject: NSObject, SentryStreamable { | ||
|
||
private let shouldReturnNilInputStream: Bool | ||
private let streamSizeValue: UInt? | ||
private let shouldReturnErrorStream: Bool | ||
|
||
public init(streamSize: UInt?, shouldReturnNilInputStream: Bool, shouldReturnErrorStream: Bool = false) { | ||
self.streamSizeValue = streamSize | ||
self.shouldReturnNilInputStream = shouldReturnNilInputStream | ||
self.shouldReturnErrorStream = shouldReturnErrorStream | ||
super.init() | ||
} | ||
|
||
public func asInputStream() -> InputStream? { | ||
if shouldReturnNilInputStream { | ||
return nil | ||
} | ||
if shouldReturnErrorStream { | ||
return ErrorInputStream() | ||
} | ||
return InputStream(data: Data()) | ||
} | ||
|
||
public func streamSize() -> UInt? { | ||
return streamSizeValue | ||
} | ||
|
||
// MARK: - Convenience factory methods for common test scenarios | ||
|
||
public static func objectWithNilInputStream() -> TestStreamableObject { | ||
return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: true) | ||
} | ||
|
||
public static func objectWithZeroSize() -> TestStreamableObject { | ||
return TestStreamableObject(streamSize: 0, shouldReturnNilInputStream: false) | ||
} | ||
|
||
public static func objectWithNegativeSize() -> TestStreamableObject { | ||
return TestStreamableObject(streamSize: nil, shouldReturnNilInputStream: false) | ||
} | ||
|
||
public static func objectWithErrorStream() -> TestStreamableObject { | ||
return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: false, shouldReturnErrorStream: true) | ||
} | ||
|
||
public static func objectWithZeroBytesRead() -> TestStreamableObject { | ||
return TestStreamableObject(streamSize: 10, shouldReturnNilInputStream: false, shouldReturnErrorStream: false) | ||
} | ||
|
||
public static func objectWithLargeSize() -> TestStreamableObject { | ||
// Return size larger than UInt32.max to test truncation | ||
return TestStreamableObject( | ||
streamSize: UInt.max, | ||
shouldReturnNilInputStream: false, | ||
shouldReturnErrorStream: false | ||
) | ||
} | ||
} |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
extension Data: SentryStreamable { | ||
func asInputStream() -> InputStream? { | ||
return InputStream(data: self) | ||
} | ||
|
||
func streamSize() -> UInt? { | ||
return UInt(self.count) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,98 @@ | ||||||||||||||
/** | ||||||||||||||
* This is a partial implementation of the MessagePack format. | ||||||||||||||
* We only need to concatenate a list of NSData into an envelope item. | ||||||||||||||
*/ | ||||||||||||||
class SentryMsgPackSerializer { | ||||||||||||||
@objc | ||||||||||||||
static func serializeDictionary(toMessagePack dictionary: [String: Any], intoFile fileURL: URL) -> Bool { | ||||||||||||||
do { | ||||||||||||||
let data = try serializeDictionaryToMessagePack(dictionary) | ||||||||||||||
try data.write(to: fileURL) | ||||||||||||||
return true | ||||||||||||||
} catch { | ||||||||||||||
SentrySDKLog.error("Failed to serialize dictionary to MessagePack or write to file - Error: \(error)") | ||||||||||||||
return false | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static func serializeDictionaryToMessagePack(_ dictionary: [String: Any]) throws -> Data { // swiftlint:disable:this function_body_length | ||||||||||||||
let outputStream = OutputStream.toMemory() | ||||||||||||||
outputStream.open() | ||||||||||||||
defer { outputStream.close() } | ||||||||||||||
|
||||||||||||||
let mapHeader = UInt8(0x80 | dictionary.count) // Map up to 15 elements | ||||||||||||||
_ = outputStream.write([mapHeader], maxLength: 1) | ||||||||||||||
|
||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The map header implementation silently truncates dictionaries with more than 15 elements by using
The current silent truncation can lead to data corruption that's difficult to debug.
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
for (key, anyValue) in dictionary { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stream write operations discard the return value which indicates the number of bytes actually written. If the write fails or writes fewer bytes than expected, this will silently corrupt the MessagePack data. Consider checking write return values and throwing errors on failure.
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
guard let value = anyValue as? SentryStreamable else { | ||||||||||||||
throw SentryMsgPackSerializerError.invalidValue("Value does not conform to SentryStreamable: \(anyValue)") | ||||||||||||||
} | ||||||||||||||
guard let keyData = key.data(using: .utf8) else { | ||||||||||||||
throw SentryMsgPackSerializerError.invalidInput("Could not encode key as UTF-8: \(key)") | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
let str8Header: UInt8 = 0xD9 // String up to 255 characters | ||||||||||||||
let keyLength = UInt8(truncatingIfNeeded: keyData.count) // Truncates if > 255, matching Objective-C behavior | ||||||||||||||
_ = outputStream.write([str8Header], maxLength: 1) | ||||||||||||||
_ = outputStream.write([keyLength], maxLength: 1) | ||||||||||||||
|
||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Key length is silently truncated to UInt8 using Note: A key with 255 UTF-8 characters might actually have more than 255 bytes if it contains multi-byte Unicode characters.
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
try keyData.withUnsafeBytes { bytes in | ||||||||||||||
guard let bufferAddress = bytes.bindMemory(to: UInt8.self).baseAddress else { | ||||||||||||||
throw SentryMsgPackSerializerError.invalidInput("Could not get buffer address for key: \(key)") | ||||||||||||||
} | ||||||||||||||
_ = outputStream.write(bufferAddress, maxLength: keyData.count) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
guard let dataLength = value.streamSize(), dataLength > 0 else { | ||||||||||||||
// MsgPack is being used strictly for session replay. | ||||||||||||||
Comment on lines
+41
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
// An item with a length of 0 will not be useful. | ||||||||||||||
// If we plan to use MsgPack for something else, | ||||||||||||||
// this needs to be re-evaluated. | ||||||||||||||
SentrySDKLog.error("Data for MessagePack dictionary has no content - Input: \(value)") | ||||||||||||||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
throw SentryMsgPackSerializerError.emptyData("Empty data for MessagePack dictionary") | ||||||||||||||
} | ||||||||||||||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Serializer Fails on Empty StreamsThe |
||||||||||||||
|
||||||||||||||
let valueLength = UInt32(truncatingIfNeeded: dataLength) | ||||||||||||||
// We will always use the 4 bytes data length for simplicity. | ||||||||||||||
// Worst case we're losing 3 bytes. | ||||||||||||||
let bin32Header: UInt8 = 0xC6 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Value length is truncated to UInt32 using
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
_ = outputStream.write([bin32Header], maxLength: 1) | ||||||||||||||
|
||||||||||||||
// Write UInt32 as big endian bytes | ||||||||||||||
let lengthBytes = [ | ||||||||||||||
UInt8((valueLength >> 24) & 0xFF), | ||||||||||||||
UInt8((valueLength >> 16) & 0xFF), | ||||||||||||||
UInt8((valueLength >> 8) & 0xFF), | ||||||||||||||
UInt8(valueLength & 0xFF) | ||||||||||||||
] | ||||||||||||||
_ = outputStream.write(lengthBytes, maxLength: 4) | ||||||||||||||
|
||||||||||||||
guard let inputStream = value.asInputStream() else { | ||||||||||||||
SentrySDKLog.error("Could not get input stream - Input: \(value)") | ||||||||||||||
throw SentryMsgPackSerializerError.streamError("Could not get input stream from value") | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
inputStream.open() | ||||||||||||||
defer { inputStream.close() } | ||||||||||||||
|
||||||||||||||
var buffer = [UInt8](repeating: 0, count: 1_024) | ||||||||||||||
var bytesRead: Int | ||||||||||||||
|
||||||||||||||
while inputStream.hasBytesAvailable { | ||||||||||||||
bytesRead = inputStream.read(&buffer, maxLength: buffer.count) | ||||||||||||||
if bytesRead > 0 { | ||||||||||||||
_ = outputStream.write(buffer, maxLength: bytesRead) | ||||||||||||||
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stream write operations inside the while loop don't check the return value, which could lead to partial writes and corrupted MessagePack data. The loop should verify that all bytes are written successfully.
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||
} else if bytesRead < 0 { | ||||||||||||||
SentrySDKLog.error("Error reading bytes from input stream - Input: \(value) - \(bytesRead)") | ||||||||||||||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
throw SentryMsgPackSerializerError.streamError("Error reading bytes from input stream") | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
guard let data = outputStream.property(forKey: .dataWrittenToMemoryStreamKey) as? Data else { | ||||||||||||||
throw SentryMsgPackSerializerError.outputError("Could not retrieve data from memory stream") | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return data | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
enum SentryMsgPackSerializerError: Error { | ||
case dictionaryTooLarge | ||
case invalidValue(String) | ||
case invalidInput(String) | ||
case emptyData(String) | ||
case streamError(String) | ||
case outputError(String) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
protocol SentryStreamable { | ||
func asInputStream() -> InputStream? | ||
func streamSize() -> UInt? | ||
} | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
extension URL: SentryStreamable { | ||
func asInputStream() -> InputStream? { | ||
return InputStream(url: self) | ||
} | ||
|
||
func streamSize() -> UInt? { | ||
let attributes: [FileAttributeKey: Any] | ||
philprime marked this conversation as resolved.
Show resolved
Hide resolved
|
||
do { | ||
attributes = try FileManager.default.attributesOfItem(atPath: path) | ||
} catch { | ||
SentrySDKLog.error("Could not read file attributes - File: \(self) - Error: \(error)") | ||
return nil | ||
} | ||
guard let fileSize = attributes[.size] as? NSNumber else { | ||
SentrySDKLog.error("Could not read file size attribute - File: \(self)") | ||
return nil | ||
} | ||
return fileSize.uintValue | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inconsistent Error Handling in Streamable ProtocolThe Additional Locations (1) |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Serializer Memory Leak & Map Header Crash
The
SentryMsgPackSerializer
accumulates all data in memory before writing to a file, potentially causing high memory usage or OOM errors for large datasets. Also, the map header's dictionary count calculation may crash at runtime if the count exceeds 255, unlike the original Objective-C truncation behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is true? At least the first part about accumulating everything to memory. Any reason we can't use the ObjC behavior here of having the output stream write to a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double-check why I changed it, it's been a while