Repository navigation
lsan: fix sql.postgres suppressions #22934
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ typedef struct TypedArrayDataCell { | |
|
|
||
| typedef union DataCellValue { | ||
| uint8_t null_value; | ||
| WTF::StringImpl* string; | ||
| BunString string; | ||
| double number; | ||
| int32_t integer; | ||
| int64_t bigint; | ||
|
|
@@ -69,7 +69,7 @@ typedef union DataCellValue { | |
| double date; | ||
| double date_with_time_zone; | ||
| size_t bytea[2]; | ||
| WTF::StringImpl* json; | ||
| BunString json; | ||
|
Collaborator
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. wait why make this change? we always want it to be a WTF::StringImpl? |
||
| DataCellArray array; | ||
| TypedArrayDataCell typed_array; | ||
| DataCellRaw raw; | ||
|
|
@@ -146,8 +146,8 @@ static JSC::JSValue toJS(JSC::VM& vm, JSC::JSGlobalObject* globalObject, DataCel | |
| return uint8Array; | ||
| } | ||
| case DataCellTag::String: { | ||
| if (cell.value.string) { | ||
| return jsString(vm, WTF::String(cell.value.string)); | ||
| if (cell.value.string.tag != BunStringTag::Dead) { | ||
| return jsString(vm, cell.value.string.transferToWTFString()); | ||
| } | ||
| return jsEmptyString(vm); | ||
| } | ||
|
|
@@ -186,8 +186,8 @@ static JSC::JSValue toJS(JSC::VM& vm, JSC::JSGlobalObject* globalObject, DataCel | |
| return uint8Array; | ||
| } | ||
| case DataCellTag::Json: { | ||
| if (cell.value.json) { | ||
| auto str = WTF::String(cell.value.json); | ||
| if (cell.value.json.tag != BunStringTag::Dead) { | ||
| auto str = cell.value.json.transferToWTFString(); | ||
| JSC::JSValue json = JSC::JSONParse(globalObject, str); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return json; | ||
|
|
@@ -430,7 +430,7 @@ extern "C" EncodedJSValue JSC__constructObjectFromDataCell( | |
| return JSValue::encode(toJS(array, structure, cells, count, globalObject, Bun::BunStructureFlags(flags), BunResultMode(result_mode), namesPtr, namesCount)); | ||
| } | ||
|
|
||
| extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject, JSC::JSCell* owner, uint32_t capacity, ExternColumnIdentifier* namesPtr) | ||
| extern "C" [[ZIG_EXPORT(nothrow)]] EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject, JSC::JSCell* owner, Bun::ExternColumnIdentifier* namesPtr, size_t capacity) | ||
| { | ||
| auto& vm = JSC::getVM(globalObject); | ||
|
|
||
|
|
@@ -441,7 +441,7 @@ extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject | |
| for (uint32_t i = 0; i < capacity; i++) { | ||
| ExternColumnIdentifier& name = names[i]; | ||
| if (name.isNamedColumn()) { | ||
| propertyNames.add(Identifier::fromString(vm, name.name.toWTFString())); | ||
| propertyNames.add(Identifier::fromString(vm, name.name.transferToWTFString())); | ||
|
Collaborator
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. This is probably not correct. This will make it so the property name is no longer usable and that string may outlive this call |
||
| } | ||
| nonDuplicateCount += !name.isDuplicateColumn(); | ||
| if (nonDuplicateCount == JSFinalObject::maxInlineCapacity) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -155,7 +155,7 @@ fn parseArray(bytes: []const u8, bigint: bool, comptime arrayType: types.Tag, gl | |||||||
| const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..]; | ||||||||
| defer if (needs_dynamic_buffer) bun.default_allocator.free(buffer); | ||||||||
| const unescaped = unescapePostgresString(str_bytes, buffer) catch return error.InvalidByteSequence; | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .json, .value = .{ .json = if (unescaped.len > 0) String.cloneUTF8(unescaped).value.WTFStringImpl else null }, .free_value = 1 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .json, .value = .{ .json = if (unescaped.len > 0) String.cloneUTF8(unescaped) else .dead }, .free_value = 1 }); | ||||||||
| slice = trySlice(slice, current_idx + 1); | ||||||||
|
Comment on lines
155
to
159
Contributor
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. Fix reversed buffer allocation condition (can cause spurious BufferTooSmall and heap churn) needs_dynamic_buffer should be true when input exceeds the stack buffer. Also, prefer slicing the stack buffer to str_bytes.len. Apply: - const needs_dynamic_buffer = str_bytes.len < stack_buffer.len;
- const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..];
+ const needs_dynamic_buffer = str_bytes.len > stack_buffer.len;
+ const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..str_bytes.len];And similarly below: - const needs_dynamic_buffer = str_bytes.len < stack_buffer.len;
- const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..];
+ const needs_dynamic_buffer = str_bytes.len > stack_buffer.len;
+ const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..str_bytes.len];Also applies to: 171-176 |
||||||||
| continue; | ||||||||
| }, | ||||||||
|
|
@@ -164,15 +164,15 @@ fn parseArray(bytes: []const u8, bigint: bool, comptime arrayType: types.Tag, gl | |||||||
| const str_bytes = slice[1..current_idx]; | ||||||||
| if (str_bytes.len == 0) { | ||||||||
| // empty string | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = null }, .free_value = 1 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = .dead }, .free_value = 1 }); | ||||||||
| slice = trySlice(slice, current_idx + 1); | ||||||||
| continue; | ||||||||
| } | ||||||||
| const needs_dynamic_buffer = str_bytes.len < stack_buffer.len; | ||||||||
| const buffer = if (needs_dynamic_buffer) try bun.default_allocator.alloc(u8, str_bytes.len) else stack_buffer[0..]; | ||||||||
| defer if (needs_dynamic_buffer) bun.default_allocator.free(buffer); | ||||||||
| const string_bytes = unescapePostgresString(str_bytes, buffer) catch return error.InvalidByteSequence; | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (string_bytes.len > 0) String.cloneUTF8(string_bytes).value.WTFStringImpl else null }, .free_value = 1 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (string_bytes.len > 0) String.cloneUTF8(string_bytes) else .dead }, .free_value = 1 }); | ||||||||
|
|
||||||||
| slice = trySlice(slice, current_idx + 1); | ||||||||
| continue; | ||||||||
|
|
@@ -243,9 +243,9 @@ fn parseArray(bytes: []const u8, bigint: bool, comptime arrayType: types.Tag, gl | |||||||
| } else { | ||||||||
| // the only escape sequency possible here is \b | ||||||||
| if (bun.strings.eqlComptime(element, "\\b")) { | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8("\x08").value.WTFStringImpl }, .free_value = 1 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.static("\x08") }, .free_value = 1 }); | ||||||||
| } else { | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element).value.WTFStringImpl else null }, .free_value = 0 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element) else .dead }, .free_value = 0 }); | ||||||||
| } | ||||||||
|
Comment on lines
+248
to
249
Contributor
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. Memory leak: free_value must be 1 for cloneUTF8 result This path creates a BunString via cloneUTF8 but sets free_value=0, so deref is skipped and leaks. Apply: - try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element) else .dead }, .free_value = 0 });
+ try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element) else .dead }, .free_value = 1 });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||
| } | ||||||||
| slice = trySlice(slice, current_idx); | ||||||||
|
|
@@ -405,7 +405,7 @@ fn parseArray(bytes: []const u8, bigint: bool, comptime arrayType: types.Tag, gl | |||||||
| if (bigint) { | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .int8, .value = .{ .int8 = std.fmt.parseInt(i64, element, 0) catch return error.UnsupportedArrayFormat } }); | ||||||||
| } else { | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element).value.WTFStringImpl else null }, .free_value = 1 }); | ||||||||
| try array.append(bun.default_allocator, SQLDataCell{ .tag = .string, .value = .{ .string = if (element.len > 0) bun.String.cloneUTF8(element) else .dead }, .free_value = 1 }); | ||||||||
| } | ||||||||
| slice = trySlice(slice, current_idx); | ||||||||
| continue; | ||||||||
|
|
@@ -536,7 +536,7 @@ pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, | |||||||
| // .int8 is a 64-bit integer always string | ||||||||
| return SQLDataCell{ .tag = .int8, .value = .{ .int8 = std.fmt.parseInt(i64, bytes, 0) catch 0 } }; | ||||||||
| } else { | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes).value.WTFStringImpl else null }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes) else .dead }, .free_value = 1 }; | ||||||||
| } | ||||||||
| }, | ||||||||
| .float8 => { | ||||||||
|
|
@@ -566,14 +566,14 @@ pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, | |||||||
|
|
||||||||
| // if is binary format lets display as a string because JS cant handle it in a safe way | ||||||||
| const result = parseBinaryNumeric(bytes, &numeric_buffer) catch return error.UnsupportedNumericFormat; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(result.slice()).value.WTFStringImpl }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(result.slice()) }, .free_value = 1 }; | ||||||||
| } else { | ||||||||
| // nice text is actually what we want here | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) String.cloneUTF8(bytes).value.WTFStringImpl else null }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) String.cloneUTF8(bytes) else .dead }, .free_value = 1 }; | ||||||||
| } | ||||||||
| }, | ||||||||
| .jsonb, .json => { | ||||||||
| return SQLDataCell{ .tag = .json, .value = .{ .json = if (bytes.len > 0) String.cloneUTF8(bytes).value.WTFStringImpl else null }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .json, .value = .{ .json = if (bytes.len > 0) String.cloneUTF8(bytes) else .dead }, .free_value = 1 }; | ||||||||
| }, | ||||||||
| .bool => { | ||||||||
| if (binary) { | ||||||||
|
|
@@ -614,7 +614,7 @@ pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, | |||||||
| var buffer: [32]u8 = undefined; | ||||||||
| const len = Postgres__formatTime(microseconds, &buffer, buffer.len); | ||||||||
|
|
||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(buffer[0..len]).value.WTFStringImpl }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(buffer[0..len]) }, .free_value = 1 }; | ||||||||
| } else if (tag == .timetz and bytes.len == 12) { | ||||||||
| // PostgreSQL sends timetz as microseconds since midnight (8 bytes) + timezone offset in seconds (4 bytes) | ||||||||
| const microseconds = @byteSwap(@as(i64, @bitCast(bytes[0..8].*))); | ||||||||
|
|
@@ -624,13 +624,13 @@ pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, | |||||||
| var buffer: [48]u8 = undefined; | ||||||||
| const len = Postgres__formatTimeTz(microseconds, tz_offset_seconds, &buffer, buffer.len); | ||||||||
|
|
||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(buffer[0..len]).value.WTFStringImpl }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = bun.String.cloneUTF8(buffer[0..len]) }, .free_value = 1 }; | ||||||||
| } else { | ||||||||
| return error.InvalidBinaryData; | ||||||||
| } | ||||||||
| } else { | ||||||||
| // Text format - just return as string | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes).value.WTFStringImpl else null }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes) else .dead }, .free_value = 1 }; | ||||||||
| } | ||||||||
| }, | ||||||||
|
|
||||||||
|
|
@@ -697,7 +697,7 @@ pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, | |||||||
| return try parseArray(bytes, bigint, tag, globalObject, null, false); | ||||||||
| }, | ||||||||
| else => { | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes).value.WTFStringImpl else null }, .free_value = 1 }; | ||||||||
| return SQLDataCell{ .tag = .string, .value = .{ .string = if (bytes.len > 0) bun.String.cloneUTF8(bytes) else .dead }, .free_value = 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.
🧩 Analysis chain
Verify proper memory management for BunString fields.
The change from
WTF::StringImpl*toBunStringfor string and json fields is a good improvement for memory safety. However, ensure that:.deadsentinel when empty)Also applies to: 72-72
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 9803
Implement DataCell destructor for BunString cleanup
DataCell::~DataCell()in src/bun.js/bindings/SQLClient.cpp to deinitvalue.stringandvalue.jsonwhentag == DataCellTag::StringorDataCellTag::Json.BunStringis initialized withDeadtag.🤖 Prompt for AI Agents