authorgravatar for rganesan@arista.comGanesan Rajagopal <rganesan@arista.com> 2023-04-04 15:41:25+05:30
committergravatar for noreply@github.comGitHub <noreply@github.com> 2023-04-04 13:11:25+03:00
log49b56f88b92eb0f0e66e7cfa329392c918c1777e
tree69db3a981457c21cac91aa1729f2eb8c419100af
parent771d07268f7ecc9535ad6fbb8448c76581bf5188
signaturebadge-question-mark Signed by PGP key 4AEE18F83AFDEB23

GPA: Catch invalid frees

* GPA: Catch invalid frees Fix #14791: Catch cases where an invalid slice is passed to free(). This was silently ignored before but now logs an error. This change uses a AutoHashMap to keep track of the sizes which seems to be an overkill but seems like the easiest way to catch these errors. * GPA: Add wrong alignment checks to free/resize Implement @Inkryption's suggestion to catch free/resize with the wrong alignment. I also changed the naming to match large allocations.

1 files changed, 82 insertions(+), 0 deletions(-)

lib/std/heap/general_purpose_allocator.zig+82
...@@ -160,6 +160,7 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -160,6 +160,7 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
160 backing_allocator: Allocator = std.heap.page_allocator,160 backing_allocator: Allocator = std.heap.page_allocator,
161 buckets: [small_bucket_count]?*BucketHeader = [1]?*BucketHeader{null} ** small_bucket_count,161 buckets: [small_bucket_count]?*BucketHeader = [1]?*BucketHeader{null} ** small_bucket_count,
162 large_allocations: LargeAllocTable = .{},162 large_allocations: LargeAllocTable = .{},
163 small_allocations: if (config.safety) SmallAllocTable else void = if (config.safety) .{} else {},
163 empty_buckets: if (config.retain_metadata) ?*BucketHeader else void =164 empty_buckets: if (config.retain_metadata) ?*BucketHeader else void =
164 if (config.retain_metadata) null else {},165 if (config.retain_metadata) null else {},
165166
...@@ -194,6 +195,11 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -194,6 +195,11 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
194 const small_bucket_count = math.log2(page_size);195 const small_bucket_count = math.log2(page_size);
195 const largest_bucket_object_size = 1 << (small_bucket_count - 1);196 const largest_bucket_object_size = 1 << (small_bucket_count - 1);
196197
198 const SmallAlloc = struct {
199 requested_size: usize,
200 log2_ptr_align: u8,
201 };
202
197 const LargeAlloc = struct {203 const LargeAlloc = struct {
198 bytes: []u8,204 bytes: []u8,
199 requested_size: if (config.enable_memory_limit) usize else void,205 requested_size: if (config.enable_memory_limit) usize else void,
...@@ -227,6 +233,7 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -227,6 +233,7 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
227 }233 }
228 };234 };
229 const LargeAllocTable = std.AutoHashMapUnmanaged(usize, LargeAlloc);235 const LargeAllocTable = std.AutoHashMapUnmanaged(usize, LargeAlloc);
236 const SmallAllocTable = std.AutoHashMapUnmanaged(usize, SmallAlloc);
230237
231 // Bucket: In memory, in order:238 // Bucket: In memory, in order:
232 // * BucketHeader239 // * BucketHeader
...@@ -430,6 +437,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -430,6 +437,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
430 self.freeRetainedMetadata();437 self.freeRetainedMetadata();
431 }438 }
432 self.large_allocations.deinit(self.backing_allocator);439 self.large_allocations.deinit(self.backing_allocator);
440 if (config.safety) {
441 self.small_allocations.deinit(self.backing_allocator);
442 }
433 self.* = undefined;443 self.* = undefined;
434 return leaks;444 return leaks;
435 }445 }
...@@ -706,6 +716,34 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -706,6 +716,34 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
706 }716 }
707717
708 // Definitely an in-use small alloc now.718 // Definitely an in-use small alloc now.
719 if (config.safety) {
720 const entry = self.small_allocations.getEntry(@ptrToInt(old_mem.ptr)) orelse
721 @panic("Invalid free");
722 if (old_mem.len != entry.value_ptr.requested_size or log2_old_align != entry.value_ptr.log2_ptr_align) {
723 var addresses: [stack_n]usize = [1]usize{0} ** stack_n;
724 var free_stack_trace = StackTrace{
725 .instruction_addresses = &addresses,
726 .index = 0,
727 };
728 std.debug.captureStackTrace(ret_addr, &free_stack_trace);
729 if (old_mem.len != entry.value_ptr.requested_size) {
730 log.err("Allocation size {d} bytes does not match resize size {d}. Allocation: {} Resize: {}", .{
731 entry.value_ptr.requested_size,
732 old_mem.len,
733 bucketStackTrace(bucket, size_class, slot_index, .alloc),
734 free_stack_trace,
735 });
736 }
737 if (log2_old_align != entry.value_ptr.log2_ptr_align) {
738 log.err("Allocation alignment {d} does not match resize alignment {d}. Allocation: {} Resize: {}", .{
739 @as(usize, 1) << @intCast(math.Log2Int(usize), entry.value_ptr.log2_ptr_align),
740 @as(usize, 1) << @intCast(math.Log2Int(usize), log2_old_align),
741 bucketStackTrace(bucket, size_class, slot_index, .alloc),
742 free_stack_trace,
743 });
744 }
745 }
746 }
709 const prev_req_bytes = self.total_requested_bytes;747 const prev_req_bytes = self.total_requested_bytes;
710 if (config.enable_memory_limit) {748 if (config.enable_memory_limit) {
711 const new_req_bytes = prev_req_bytes + new_size - old_mem.len;749 const new_req_bytes = prev_req_bytes + new_size - old_mem.len;
...@@ -726,6 +764,10 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -726,6 +764,10 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
726 old_mem.len, old_mem.ptr, new_size,764 old_mem.len, old_mem.ptr, new_size,
727 });765 });
728 }766 }
767 if (config.safety) {
768 const entry = self.small_allocations.getEntry(@ptrToInt(old_mem.ptr)).?;
769 entry.value_ptr.requested_size = new_size;
770 }
729 return true;771 return true;
730 }772 }
731773
...@@ -796,6 +838,35 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -796,6 +838,35 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
796 }838 }
797839
798 // Definitely an in-use small alloc now.840 // Definitely an in-use small alloc now.
841 if (config.safety) {
842 const entry = self.small_allocations.getEntry(@ptrToInt(old_mem.ptr)) orelse
843 @panic("Invalid free");
844 if (old_mem.len != entry.value_ptr.requested_size or log2_old_align != entry.value_ptr.log2_ptr_align) {
845 var addresses: [stack_n]usize = [1]usize{0} ** stack_n;
846 var free_stack_trace = StackTrace{
847 .instruction_addresses = &addresses,
848 .index = 0,
849 };
850 std.debug.captureStackTrace(ret_addr, &free_stack_trace);
851 if (old_mem.len != entry.value_ptr.requested_size) {
852 log.err("Allocation size {d} bytes does not match free size {d}. Allocation: {} Free: {}", .{
853 entry.value_ptr.requested_size,
854 old_mem.len,
855 bucketStackTrace(bucket, size_class, slot_index, .alloc),
856 free_stack_trace,
857 });
858 }
859 if (log2_old_align != entry.value_ptr.log2_ptr_align) {
860 log.err("Allocation alignment {d} does not match free alignment {d}. Allocation: {} Free: {}", .{
861 @as(usize, 1) << @intCast(math.Log2Int(usize), entry.value_ptr.log2_ptr_align),
862 @as(usize, 1) << @intCast(math.Log2Int(usize), log2_old_align),
863 bucketStackTrace(bucket, size_class, slot_index, .alloc),
864 free_stack_trace,
865 });
866 }
867 }
868 }
869
799 if (config.enable_memory_limit) {870 if (config.enable_memory_limit) {
800 self.total_requested_bytes -= old_mem.len;871 self.total_requested_bytes -= old_mem.len;
801 }872 }
...@@ -840,6 +911,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -840,6 +911,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
840 } else {911 } else {
841 @memset(old_mem.ptr, undefined, old_mem.len);912 @memset(old_mem.ptr, undefined, old_mem.len);
842 }913 }
914 if (config.safety) {
915 assert(self.small_allocations.remove(@ptrToInt(old_mem.ptr)));
916 }
843 if (config.verbose_log) {917 if (config.verbose_log) {
844 log.info("small free {d} bytes at {*}", .{ old_mem.len, old_mem.ptr });918 log.info("small free {d} bytes at {*}", .{ old_mem.len, old_mem.ptr });
845 }919 }
...@@ -903,8 +977,16 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {...@@ -903,8 +977,16 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
903 return slice.ptr;977 return slice.ptr;
904 }978 }
905979
980 if (config.safety) {
981 try self.small_allocations.ensureUnusedCapacity(self.backing_allocator, 1);
982 }
906 const new_size_class = math.ceilPowerOfTwoAssert(usize, new_aligned_size);983 const new_size_class = math.ceilPowerOfTwoAssert(usize, new_aligned_size);
907 const ptr = try self.allocSlot(new_size_class, ret_addr);984 const ptr = try self.allocSlot(new_size_class, ret_addr);
985 if (config.safety) {
986 const gop = self.small_allocations.getOrPutAssumeCapacity(@ptrToInt(ptr));
987 gop.value_ptr.requested_size = len;
988 gop.value_ptr.log2_ptr_align = log2_ptr_align;
989 }
908 if (config.verbose_log) {990 if (config.verbose_log) {
909 log.info("small alloc {d} bytes at {*}", .{ len, ptr });991 log.info("small alloc {d} bytes at {*}", .{ len, ptr });
910 }992 }