authorgravatar for squeek502@hotmail.comRyan Liptak <squeek502@hotmail.com> 2022-12-14 19:05:25-08:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2022-12-15 02:56:24-05:00
log83e0e23f8aa2d90495bbf9c3649aa68f7c55c926
tree47d4660de7134582454d3ef229fe6d516070208a
parent8ff9284c469226faa2be0c73ab27672e1e8a55d1

ArrayList.toOwnedSlice: Fix potential for leaks when using errdefer

#13666 introduced a footgun when using `toOwnedSlice` with `errdefer array_list.deinit()`, since `toOwnedSlice` could retain capacity if `resize` failed, meaning it would leak without `deinit` being called. This meant that the only correct way to use `toOwnedSlice` was with `defer` instead of `errdefer` to ensure that the ArrayList would get cleaned up. Now, toOwnedSlice will now behave similarly to how it did before #13666, in that it will always clear the ArrayList's capacity if the resize/realloc succeeds. This also reverts commit 05890a12f532ba9d58904a14381ec174b9efe473, which was contingent on the modified toOwnedSlice behavior. Closes #13946

1 files changed, 11 insertions(+), 13 deletions(-)

lib/std/array_list.zig+11-13
...@@ -51,7 +51,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -51,7 +51,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
51 return if (alignment) |a| ([:s]align(a) T) else [:s]T;51 return if (alignment) |a| ([:s]align(a) T) else [:s]T;
52 }52 }
5353
54 /// Deinitialize with `deinit`.54 /// Deinitialize with `deinit` or use `toOwnedSlice`.
55 pub fn init(allocator: Allocator) Self {55 pub fn init(allocator: Allocator) Self {
56 return Self{56 return Self{
57 .items = &[_]T{},57 .items = &[_]T{},
...@@ -62,7 +62,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -62,7 +62,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
6262
63 /// Initialize with capacity to hold at least `num` elements.63 /// Initialize with capacity to hold at least `num` elements.
64 /// The resulting capacity is likely to be equal to `num`.64 /// The resulting capacity is likely to be equal to `num`.
65 /// Deinitialize with `deinit`.65 /// Deinitialize with `deinit` or use `toOwnedSlice`.
66 pub fn initCapacity(allocator: Allocator, num: usize) Allocator.Error!Self {66 pub fn initCapacity(allocator: Allocator, num: usize) Allocator.Error!Self {
67 var self = Self.init(allocator);67 var self = Self.init(allocator);
68 try self.ensureTotalCapacityPrecise(num);68 try self.ensureTotalCapacityPrecise(num);
...@@ -78,7 +78,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -78,7 +78,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
7878
79 /// ArrayList takes ownership of the passed in slice. The slice must have been79 /// ArrayList takes ownership of the passed in slice. The slice must have been
80 /// allocated with `allocator`.80 /// allocated with `allocator`.
81 /// Deinitialize with `deinit`.81 /// Deinitialize with `deinit` or use `toOwnedSlice`.
82 pub fn fromOwnedSlice(allocator: Allocator, slice: Slice) Self {82 pub fn fromOwnedSlice(allocator: Allocator, slice: Slice) Self {
83 return Self{83 return Self{
84 .items = slice,84 .items = slice,
...@@ -97,8 +97,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -97,8 +97,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
97 }97 }
9898
99 /// The caller owns the returned memory. Empties this ArrayList,99 /// The caller owns the returned memory. Empties this ArrayList,
100 /// however its capacity may or may not be cleared and deinit() is100 /// Its capacity is cleared, making deinit() safe but unnecessary to call.
101 /// still required to clean up its memory.
102 pub fn toOwnedSlice(self: *Self) Allocator.Error!Slice {101 pub fn toOwnedSlice(self: *Self) Allocator.Error!Slice {
103 const allocator = self.allocator;102 const allocator = self.allocator;
104103
...@@ -112,7 +111,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -112,7 +111,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
112 const new_memory = try allocator.alignedAlloc(T, alignment, self.items.len);111 const new_memory = try allocator.alignedAlloc(T, alignment, self.items.len);
113 mem.copy(T, new_memory, self.items);112 mem.copy(T, new_memory, self.items);
114 @memset(@ptrCast([*]u8, self.items.ptr), undefined, self.items.len * @sizeOf(T));113 @memset(@ptrCast([*]u8, self.items.ptr), undefined, self.items.len * @sizeOf(T));
115 self.items.len = 0;114 self.clearAndFree();
116 return new_memory;115 return new_memory;
117 }116 }
118117
...@@ -475,7 +474,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {...@@ -475,7 +474,7 @@ pub fn ArrayListAligned(comptime T: type, comptime alignment: ?u29) type {
475/// An ArrayList, but the allocator is passed as a parameter to the relevant functions474/// An ArrayList, but the allocator is passed as a parameter to the relevant functions
476/// rather than stored in the struct itself. The same allocator **must** be used throughout475/// rather than stored in the struct itself. The same allocator **must** be used throughout
477/// the entire lifetime of an ArrayListUnmanaged. Initialize directly or with476/// the entire lifetime of an ArrayListUnmanaged. Initialize directly or with
478/// `initCapacity`, and deinitialize with `deinit`.477/// `initCapacity`, and deinitialize with `deinit` or use `toOwnedSlice`.
479pub fn ArrayListUnmanaged(comptime T: type) type {478pub fn ArrayListUnmanaged(comptime T: type) type {
480 return ArrayListAlignedUnmanaged(T, null);479 return ArrayListAlignedUnmanaged(T, null);
481}480}
...@@ -483,7 +482,7 @@ pub fn ArrayListUnmanaged(comptime T: type) type {...@@ -483,7 +482,7 @@ pub fn ArrayListUnmanaged(comptime T: type) type {
483/// An ArrayListAligned, but the allocator is passed as a parameter to the relevant482/// An ArrayListAligned, but the allocator is passed as a parameter to the relevant
484/// functions rather than stored in the struct itself. The same allocator **must**483/// functions rather than stored in the struct itself. The same allocator **must**
485/// be used throughout the entire lifetime of an ArrayListAlignedUnmanaged.484/// be used throughout the entire lifetime of an ArrayListAlignedUnmanaged.
486/// Initialize directly or with `initCapacity`, and deinitialize with `deinit`.485/// Initialize directly or with `initCapacity`, and deinitialize with `deinit` or use `toOwnedSlice`.
487pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) type {486pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) type {
488 if (alignment) |a| {487 if (alignment) |a| {
489 if (a == @alignOf(T)) {488 if (a == @alignOf(T)) {
...@@ -514,7 +513,7 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ...@@ -514,7 +513,7 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ
514513
515 /// Initialize with capacity to hold at least num elements.514 /// Initialize with capacity to hold at least num elements.
516 /// The resulting capacity is likely to be equal to `num`.515 /// The resulting capacity is likely to be equal to `num`.
517 /// Deinitialize with `deinit`.516 /// Deinitialize with `deinit` or use `toOwnedSlice`.
518 pub fn initCapacity(allocator: Allocator, num: usize) Allocator.Error!Self {517 pub fn initCapacity(allocator: Allocator, num: usize) Allocator.Error!Self {
519 var self = Self{};518 var self = Self{};
520 try self.ensureTotalCapacityPrecise(allocator, num);519 try self.ensureTotalCapacityPrecise(allocator, num);
...@@ -533,9 +532,8 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ...@@ -533,9 +532,8 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ
533 return .{ .items = self.items, .capacity = self.capacity, .allocator = allocator };532 return .{ .items = self.items, .capacity = self.capacity, .allocator = allocator };
534 }533 }
535534
536 /// The caller owns the returned memory. Empties this ArrayList,535 /// The caller owns the returned memory. Empties this ArrayList.
537 /// however its capacity may or may not be cleared and deinit() is536 /// Its capacity is cleared, making deinit() safe but unnecessary to call.
538 /// still required to clean up its memory.
539 pub fn toOwnedSlice(self: *Self, allocator: Allocator) Allocator.Error!Slice {537 pub fn toOwnedSlice(self: *Self, allocator: Allocator) Allocator.Error!Slice {
540 const old_memory = self.allocatedSlice();538 const old_memory = self.allocatedSlice();
541 if (allocator.resize(old_memory, self.items.len)) {539 if (allocator.resize(old_memory, self.items.len)) {
...@@ -547,7 +545,7 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ...@@ -547,7 +545,7 @@ pub fn ArrayListAlignedUnmanaged(comptime T: type, comptime alignment: ?u29) typ
547 const new_memory = try allocator.alignedAlloc(T, alignment, self.items.len);545 const new_memory = try allocator.alignedAlloc(T, alignment, self.items.len);
548 mem.copy(T, new_memory, self.items);546 mem.copy(T, new_memory, self.items);
549 @memset(@ptrCast([*]u8, self.items.ptr), undefined, self.items.len * @sizeOf(T));547 @memset(@ptrCast([*]u8, self.items.ptr), undefined, self.items.len * @sizeOf(T));
550 self.items.len = 0;548 self.clearAndFree(allocator);
551 return new_memory;549 return new_memory;
552 }550 }
553551