Commit edb06ab2 authored by 徐豪's avatar 徐豪
Browse files

init

parents

Too many changes to show.

To preserve performance only 532 of 532+ files are displayed.
From 97f14ebfd8d24d71e10c450e0a90b6322f9c0d59 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 22 Dec 2020 15:33:08 +0100
Subject: [PATCH] Expose `Thread#memory_allocations` counters
This provides currently a per-thread GC heap slots
and malloc allocations statistics.
This is designed to measure a memory allocations
in a multi-threaded environments (concurrent requests
processing) with an accurate information about allocated
memory within a given execution context.
Example: Measure memory pressure generated by a given
requests to easier find requests with a lot of allocations.
---
gc.c | 20 ++++++
.../test_thread_trace_memory_allocations.rb | 67 +++++++++++++++++++
thread.c | 55 +++++++++++++++
vm_core.h | 17 +++++
4 files changed, 159 insertions(+)
create mode 100644 test/ruby/test_thread_trace_memory_allocations.rb
diff --git a/gc.c b/gc.c
index 73faf46b128b..f2dcd2935052 100644
--- a/gc.c
+++ b/gc.c
@@ -2172,6 +2172,13 @@ newobj_init(VALUE klass, VALUE flags, VALUE v1, VALUE v2, VALUE v3, int wb_prote
GC_ASSERT(!SPECIAL_CONST_P(obj)); /* check alignment */
#endif
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_allocated_objects);
+ }
+#endif
+
objspace->total_allocated_objects++;
gc_report(5, objspace, "newobj: %s\n", obj_info(obj));
@@ -9732,6 +9739,19 @@ objspace_malloc_increase(rb_objspace_t *objspace, void *mem, size_t new_size, si
#endif
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ if (new_size > old_size) {
+ ATOMIC_SIZE_ADD(th->memory_allocations.total_malloc_bytes, new_size - old_size);
+ }
+
+ if (type == MEMOP_TYPE_MALLOC) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_mallocs);
+ }
+ }
+#endif
+
if (type == MEMOP_TYPE_MALLOC) {
retry:
if (malloc_increase > malloc_limit && ruby_native_thread_p() && !dont_gc) {
diff --git a/test/ruby/test_thread_trace_memory_allocations.rb b/test/ruby/test_thread_trace_memory_allocations.rb
new file mode 100644
index 000000000000..2e281513578b
--- /dev/null
+++ b/test/ruby/test_thread_trace_memory_allocations.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require 'test/unit'
+
+class TestThreadTraceMemoryAllocations < Test::Unit::TestCase
+ def test_disabled_trace_memory_allocations
+ Thread.trace_memory_allocations = false
+
+ assert_predicate Thread.current.memory_allocations, :nil?
+ end
+
+ def test_enabled_trace_memory_allocations
+ Thread.trace_memory_allocations = true
+
+ assert_not_nil(Thread.current.memory_allocations)
+ end
+
+ def test_only_this_thread_allocations_are_counted
+ changed = {
+ total_allocated_objects: 1000,
+ total_malloc_bytes: 1_000_000,
+ total_mallocs: 100
+ }
+
+ Thread.trace_memory_allocations = true
+
+ assert_less_than(changed) do
+ Thread.new do
+ assert_greater_than(changed) do
+ # This will allocate: 5k objects, 5k mallocs, 5MB
+ allocate(5000, 1000)
+ end
+ end.join
+
+ # This will allocate: 50 objects, 50 mallocs, 500 bytes
+ allocate(50, 10)
+ end
+ end
+
+ private
+
+ def allocate(slots, bytes)
+ Array.new(slots).map do
+ '0' * bytes
+ end
+ end
+
+ def assert_greater_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :<=, after[key]-before[key], "expected the #{key} to change more than #{by}")
+ end
+ end
+
+ def assert_less_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :>, after[key]-before[key], "expected the #{key} to change less than #{by}")
+ end
+ end
+end
diff --git a/thread.c b/thread.c
index 708aaa471d99..d68a59e9f2d6 100644
--- a/thread.c
+++ b/thread.c
@@ -5143,6 +5143,55 @@ rb_thread_backtrace_locations_m(int argc, VALUE *argv, VALUE thval)
return rb_vm_thread_backtrace_locations(argc, argv, thval);
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+rb_thread_t *
+ruby_threadptr_for_trace_memory_allocations(void)
+{
+ // The order of this checks is important due
+ // to how Ruby VM is initialized
+ if (GET_VM()->thread_trace_memory_allocations && GET_EC() != NULL) {
+ return GET_THREAD();
+ }
+
+ return NULL;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations(VALUE _)
+{
+ return GET_THREAD()->vm->thread_trace_memory_allocations ? Qtrue : Qfalse;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations_set(VALUE self, VALUE val)
+{
+ GET_THREAD()->vm->thread_trace_memory_allocations = RTEST(val);
+ return val;
+}
+
+static VALUE
+rb_thread_memory_allocations(VALUE self)
+{
+ rb_thread_t *th = rb_thread_ptr(self);
+
+ if (!th->vm->thread_trace_memory_allocations) {
+ return Qnil;
+ }
+
+ VALUE ret = rb_hash_new();
+
+ VALUE total_allocated_objects = ID2SYM(rb_intern_const("total_allocated_objects"));
+ VALUE total_malloc_bytes = ID2SYM(rb_intern_const("total_malloc_bytes"));
+ VALUE total_mallocs = ID2SYM(rb_intern_const("total_mallocs"));
+
+ rb_hash_aset(ret, total_allocated_objects, SIZET2NUM(th->memory_allocations.total_allocated_objects));
+ rb_hash_aset(ret, total_malloc_bytes, SIZET2NUM(th->memory_allocations.total_malloc_bytes));
+ rb_hash_aset(ret, total_mallocs, SIZET2NUM(th->memory_allocations.total_mallocs));
+
+ return ret;
+}
+#endif
+
/*
* Document-class: ThreadError
*
@@ -5230,6 +5279,12 @@ Init_Thread(void)
rb_define_method(rb_cThread, "to_s", rb_thread_to_s, 0);
rb_define_alias(rb_cThread, "inspect", "to_s");
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations", rb_thread_s_trace_memory_allocations, 0);
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations=", rb_thread_s_trace_memory_allocations_set, 1);
+ rb_define_method(rb_cThread, "memory_allocations", rb_thread_memory_allocations, 0);
+#endif
+
rb_vm_register_special_exception(ruby_error_stream_closed, rb_eIOError,
"stream closed in another thread");
diff --git a/vm_core.h b/vm_core.h
index 12c3ac377551..63cdf55fa6ed 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -69,6 +69,13 @@
# define VM_INSN_INFO_TABLE_IMPL 2
#endif
+/*
+ * track a per thread memory allocations
+ */
+#ifndef THREAD_TRACE_MEMORY_ALLOCATIONS
+# define THREAD_TRACE_MEMORY_ALLOCATIONS 1
+#endif
+
#include "ruby/ruby.h"
#include "ruby/st.h"
@@ -602,6 +609,7 @@ typedef struct rb_vm_struct {
unsigned int running: 1;
unsigned int thread_abort_on_exception: 1;
unsigned int thread_report_on_exception: 1;
+ unsigned int thread_trace_memory_allocations: 1;
unsigned int safe_level_: 1;
int sleeper;
@@ -960,6 +968,14 @@ typedef struct rb_thread_struct {
rb_thread_list_t *join_list;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ struct {
+ size_t total_allocated_objects;
+ size_t total_malloc_bytes;
+ size_t total_mallocs;
+ } memory_allocations;
+#endif
+
union {
struct {
VALUE proc;
@@ -1852,6 +1868,7 @@ void rb_threadptr_interrupt(rb_thread_t *th);
void rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th);
void rb_threadptr_pending_interrupt_clear(rb_thread_t *th);
void rb_threadptr_pending_interrupt_enque(rb_thread_t *th, VALUE v);
+rb_thread_t *ruby_threadptr_for_trace_memory_allocations(void);
VALUE rb_ec_get_errinfo(const rb_execution_context_t *ec);
void rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo);
void rb_execution_context_update(const rb_execution_context_t *ec);
From 97f14ebfd8d24d71e10c450e0a90b6322f9c0d59 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 22 Dec 2020 15:33:08 +0100
Subject: [PATCH] Expose `Thread#memory_allocations` counters
This provides currently a per-thread GC heap slots
and malloc allocations statistics.
This is designed to measure a memory allocations
in a multi-threaded environments (concurrent requests
processing) with an accurate information about allocated
memory within a given execution context.
Example: Measure memory pressure generated by a given
requests to easier find requests with a lot of allocations.
Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/296530
---
gc.c | 20 ++++++
.../test_thread_trace_memory_allocations.rb | 67 +++++++++++++++++++
thread.c | 55 +++++++++++++++
vm_core.h | 17 +++++
4 files changed, 159 insertions(+)
create mode 100644 test/ruby/test_thread_trace_memory_allocations.rb
diff --git a/gc.c b/gc.c
index 5d0c342..b82d35f 100644
--- a/gc.c
+++ b/gc.c
@@ -2126,6 +2126,13 @@ newobj_init(VALUE klass, VALUE flags, int wb_protected, rb_objspace_t *objspace,
// TODO: make it atomic, or ractor local
objspace->total_allocated_objects++;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_allocated_objects);
+ }
+#endif
+
#if RGENGC_PROFILE
if (wb_protected) {
objspace->profile.total_generated_normal_object_count++;
@@ -10446,6 +10453,19 @@ objspace_malloc_increase(rb_objspace_t *objspace, void *mem, size_t new_size, si
#endif
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ if (new_size > old_size) {
+ ATOMIC_SIZE_ADD(th->memory_allocations.total_malloc_bytes, new_size - old_size);
+ }
+
+ if (type == MEMOP_TYPE_MALLOC) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_mallocs);
+ }
+ }
+#endif
+
if (type == MEMOP_TYPE_MALLOC) {
retry:
if (malloc_increase > malloc_limit && ruby_native_thread_p() && !dont_gc_val()) {
diff --git a/test/ruby/test_thread_trace_memory_allocations.rb b/test/ruby/test_thread_trace_memory_allocations.rb
new file mode 100644
index 0000000..2e28151
--- /dev/null
+++ b/test/ruby/test_thread_trace_memory_allocations.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require 'test/unit'
+
+class TestThreadTraceMemoryAllocations < Test::Unit::TestCase
+ def test_disabled_trace_memory_allocations
+ Thread.trace_memory_allocations = false
+
+ assert_predicate Thread.current.memory_allocations, :nil?
+ end
+
+ def test_enabled_trace_memory_allocations
+ Thread.trace_memory_allocations = true
+
+ assert_not_nil(Thread.current.memory_allocations)
+ end
+
+ def test_only_this_thread_allocations_are_counted
+ changed = {
+ total_allocated_objects: 1000,
+ total_malloc_bytes: 1_000_000,
+ total_mallocs: 100
+ }
+
+ Thread.trace_memory_allocations = true
+
+ assert_less_than(changed) do
+ Thread.new do
+ assert_greater_than(changed) do
+ # This will allocate: 5k objects, 5k mallocs, 5MB
+ allocate(5000, 1000)
+ end
+ end.join
+
+ # This will allocate: 50 objects, 50 mallocs, 500 bytes
+ allocate(50, 10)
+ end
+ end
+
+ private
+
+ def allocate(slots, bytes)
+ Array.new(slots).map do
+ '0' * bytes
+ end
+ end
+
+ def assert_greater_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :<=, after[key]-before[key], "expected the #{key} to change more than #{by}")
+ end
+ end
+
+ def assert_less_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :>, after[key]-before[key], "expected the #{key} to change less than #{by}")
+ end
+ end
+end
diff --git a/thread.c b/thread.c
index 59279ef..be6014f 100644
--- a/thread.c
+++ b/thread.c
@@ -5438,6 +5438,55 @@ Init_Thread_Mutex(void)
rb_native_mutex_initialize(&th->interrupt_lock);
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+rb_thread_t *
+ruby_threadptr_for_trace_memory_allocations(void)
+{
+ // The order of this checks is important due
+ // to how Ruby VM is initialized
+ if (GET_VM()->thread_trace_memory_allocations && GET_EC() != NULL) {
+ return GET_THREAD();
+ }
+
+ return NULL;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations(VALUE _)
+{
+ return GET_THREAD()->vm->thread_trace_memory_allocations ? Qtrue : Qfalse;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations_set(VALUE self, VALUE val)
+{
+ GET_THREAD()->vm->thread_trace_memory_allocations = RTEST(val);
+ return val;
+}
+
+static VALUE
+rb_thread_memory_allocations(VALUE self)
+{
+ rb_thread_t *th = rb_thread_ptr(self);
+
+ if (!th->vm->thread_trace_memory_allocations) {
+ return Qnil;
+ }
+
+ VALUE ret = rb_hash_new();
+
+ VALUE total_allocated_objects = ID2SYM(rb_intern_const("total_allocated_objects"));
+ VALUE total_malloc_bytes = ID2SYM(rb_intern_const("total_malloc_bytes"));
+ VALUE total_mallocs = ID2SYM(rb_intern_const("total_mallocs"));
+
+ rb_hash_aset(ret, total_allocated_objects, SIZET2NUM(th->memory_allocations.total_allocated_objects));
+ rb_hash_aset(ret, total_malloc_bytes, SIZET2NUM(th->memory_allocations.total_malloc_bytes));
+ rb_hash_aset(ret, total_mallocs, SIZET2NUM(th->memory_allocations.total_mallocs));
+
+ return ret;
+}
+#endif
+
/*
* Document-class: ThreadError
*
@@ -5523,6 +5572,12 @@ Init_Thread(void)
rb_define_method(rb_cThread, "to_s", rb_thread_to_s, 0);
rb_define_alias(rb_cThread, "inspect", "to_s");
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations", rb_thread_s_trace_memory_allocations, 0);
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations=", rb_thread_s_trace_memory_allocations_set, 1);
+ rb_define_method(rb_cThread, "memory_allocations", rb_thread_memory_allocations, 0);
+#endif
+
rb_vm_register_special_exception(ruby_error_stream_closed, rb_eIOError,
"stream closed in another thread");
diff --git a/vm_core.h b/vm_core.h
index 6ab3608..196ab92 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -97,6 +97,13 @@
# define VM_INSN_INFO_TABLE_IMPL 2
#endif
+/*
+ * track a per thread memory allocations
+ */
+#ifndef THREAD_TRACE_MEMORY_ALLOCATIONS
+# define THREAD_TRACE_MEMORY_ALLOCATIONS 1
+#endif
+
#if defined(NSIG_MAX) /* POSIX issue 8 */
# undef NSIG
# define NSIG NSIG_MAX
@@ -606,6 +613,7 @@ typedef struct rb_vm_struct {
unsigned int thread_abort_on_exception: 1;
unsigned int thread_report_on_exception: 1;
unsigned int thread_ignore_deadlock: 1;
+ unsigned int thread_trace_memory_allocations: 1;
/* object management */
VALUE mark_object_ary;
@@ -986,6 +994,14 @@ typedef struct rb_thread_struct {
struct rb_waiting_list *join_list;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ struct {
+ size_t total_allocated_objects;
+ size_t total_malloc_bytes;
+ size_t total_mallocs;
+ } memory_allocations;
+#endif
+
union {
struct {
VALUE proc;
@@ -1906,6 +1922,7 @@ void rb_threadptr_interrupt(rb_thread_t *th);
void rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th);
void rb_threadptr_pending_interrupt_clear(rb_thread_t *th);
void rb_threadptr_pending_interrupt_enque(rb_thread_t *th, VALUE v);
+rb_thread_t *ruby_threadptr_for_trace_memory_allocations(void);
VALUE rb_ec_get_errinfo(const rb_execution_context_t *ec);
void rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo);
void rb_execution_context_update(const rb_execution_context_t *ec);
From 431c627d3af67fb664b6285720ef3a2ab53c6030 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 22 Dec 2020 15:33:08 +0100
Subject: [PATCH] Expose `Thread#memory_allocations` counters
This provides currently a per-thread GC heap slots
and malloc allocations statistics.
This is designed to measure a memory allocations
in a multi-threaded environments (concurrent requests
processing) with an accurate information about allocated
memory within a given execution context.
Example: Measure memory pressure generated by a given
requests to easier find requests with a lot of allocations.
Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/296530
---
gc.c | 20 ++++++
.../test_thread_trace_memory_allocations.rb | 67 +++++++++++++++++++
thread.c | 55 +++++++++++++++
vm_core.h | 17 +++++
4 files changed, 159 insertions(+)
create mode 100644 test/ruby/test_thread_trace_memory_allocations.rb
diff --git a/gc.c b/gc.c
index 030a4627bd..5e57e4bbfd 100644
--- a/gc.c
+++ b/gc.c
@@ -2285,6 +2285,13 @@ newobj_init(VALUE klass, VALUE flags, int wb_protected, rb_objspace_t *objspace,
// TODO: make it atomic, or ractor local
objspace->total_allocated_objects++;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_allocated_objects);
+ }
+#endif
+
#if RGENGC_PROFILE
if (wb_protected) {
objspace->profile.total_generated_normal_object_count++;
@@ -11305,6 +11312,19 @@ objspace_malloc_increase_body(rb_objspace_t *objspace, void *mem, size_t new_siz
#endif
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ if (new_size > old_size) {
+ ATOMIC_SIZE_ADD(th->memory_allocations.total_malloc_bytes, new_size - old_size);
+ }
+
+ if (type == MEMOP_TYPE_MALLOC) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_mallocs);
+ }
+ }
+#endif
+
if (type == MEMOP_TYPE_MALLOC) {
retry:
if (malloc_increase > malloc_limit && ruby_native_thread_p() && !dont_gc_val()) {
diff --git a/test/ruby/test_thread_trace_memory_allocations.rb b/test/ruby/test_thread_trace_memory_allocations.rb
new file mode 100644
index 0000000000..2e28151357
--- /dev/null
+++ b/test/ruby/test_thread_trace_memory_allocations.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require 'test/unit'
+
+class TestThreadTraceMemoryAllocations < Test::Unit::TestCase
+ def test_disabled_trace_memory_allocations
+ Thread.trace_memory_allocations = false
+
+ assert_predicate Thread.current.memory_allocations, :nil?
+ end
+
+ def test_enabled_trace_memory_allocations
+ Thread.trace_memory_allocations = true
+
+ assert_not_nil(Thread.current.memory_allocations)
+ end
+
+ def test_only_this_thread_allocations_are_counted
+ changed = {
+ total_allocated_objects: 1000,
+ total_malloc_bytes: 1_000_000,
+ total_mallocs: 100
+ }
+
+ Thread.trace_memory_allocations = true
+
+ assert_less_than(changed) do
+ Thread.new do
+ assert_greater_than(changed) do
+ # This will allocate: 5k objects, 5k mallocs, 5MB
+ allocate(5000, 1000)
+ end
+ end.join
+
+ # This will allocate: 50 objects, 50 mallocs, 500 bytes
+ allocate(50, 10)
+ end
+ end
+
+ private
+
+ def allocate(slots, bytes)
+ Array.new(slots).map do
+ '0' * bytes
+ end
+ end
+
+ def assert_greater_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :<=, after[key]-before[key], "expected the #{key} to change more than #{by}")
+ end
+ end
+
+ def assert_less_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :>, after[key]-before[key], "expected the #{key} to change less than #{by}")
+ end
+ end
+end
diff --git a/thread.c b/thread.c
index de29340713..c02f2b7945 100644
--- a/thread.c
+++ b/thread.c
@@ -5402,6 +5402,55 @@ Init_Thread_Mutex(void)
rb_native_mutex_initialize(&th->interrupt_lock);
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+rb_thread_t *
+ruby_threadptr_for_trace_memory_allocations(void)
+{
+ // The order of this checks is important due
+ // to how Ruby VM is initialized
+ if (GET_VM()->thread_trace_memory_allocations && GET_EC() != NULL) {
+ return GET_THREAD();
+ }
+
+ return NULL;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations(VALUE _)
+{
+ return GET_THREAD()->vm->thread_trace_memory_allocations ? Qtrue : Qfalse;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations_set(VALUE self, VALUE val)
+{
+ GET_THREAD()->vm->thread_trace_memory_allocations = RTEST(val);
+ return val;
+}
+
+static VALUE
+rb_thread_memory_allocations(VALUE self)
+{
+ rb_thread_t *th = rb_thread_ptr(self);
+
+ if (!th->vm->thread_trace_memory_allocations) {
+ return Qnil;
+ }
+
+ VALUE ret = rb_hash_new();
+
+ VALUE total_allocated_objects = ID2SYM(rb_intern_const("total_allocated_objects"));
+ VALUE total_malloc_bytes = ID2SYM(rb_intern_const("total_malloc_bytes"));
+ VALUE total_mallocs = ID2SYM(rb_intern_const("total_mallocs"));
+
+ rb_hash_aset(ret, total_allocated_objects, SIZET2NUM(th->memory_allocations.total_allocated_objects));
+ rb_hash_aset(ret, total_malloc_bytes, SIZET2NUM(th->memory_allocations.total_malloc_bytes));
+ rb_hash_aset(ret, total_mallocs, SIZET2NUM(th->memory_allocations.total_mallocs));
+
+ return ret;
+}
+#endif
+
/*
* Document-class: ThreadError
*
@@ -5488,6 +5537,12 @@ Init_Thread(void)
rb_define_method(rb_cThread, "to_s", rb_thread_to_s, 0);
rb_define_alias(rb_cThread, "inspect", "to_s");
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations", rb_thread_s_trace_memory_allocations, 0);
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations=", rb_thread_s_trace_memory_allocations_set, 1);
+ rb_define_method(rb_cThread, "memory_allocations", rb_thread_memory_allocations, 0);
+#endif
+
rb_vm_register_special_exception(ruby_error_stream_closed, rb_eIOError,
"stream closed in another thread");
diff --git a/vm_core.h b/vm_core.h
index 11866b85e5..f36b582cc7 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -94,6 +94,13 @@
# define VM_INSN_INFO_TABLE_IMPL 2
#endif
+/*
+ * track a per thread memory allocations
+ */
+#ifndef THREAD_TRACE_MEMORY_ALLOCATIONS
+# define THREAD_TRACE_MEMORY_ALLOCATIONS 1
+#endif
+
#if defined(NSIG_MAX) /* POSIX issue 8 */
# undef NSIG
# define NSIG NSIG_MAX
@@ -663,6 +670,7 @@ typedef struct rb_vm_struct {
unsigned int thread_abort_on_exception: 1;
unsigned int thread_report_on_exception: 1;
unsigned int thread_ignore_deadlock: 1;
+ unsigned int thread_trace_memory_allocations: 1;
/* object management */
VALUE mark_object_ary;
@@ -1048,6 +1056,14 @@ typedef struct rb_thread_struct {
struct rb_waiting_list *join_list;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ struct {
+ size_t total_allocated_objects;
+ size_t total_malloc_bytes;
+ size_t total_mallocs;
+ } memory_allocations;
+#endif
+
union {
struct {
VALUE proc;
@@ -1973,6 +1989,7 @@ void rb_threadptr_interrupt(rb_thread_t *th);
void rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th);
void rb_threadptr_pending_interrupt_clear(rb_thread_t *th);
void rb_threadptr_pending_interrupt_enque(rb_thread_t *th, VALUE v);
+rb_thread_t *ruby_threadptr_for_trace_memory_allocations(void);
VALUE rb_ec_get_errinfo(const rb_execution_context_t *ec);
void rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo);
void rb_execution_context_update(const rb_execution_context_t *ec);
--
2.39.2
From 431c627d3af67fb664b6285720ef3a2ab53c6030 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Tue, 22 Dec 2020 15:33:08 +0100
Subject: [PATCH] Expose `Thread#memory_allocations` counters
This provides currently a per-thread GC heap slots
and malloc allocations statistics.
This is designed to measure a memory allocations
in a multi-threaded environments (concurrent requests
processing) with an accurate information about allocated
memory within a given execution context.
Example: Measure memory pressure generated by a given
requests to easier find requests with a lot of allocations.
Ref: https://gitlab.com/gitlab-org/gitlab/-/issues/296530
---
gc.c | 20 ++++++
.../test_thread_trace_memory_allocations.rb | 67 +++++++++++++++++++
thread.c | 55 +++++++++++++++
vm_core.h | 17 +++++
4 files changed, 159 insertions(+)
create mode 100644 test/ruby/test_thread_trace_memory_allocations.rb
diff --git a/gc.c b/gc.c
index 54400e4f6b..9fa9bf35d9 100644
--- a/gc.c
+++ b/gc.c
@@ -2541,6 +2541,13 @@ newobj_init(VALUE klass, VALUE flags, int wb_protected, rb_objspace_t *objspace,
// TODO: make it atomic, or ractor local
objspace->total_allocated_objects++;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_allocated_objects);
+ }
+#endif
+
#if RGENGC_PROFILE
if (wb_protected) {
objspace->profile.total_generated_normal_object_count++;
@@ -12130,6 +12137,19 @@ objspace_malloc_increase_body(rb_objspace_t *objspace, void *mem, size_t new_siz
#endif
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_thread_t *th = ruby_threadptr_for_trace_memory_allocations();
+ if (th) {
+ if (new_size > old_size) {
+ ATOMIC_SIZE_ADD(th->memory_allocations.total_malloc_bytes, new_size - old_size);
+ }
+
+ if (type == MEMOP_TYPE_MALLOC) {
+ ATOMIC_SIZE_INC(th->memory_allocations.total_mallocs);
+ }
+ }
+#endif
+
if (type == MEMOP_TYPE_MALLOC) {
retry:
if (malloc_increase > malloc_limit && ruby_native_thread_p() && !dont_gc_val()) {
diff --git a/test/ruby/test_thread_trace_memory_allocations.rb b/test/ruby/test_thread_trace_memory_allocations.rb
new file mode 100644
index 0000000000..2e28151357
--- /dev/null
+++ b/test/ruby/test_thread_trace_memory_allocations.rb
@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require 'test/unit'
+
+class TestThreadTraceMemoryAllocations < Test::Unit::TestCase
+ def test_disabled_trace_memory_allocations
+ Thread.trace_memory_allocations = false
+
+ assert_predicate Thread.current.memory_allocations, :nil?
+ end
+
+ def test_enabled_trace_memory_allocations
+ Thread.trace_memory_allocations = true
+
+ assert_not_nil(Thread.current.memory_allocations)
+ end
+
+ def test_only_this_thread_allocations_are_counted
+ changed = {
+ total_allocated_objects: 1000,
+ total_malloc_bytes: 1_000_000,
+ total_mallocs: 100
+ }
+
+ Thread.trace_memory_allocations = true
+
+ assert_less_than(changed) do
+ Thread.new do
+ assert_greater_than(changed) do
+ # This will allocate: 5k objects, 5k mallocs, 5MB
+ allocate(5000, 1000)
+ end
+ end.join
+
+ # This will allocate: 50 objects, 50 mallocs, 500 bytes
+ allocate(50, 10)
+ end
+ end
+
+ private
+
+ def allocate(slots, bytes)
+ Array.new(slots).map do
+ '0' * bytes
+ end
+ end
+
+ def assert_greater_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :<=, after[key]-before[key], "expected the #{key} to change more than #{by}")
+ end
+ end
+
+ def assert_less_than(keys)
+ before = Thread.current.memory_allocations
+ yield
+ after = Thread.current.memory_allocations
+
+ keys.each do |key, by|
+ assert_operator(by, :>, after[key]-before[key], "expected the #{key} to change less than #{by}")
+ end
+ end
+end
diff --git a/thread.c b/thread.c
index 56a5644552..176509e1be 100644
--- a/thread.c
+++ b/thread.c
@@ -5289,6 +5289,55 @@ Init_Thread_Mutex(void)
rb_native_mutex_initialize(&th->interrupt_lock);
}
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+rb_thread_t *
+ruby_threadptr_for_trace_memory_allocations(void)
+{
+ // The order of this checks is important due
+ // to how Ruby VM is initialized
+ if (GET_VM()->thread_trace_memory_allocations && GET_EC() != NULL) {
+ return GET_THREAD();
+ }
+
+ return NULL;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations(VALUE _)
+{
+ return GET_THREAD()->vm->thread_trace_memory_allocations ? Qtrue : Qfalse;
+}
+
+static VALUE
+rb_thread_s_trace_memory_allocations_set(VALUE self, VALUE val)
+{
+ GET_THREAD()->vm->thread_trace_memory_allocations = RTEST(val);
+ return val;
+}
+
+static VALUE
+rb_thread_memory_allocations(VALUE self)
+{
+ rb_thread_t *th = rb_thread_ptr(self);
+
+ if (!th->vm->thread_trace_memory_allocations) {
+ return Qnil;
+ }
+
+ VALUE ret = rb_hash_new();
+
+ VALUE total_allocated_objects = ID2SYM(rb_intern_const("total_allocated_objects"));
+ VALUE total_malloc_bytes = ID2SYM(rb_intern_const("total_malloc_bytes"));
+ VALUE total_mallocs = ID2SYM(rb_intern_const("total_mallocs"));
+
+ rb_hash_aset(ret, total_allocated_objects, SIZET2NUM(th->memory_allocations.total_allocated_objects));
+ rb_hash_aset(ret, total_malloc_bytes, SIZET2NUM(th->memory_allocations.total_malloc_bytes));
+ rb_hash_aset(ret, total_mallocs, SIZET2NUM(th->memory_allocations.total_mallocs));
+
+ return ret;
+}
+#endif
+
/*
* Document-class: ThreadError
*
@@ -5371,6 +5420,12 @@ Init_Thread(void)
rb_define_method(rb_cThread, "to_s", rb_thread_to_s, 0);
rb_define_alias(rb_cThread, "inspect", "to_s");
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations", rb_thread_s_trace_memory_allocations, 0);
+ rb_define_singleton_method(rb_cThread, "trace_memory_allocations=", rb_thread_s_trace_memory_allocations_set, 1);
+ rb_define_method(rb_cThread, "memory_allocations", rb_thread_memory_allocations, 0);
+#endif
+
rb_vm_register_special_exception(ruby_error_stream_closed, rb_eIOError,
"stream closed in another thread");
diff --git a/vm_core.h b/vm_core.h
index d86fdbaecd..ecd17fb6f4 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -114,6 +114,13 @@ extern int ruby_assert_critical_section_entered;
# define VM_INSN_INFO_TABLE_IMPL 2
#endif
+/*
+ * track a per thread memory allocations
+ */
+#ifndef THREAD_TRACE_MEMORY_ALLOCATIONS
+# define THREAD_TRACE_MEMORY_ALLOCATIONS 1
+#endif
+
#if defined(NSIG_MAX) /* POSIX issue 8 */
# undef NSIG
# define NSIG NSIG_MAX
@@ -661,6 +668,7 @@ typedef struct rb_vm_struct {
unsigned int thread_abort_on_exception: 1;
unsigned int thread_report_on_exception: 1;
unsigned int thread_ignore_deadlock: 1;
+ unsigned int thread_trace_memory_allocations: 1;
/* object management */
VALUE mark_object_ary;
@@ -1040,6 +1048,14 @@ typedef struct rb_thread_struct {
struct rb_waiting_list *join_list;
+#if THREAD_TRACE_MEMORY_ALLOCATIONS
+ struct {
+ size_t total_allocated_objects;
+ size_t total_malloc_bytes;
+ size_t total_mallocs;
+ } memory_allocations;
+#endif
+
union {
struct {
VALUE proc;
@@ -1968,6 +1984,7 @@ void rb_threadptr_interrupt(rb_thread_t *th);
void rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th);
void rb_threadptr_pending_interrupt_clear(rb_thread_t *th);
void rb_threadptr_pending_interrupt_enque(rb_thread_t *th, VALUE v);
+rb_thread_t *ruby_threadptr_for_trace_memory_allocations(void);
VALUE rb_ec_get_errinfo(const rb_execution_context_t *ec);
void rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo);
void rb_execution_context_update(rb_execution_context_t *ec);
--
2.39.2
diff --git a/LICENSE.txt b/LICENSE.txt
new file mode 100644
index 0000000..8a0a51d
--- /dev/null
+++ b/LICENSE.txt
@@ -0,0 +1,54 @@
+RubyGems is copyrighted free software by Chad Fowler, Rich Kilmer, Jim
+Weirich and others. You can redistribute it and/or modify it under
+either the terms of the MIT license (see the file MIT.txt), or the
+conditions below:
+
+1. You may make and give away verbatim copies of the source form of the
+ software without restriction, provided that you duplicate all of the
+ original copyright notices and associated disclaimers.
+
+2. You may modify your copy of the software in any way, provided that
+ you do at least ONE of the following:
+
+ a. place your modifications in the Public Domain or otherwise
+ make them Freely Available, such as by posting said
+ modifications to Usenet or an equivalent medium, or by allowing
+ the author to include your modifications in the software.
+
+ b. use the modified software only within your corporation or
+ organization.
+
+ c. give non-standard executables non-standard names, with
+ instructions on where to get the original software distribution.
+
+ d. make other distribution arrangements with the author.
+
+3. You may distribute the software in object code or executable
+ form, provided that you do at least ONE of the following:
+
+ a. distribute the executables and library files of the software,
+ together with instructions (in the manual page or equivalent)
+ on where to get the original distribution.
+
+ b. accompany the distribution with the machine-readable source of
+ the software.
+
+ c. give non-standard executables non-standard names, with
+ instructions on where to get the original software distribution.
+
+ d. make other distribution arrangements with the author.
+
+4. You may modify and include the part of the software into any other
+ software (possibly commercial).
+
+5. The scripts and library files supplied as input to or produced as
+ output from the software do not automatically fall under the
+ copyright of the software, but belong to whomever generated them,
+ and may be sold commercially, and may be aggregated with this
+ software.
+
+6. THIS SOFTWARE IS PROVIDED "AS IS" AND WITHOUT ANY EXPRESS OR
+ IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
+ WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ PURPOSE.
+
diff --git a/sv.c b/sv.c
index 0125795..b79dc4f 100644
--- a/sv.c
+++ b/sv.c
@@ -167,7 +167,7 @@ int status(char *unused) {
}
else {
outs("; ");
- if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
+ if (svstatus_get()) { svstatus_print("log"); outs("\n"); }
}
islog =0;
flush("");
diff --git a/unzip.c b/unzip.c
index 8dbfc95..954b204 100644
--- a/unzip.c
+++ b/unzip.c
@@ -570,8 +570,11 @@ Send bug reports using //www.info-zip.org/zip-bug.html; see README for details.\
#else /* !VMS */
# ifdef COPYRIGHT_CLEAN
static ZCONST char Far UnzipUsageLine1[] = "\
-UnZip %d.%d%d%s of %s, by Info-ZIP. Maintained by C. Spieler. Send\n\
-bug reports using http://www.info-zip.org/zip-bug.html; see README for details.\
+UnZip %d.%d%d%s of %s, by GitLab Inc. The original UnZip sources are available\n\
+from Info-ZIP's home site at http://www.info-zip.org/pub/infozip/UnZip.html.\n\
+Our patches are available from\n\
+https://gitlab.com/gitlab-org/omnibus-gitlab/tree/master/config/patches/unzip.\n\
+Send any bug reports on this port to support@gitlab.com.\
\n\n";
# else
static ZCONST char Far UnzipUsageLine1[] = "\
From: Santiago Vila <sanvila@debian.org>
Subject: In Debian, manpages are in section 1, not in section 1L
X-Debian-version: 5.52-3
--- a/man/funzip.1
+++ b/man/funzip.1
@@ -20,7 +20,7 @@
.in -4n
..
.\" =========================================================================
-.TH FUNZIP 1L "20 April 2009 (v3.95)" "Info-ZIP"
+.TH FUNZIP 1 "20 April 2009 (v3.95)" "Info-ZIP"
.SH NAME
funzip \- filter for extracting from a ZIP archive in a pipe
.PD
@@ -78,7 +78,7 @@
.EE
.PP
To use \fIzip\fP and \fIfunzip\fP in place of \fIcompress\fP(1) and
-\fIzcat\fP(1) (or \fIgzip\fP(1L) and \fIgzcat\fP(1L)) for tape backups:
+\fIzcat\fP(1) (or \fIgzip\fP(1) and \fIgzcat\fP(1)) for tape backups:
.PP
.EX
tar cf \- . | zip \-7 | dd of=/dev/nrst0 obs=8k
@@ -108,8 +108,8 @@
.PD
.\" =========================================================================
.SH "SEE ALSO"
-\fIgzip\fP(1L), \fIunzip\fP(1L), \fIunzipsfx\fP(1L), \fIzip\fP(1L),
-\fIzipcloak\fP(1L), \fIzipinfo\fP(1L), \fIzipnote\fP(1L), \fIzipsplit\fP(1L)
+\fIgzip\fP(1), \fIunzip\fP(1), \fIunzipsfx\fP(1), \fIzip\fP(1),
+\fIzipcloak\fP(1), \fIzipinfo\fP(1), \fIzipnote\fP(1), \fIzipsplit\fP(1)
.PD
.\" =========================================================================
.SH URL
--- a/man/unzip.1
+++ b/man/unzip.1
@@ -20,7 +20,7 @@
.in -4n
..
.\" =========================================================================
-.TH UNZIP 1L "20 April 2009 (v6.0)" "Info-ZIP"
+.TH UNZIP 1 "20 April 2009 (v6.0)" "Info-ZIP"
.SH NAME
unzip \- list, test and extract compressed files in a ZIP archive
.PD
@@ -34,7 +34,7 @@
\fIunzip\fP will list, test, or extract files from a ZIP archive, commonly
found on MS-DOS systems. The default behavior (with no options) is to extract
into the current directory (and subdirectories below it) all files from the
-specified ZIP archive. A companion program, \fIzip\fP(1L), creates ZIP
+specified ZIP archive. A companion program, \fIzip\fP(1), creates ZIP
archives; both programs are compatible with archives created by PKWARE's
\fIPKZIP\fP and \fIPKUNZIP\fP for MS-DOS, but in many cases the program
options or default behaviors differ.
@@ -105,8 +105,8 @@
list of all possible flags. The exhaustive list follows:
.TP
.B \-Z
-\fIzipinfo\fP(1L) mode. If the first option on the command line is \fB\-Z\fP,
-the remaining options are taken to be \fIzipinfo\fP(1L) options. See the
+\fIzipinfo\fP(1) mode. If the first option on the command line is \fB\-Z\fP,
+the remaining options are taken to be \fIzipinfo\fP(1) options. See the
appropriate manual page for a description of these options.
.TP
.B \-A
@@ -178,7 +178,7 @@
compressed size and compression ratio figures are independent of the entry's
encryption status and show the correct compression performance. (The complete
size of the encrypted compressed data stream for zipfile entries is reported
-by the more verbose \fIzipinfo\fP(1L) reports, see the separate manual.)
+by the more verbose \fIzipinfo\fP(1) reports, see the separate manual.)
When no zipfile is specified (that is, the complete command is simply
``\fCunzip \-v\fR''), a diagnostic screen is printed. In addition to
the normal header with release date and version, \fIunzip\fP lists the
@@ -379,8 +379,8 @@
.TP
.B \-N
[Amiga] extract file comments as Amiga filenotes. File comments are created
-with the \-c option of \fIzip\fP(1L), or with the \-N option of the Amiga port
-of \fIzip\fP(1L), which stores filenotes as comments.
+with the \-c option of \fIzip\fP(1), or with the \-N option of the Amiga port
+of \fIzip\fP(1), which stores filenotes as comments.
.TP
.B \-o
overwrite existing files without prompting. This is a dangerous option, so
@@ -598,7 +598,7 @@
As suggested by the examples above, the default variable names are UNZIP_OPTS
for VMS (where the symbol used to install \fIunzip\fP as a foreign command
would otherwise be confused with the environment variable), and UNZIP
-for all other operating systems. For compatibility with \fIzip\fP(1L),
+for all other operating systems. For compatibility with \fIzip\fP(1),
UNZIPOPT is also accepted (don't ask). If both UNZIP and UNZIPOPT
are defined, however, UNZIP takes precedence. \fIunzip\fP's diagnostic
option (\fB\-v\fP with no zipfile name) can be used to check the values
@@ -648,8 +648,8 @@
a password is not known, entering a null password (that is, just a carriage
return or ``Enter'') is taken as a signal to skip all further prompting.
Only unencrypted files in the archive(s) will thereafter be extracted. (In
-fact, that's not quite true; older versions of \fIzip\fP(1L) and
-\fIzipcloak\fP(1L) allowed null passwords, so \fIunzip\fP checks each encrypted
+fact, that's not quite true; older versions of \fIzip\fP(1) and
+\fIzipcloak\fP(1) allowed null passwords, so \fIunzip\fP checks each encrypted
file to see if the null password works. This may result in ``false positives''
and extraction errors, as noted above.)
.PP
@@ -943,8 +943,8 @@
.PD
.\" =========================================================================
.SH "SEE ALSO"
-\fIfunzip\fP(1L), \fIzip\fP(1L), \fIzipcloak\fP(1L), \fIzipgrep\fP(1L),
-\fIzipinfo\fP(1L), \fIzipnote\fP(1L), \fIzipsplit\fP(1L)
+\fIfunzip\fP(1), \fIzip\fP(1), \fIzipcloak\fP(1), \fIzipgrep\fP(1),
+\fIzipinfo\fP(1), \fIzipnote\fP(1), \fIzipsplit\fP(1)
.PD
.\" =========================================================================
.SH URL
--- a/man/unzipsfx.1
+++ b/man/unzipsfx.1
@@ -20,7 +20,7 @@
.in -4n
..
.\" =========================================================================
-.TH UNZIPSFX 1L "20 April 2009 (v6.0)" "Info-ZIP"
+.TH UNZIPSFX 1 "20 April 2009 (v6.0)" "Info-ZIP"
.SH NAME
unzipsfx \- self-extracting stub for prepending to ZIP archives
.PD
@@ -30,7 +30,7 @@
.PD
.\" =========================================================================
.SH DESCRIPTION
-\fIunzipsfx\fP is a modified version of \fIunzip\fP(1L) designed to be
+\fIunzipsfx\fP is a modified version of \fIunzip\fP(1) designed to be
prepended to existing ZIP archives in order to form self-extracting archives.
Instead of taking its first non-flag argument to be the zipfile(s) to be
extracted, \fIunzipsfx\fP seeks itself under the name by which it was invoked
@@ -109,7 +109,7 @@
.PD
.\" =========================================================================
.SH OPTIONS
-\fIunzipsfx\fP supports the following \fIunzip\fP(1L) options: \fB\-c\fP
+\fIunzipsfx\fP supports the following \fIunzip\fP(1) options: \fB\-c\fP
and \fB\-p\fP (extract to standard output/screen), \fB\-f\fP and \fB\-u\fP
(freshen and update existing files upon extraction), \fB\-t\fP (test
archive) and \fB\-z\fP (print archive comment). All normal listing options
@@ -118,11 +118,11 @@
those creating self-extracting archives may wish to include a short listing
in the zipfile comment.
.PP
-See \fIunzip\fP(1L) for a more complete description of these options.
+See \fIunzip\fP(1) for a more complete description of these options.
.PD
.\" =========================================================================
.SH MODIFIERS
-\fIunzipsfx\fP currently supports all \fIunzip\fP(1L) modifiers: \fB\-a\fP
+\fIunzipsfx\fP currently supports all \fIunzip\fP(1) modifiers: \fB\-a\fP
(convert text files), \fB\-n\fP (never overwrite), \fB\-o\fP (overwrite
without prompting), \fB\-q\fP (operate quietly), \fB\-C\fP (match names
case-insensitively), \fB\-L\fP (convert uppercase-OS names to lowercase),
@@ -137,18 +137,18 @@
of course continue to be supported since the zipfile format implies ASCII
storage of text files.)
.PP
-See \fIunzip\fP(1L) for a more complete description of these modifiers.
+See \fIunzip\fP(1) for a more complete description of these modifiers.
.PD
.\" =========================================================================
.SH "ENVIRONMENT OPTIONS"
-\fIunzipsfx\fP uses the same environment variables as \fIunzip\fP(1L) does,
+\fIunzipsfx\fP uses the same environment variables as \fIunzip\fP(1) does,
although this is likely to be an issue only for the person creating and
-testing the self-extracting archive. See \fIunzip\fP(1L) for details.
+testing the self-extracting archive. See \fIunzip\fP(1) for details.
.PD
.\" =========================================================================
.SH DECRYPTION
-Decryption is supported exactly as in \fIunzip\fP(1L); that is, interactively
-with a non-echoing prompt for the password(s). See \fIunzip\fP(1L) for
+Decryption is supported exactly as in \fIunzip\fP(1); that is, interactively
+with a non-echoing prompt for the password(s). See \fIunzip\fP(1) for
details. Once again, note that if the archive has no encrypted files there
is no reason to use a version of \fIunzipsfx\fP with decryption support;
that only adds to the size of the archive.
@@ -286,7 +286,7 @@
from anywhere in the user's path. The situation is not known for AmigaDOS,
Atari TOS, MacOS, etc.
.PP
-As noted above, a number of the normal \fIunzip\fP(1L) functions have
+As noted above, a number of the normal \fIunzip\fP(1) functions have
been removed in order to make \fIunzipsfx\fP smaller: usage and diagnostic
info, listing functions and extraction to other directories. Also, only
stored and deflated files are supported. The latter limitation is mainly
@@ -303,17 +303,17 @@
defined as a ``debug hunk.'') There may be compatibility problems between
the ROM levels of older Amigas and newer ones.
.PP
-All current bugs in \fIunzip\fP(1L) exist in \fIunzipsfx\fP as well.
+All current bugs in \fIunzip\fP(1) exist in \fIunzipsfx\fP as well.
.PD
.\" =========================================================================
.SH DIAGNOSTICS
\fIunzipsfx\fP's exit status (error level) is identical to that of
-\fIunzip\fP(1L); see the corresponding man page.
+\fIunzip\fP(1); see the corresponding man page.
.PD
.\" =========================================================================
.SH "SEE ALSO"
-\fIfunzip\fP(1L), \fIunzip\fP(1L), \fIzip\fP(1L), \fIzipcloak\fP(1L),
-\fIzipgrep\fP(1L), \fIzipinfo\fP(1L), \fIzipnote\fP(1L), \fIzipsplit\fP(1L)
+\fIfunzip\fP(1), \fIunzip\fP(1), \fIzip\fP(1), \fIzipcloak\fP(1),
+\fIzipgrep\fP(1), \fIzipinfo\fP(1), \fIzipnote\fP(1), \fIzipsplit\fP(1)
.PD
.PD
.\" =========================================================================
@@ -330,7 +330,7 @@
.\" =========================================================================
.SH AUTHORS
Greg Roelofs was responsible for the basic modifications to UnZip necessary
-to create UnZipSFX. See \fIunzip\fP(1L) for the current list of Zip-Bugs
+to create UnZipSFX. See \fIunzip\fP(1) for the current list of Zip-Bugs
authors, or the file CONTRIBS in the UnZip source distribution for the
full list of Info-ZIP contributors.
.PD
--- a/man/zipgrep.1
+++ b/man/zipgrep.1
@@ -8,7 +8,7 @@
.\" zipgrep.1 by Greg Roelofs.
.\"
.\" =========================================================================
-.TH ZIPGREP 1L "20 April 2009" "Info-ZIP"
+.TH ZIPGREP 1 "20 April 2009" "Info-ZIP"
.SH NAME
zipgrep \- search files in a ZIP archive for lines matching a pattern
.PD
@@ -21,7 +21,7 @@
.SH DESCRIPTION
\fIzipgrep\fP will search files within a ZIP archive for lines matching
the given string or pattern. \fIzipgrep\fP is a shell script and requires
-\fIegrep\fP(1) and \fIunzip\fP(1L) to function. Its output is identical to
+\fIegrep\fP(1) and \fIunzip\fP(1) to function. Its output is identical to
that of \fIegrep\fP(1).
.PD
.\" =========================================================================
@@ -69,8 +69,8 @@
.PD
.\" =========================================================================
.SH "SEE ALSO"
-\fIegrep\fP(1), \fIunzip\fP(1L), \fIzip\fP(1L), \fIfunzip\fP(1L),
-\fIzipcloak\fP(1L), \fIzipinfo\fP(1L), \fIzipnote\fP(1L), \fIzipsplit\fP(1L)
+\fIegrep\fP(1), \fIunzip\fP(1), \fIzip\fP(1), \fIfunzip\fP(1),
+\fIzipcloak\fP(1), \fIzipinfo\fP(1), \fIzipnote\fP(1), \fIzipsplit\fP(1)
.PD
.\" =========================================================================
.SH URL
--- a/man/zipinfo.1
+++ b/man/zipinfo.1
@@ -34,7 +34,7 @@
.in -4n
..
.\" =========================================================================
-.TH ZIPINFO 1L "20 April 2009 (v3.0)" "Info-ZIP"
+.TH ZIPINFO 1 "20 April 2009 (v3.0)" "Info-ZIP"
.SH NAME
zipinfo \- list detailed information about a ZIP archive
.PD
@@ -272,7 +272,7 @@
Note that because of limitations in the MS-DOS format used to store file
times, the seconds field is always rounded to the nearest even second.
For Unix files this is expected to change in the next major releases of
-\fIzip\fP(1L) and \fIunzip\fP.
+\fIzip\fP(1) and \fIunzip\fP.
.PP
In addition to individual file information, a default zipfile listing
also includes header and trailer lines:
@@ -361,7 +361,7 @@
As suggested above, the default variable names are ZIPINFO_OPTS for VMS
(where the symbol used to install \fIzipinfo\fP as a foreign command
would otherwise be confused with the environment variable), and ZIPINFO
-for all other operating systems. For compatibility with \fIzip\fP(1L),
+for all other operating systems. For compatibility with \fIzip\fP(1),
ZIPINFOOPT is also accepted (don't ask). If both ZIPINFO and ZIPINFOOPT
are defined, however, ZIPINFO takes precedence. \fIunzip\fP's diagnostic
option (\fB\-v\fP with no zipfile name) can be used to check the values
@@ -496,8 +496,8 @@
.PP
.\" =========================================================================
.SH "SEE ALSO"
-\fIls\fP(1), \fIfunzip\fP(1L), \fIunzip\fP(1L), \fIunzipsfx\fP(1L),
-\fIzip\fP(1L), \fIzipcloak\fP(1L), \fIzipnote\fP(1L), \fIzipsplit\fP(1L)
+\fIls\fP(1), \fIfunzip\fP(1), \fIunzip\fP(1), \fIunzipsfx\fP(1),
+\fIzip\fP(1), \fIzipcloak\fP(1), \fIzipnote\fP(1), \fIzipsplit\fP(1)
.PD
.\" =========================================================================
.SH URL
From: Aurelien Jarno <aurel32@debian.org>
Subject: #include <unistd.h> for kFreeBSD
Bug-Debian: https://bugs.debian.org/340693
X-Debian-version: 5.52-8
--- a/unix/unxcfg.h
+++ b/unix/unxcfg.h
@@ -52,6 +52,7 @@
#include <sys/types.h> /* off_t, time_t, dev_t, ... */
#include <sys/stat.h>
+#include <unistd.h>
#ifdef NO_OFF_T
typedef long zoff_t;
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Handle the PKWare verification bit of internal attributes
Bug-Debian: https://bugs.debian.org/630078
X-Debian-version: 6.0-5
--- a/process.c
+++ b/process.c
@@ -1729,6 +1729,13 @@
else if (uO.L_flag > 1) /* let -LL force lower case for all names */
G.pInfo->lcflag = 1;
+ /* Handle the PKWare verification bit, bit 2 (0x0004) of internal
+ attributes. If this is set, then a verification checksum is in the
+ first 3 bytes of the external attributes. In this case all we can use
+ for setting file attributes is the last external attributes byte. */
+ if (G.crec.internal_file_attributes & 0x0004)
+ G.crec.external_file_attributes &= (ulg)0xff;
+
/* do Amigas (AMIGA_) also have volume labels? */
if (IS_VOLID(G.crec.external_file_attributes) &&
(G.pInfo->hostnum == FS_FAT_ || G.pInfo->hostnum == FS_HPFS_ ||
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Restore uid and gid information when requested
Bug-Debian: https://bugs.debian.org/689212
X-Debian-version: 6.0-8
--- a/process.c
+++ b/process.c
@@ -2904,7 +2904,7 @@
#ifdef IZ_HAVE_UXUIDGID
if (eb_len >= EB_UX3_MINLEN
&& z_uidgid != NULL
- && (*((EB_HEADSIZE + 0) + ef_buf) == 1)
+ && (*((EB_HEADSIZE + 0) + ef_buf) == 1))
/* only know about version 1 */
{
uch uid_size;
@@ -2916,10 +2916,10 @@
flags &= ~0x0ff; /* ignore any previous UNIX field */
if ( read_ux3_value((EB_HEADSIZE + 2) + ef_buf,
- uid_size, z_uidgid[0])
+ uid_size, &z_uidgid[0])
&&
read_ux3_value((EB_HEADSIZE + uid_size + 3) + ef_buf,
- gid_size, z_uidgid[1]) )
+ gid_size, &z_uidgid[1]) )
{
flags |= EB_UX2_VALID; /* signal success */
}
From: Andreas Schwab <schwab@linux-m68k.org>
Subject: Initialize the symlink flag
Bug-Debian: https://bugs.debian.org/717029
X-Debian-version: 6.0-10
--- a/process.c
+++ b/process.c
@@ -1758,6 +1758,12 @@
= (G.crec.general_purpose_bit_flag & (1 << 11)) == (1 << 11);
#endif
+#ifdef SYMLINKS
+ /* Initialize the symlink flag, may be set by the platform-specific
+ mapattr function. */
+ G.pInfo->symlink = 0;
+#endif
+
return PK_COOL;
} /* end function process_cdir_file_hdr() */
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Increase size of cfactorstr array to avoid buffer overflow
Bug-Debian: https://bugs.debian.org/741384
X-Debian-version: 6.0-11
--- a/list.c
+++ b/list.c
@@ -97,7 +97,7 @@
{
int do_this_file=FALSE, cfactor, error, error_in_archive=PK_COOL;
#ifndef WINDLL
- char sgn, cfactorstr[10];
+ char sgn, cfactorstr[12];
int longhdr=(uO.vflag>1);
#endif
int date_format;
From: Santiago Vila <sanvila@debian.org>
Subject: zipinfo.c: Do not crash when hostver byte is >= 100
--- a/zipinfo.c
+++ b/zipinfo.c
@@ -2114,7 +2114,7 @@
else
attribs[9] = (xattr & UNX_ISVTX)? 'T' : '-'; /* T==undefined */
- sprintf(&attribs[12], "%u.%u", hostver/10, hostver%10);
+ sprintf(&attribs[11], "%2u.%u", hostver/10, hostver%10);
break;
} /* end switch (hostnum: external attributes format) */
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Fix CVE-2014-8139: CRC32 verification heap-based overflow
Bug-Debian: https://bugs.debian.org/773722
--- a/extract.c
+++ b/extract.c
@@ -1,5 +1,5 @@
/*
- Copyright (c) 1990-2009 Info-ZIP. All rights reserved.
+ Copyright (c) 1990-2014 Info-ZIP. All rights reserved.
See the accompanying file LICENSE, version 2009-Jan-02 or later
(the contents of which are also included in unzip.h) for terms of use.
@@ -298,6 +298,8 @@
#ifndef SFX
static ZCONST char Far InconsistEFlength[] = "bad extra-field entry:\n \
EF block length (%u bytes) exceeds remaining EF data (%u bytes)\n";
+ static ZCONST char Far TooSmallEBlength[] = "bad extra-field entry:\n \
+ EF block length (%u bytes) invalid (< %d)\n";
static ZCONST char Far InvalidComprDataEAs[] =
" invalid compressed data for EAs\n";
# if (defined(WIN32) && defined(NTSD_EAS))
@@ -2023,7 +2025,8 @@
ebID = makeword(ef);
ebLen = (unsigned)makeword(ef+EB_LEN);
- if (ebLen > (ef_len - EB_HEADSIZE)) {
+ if (ebLen > (ef_len - EB_HEADSIZE))
+ {
/* Discovered some extra field inconsistency! */
if (uO.qflag)
Info(slide, 1, ((char *)slide, "%-22s ",
@@ -2158,11 +2161,19 @@
}
break;
case EF_PKVMS:
- if (makelong(ef+EB_HEADSIZE) !=
+ if (ebLen < 4)
+ {
+ Info(slide, 1,
+ ((char *)slide, LoadFarString(TooSmallEBlength),
+ ebLen, 4));
+ }
+ else if (makelong(ef+EB_HEADSIZE) !=
crc32(CRCVAL_INITIAL, ef+(EB_HEADSIZE+4),
(extent)(ebLen-4)))
+ {
Info(slide, 1, ((char *)slide,
LoadFarString(BadCRC_EAs)));
+ }
break;
case EF_PKW32:
case EF_PKUNIX:
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Fix CVE-2014-8140: out-of-bounds write issue in test_compr_eb()
Bug-Debian: https://bugs.debian.org/773722
--- a/extract.c
+++ b/extract.c
@@ -2232,10 +2232,17 @@
if (compr_offset < 4) /* field is not compressed: */
return PK_OK; /* do nothing and signal OK */
+ /* Return no/bad-data error status if any problem is found:
+ * 1. eb_size is too small to hold the uncompressed size
+ * (eb_ucsize). (Else extract eb_ucsize.)
+ * 2. eb_ucsize is zero (invalid). 2014-12-04 SMS.
+ * 3. eb_ucsize is positive, but eb_size is too small to hold
+ * the compressed data header.
+ */
if ((eb_size < (EB_UCSIZE_P + 4)) ||
- ((eb_ucsize = makelong(eb+(EB_HEADSIZE+EB_UCSIZE_P))) > 0L &&
- eb_size <= (compr_offset + EB_CMPRHEADLEN)))
- return IZ_EF_TRUNC; /* no compressed data! */
+ ((eb_ucsize = makelong( eb+ (EB_HEADSIZE+ EB_UCSIZE_P))) == 0L) ||
+ ((eb_ucsize > 0L) && (eb_size <= (compr_offset + EB_CMPRHEADLEN))))
+ return IZ_EF_TRUNC; /* no/bad compressed data! */
if (
#ifdef INT_16BIT
From: "Steven M. Schweda" <sms@antinode.info>
Subject: Fix CVE-2014-8141: out-of-bounds read issues in getZip64Data()
Bug-Debian: https://bugs.debian.org/773722
--- a/fileio.c
+++ b/fileio.c
@@ -176,6 +176,8 @@
#endif
static ZCONST char Far ExtraFieldTooLong[] =
"warning: extra field too long (%d). Ignoring...\n";
+static ZCONST char Far ExtraFieldCorrupt[] =
+ "warning: extra field (type: 0x%04x) corrupt. Continuing...\n";
#ifdef WINDLL
static ZCONST char Far DiskFullQuery[] =
@@ -2295,7 +2297,12 @@
if (readbuf(__G__ (char *)G.extra_field, length) == 0)
return PK_EOF;
/* Looks like here is where extra fields are read */
- getZip64Data(__G__ G.extra_field, length);
+ if (getZip64Data(__G__ G.extra_field, length) != PK_COOL)
+ {
+ Info(slide, 0x401, ((char *)slide,
+ LoadFarString( ExtraFieldCorrupt), EF_PKSZ64));
+ error = PK_WARN;
+ }
#ifdef UNICODE_SUPPORT
G.unipath_filename = NULL;
if (G.UzO.U_flag < 2) {
--- a/process.c
+++ b/process.c
@@ -1,5 +1,5 @@
/*
- Copyright (c) 1990-2009 Info-ZIP. All rights reserved.
+ Copyright (c) 1990-2014 Info-ZIP. All rights reserved.
See the accompanying file LICENSE, version 2009-Jan-02 or later
(the contents of which are also included in unzip.h) for terms of use.
@@ -1901,48 +1901,82 @@
and a 4-byte version of disk start number.
Sets both local header and central header fields. Not terribly clever,
but it means that this procedure is only called in one place.
+
+ 2014-12-05 SMS.
+ Added checks to ensure that enough data are available before calling
+ makeint64() or makelong(). Replaced various sizeof() values with
+ simple ("4" or "8") constants. (The Zip64 structures do not depend
+ on our variable sizes.) Error handling is crude, but we should now
+ stay within the buffer.
---------------------------------------------------------------------------*/
+#define Z64FLGS 0xffff
+#define Z64FLGL 0xffffffff
+
if (ef_len == 0 || ef_buf == NULL)
return PK_COOL;
Trace((stderr,"\ngetZip64Data: scanning extra field of length %u\n",
ef_len));
- while (ef_len >= EB_HEADSIZE) {
+ while (ef_len >= EB_HEADSIZE)
+ {
eb_id = makeword(EB_ID + ef_buf);
eb_len = makeword(EB_LEN + ef_buf);
- if (eb_len > (ef_len - EB_HEADSIZE)) {
- /* discovered some extra field inconsistency! */
+ if (eb_len > (ef_len - EB_HEADSIZE))
+ {
+ /* Extra block length exceeds remaining extra field length. */
Trace((stderr,
"getZip64Data: block length %u > rest ef_size %u\n", eb_len,
ef_len - EB_HEADSIZE));
break;
}
- if (eb_id == EF_PKSZ64) {
-
+ if (eb_id == EF_PKSZ64)
+ {
int offset = EB_HEADSIZE;
- if (G.crec.ucsize == 0xffffffff || G.lrec.ucsize == 0xffffffff){
- G.lrec.ucsize = G.crec.ucsize = makeint64(offset + ef_buf);
- offset += sizeof(G.crec.ucsize);
+ if ((G.crec.ucsize == Z64FLGL) || (G.lrec.ucsize == Z64FLGL))
+ {
+ if (offset+ 8 > ef_len)
+ return PK_ERR;
+
+ G.crec.ucsize = G.lrec.ucsize = makeint64(offset + ef_buf);
+ offset += 8;
}
- if (G.crec.csize == 0xffffffff || G.lrec.csize == 0xffffffff){
- G.csize = G.lrec.csize = G.crec.csize = makeint64(offset + ef_buf);
- offset += sizeof(G.crec.csize);
+
+ if ((G.crec.csize == Z64FLGL) || (G.lrec.csize == Z64FLGL))
+ {
+ if (offset+ 8 > ef_len)
+ return PK_ERR;
+
+ G.csize = G.crec.csize = G.lrec.csize = makeint64(offset + ef_buf);
+ offset += 8;
}
- if (G.crec.relative_offset_local_header == 0xffffffff){
+
+ if (G.crec.relative_offset_local_header == Z64FLGL)
+ {
+ if (offset+ 8 > ef_len)
+ return PK_ERR;
+
G.crec.relative_offset_local_header = makeint64(offset + ef_buf);
- offset += sizeof(G.crec.relative_offset_local_header);
+ offset += 8;
}
- if (G.crec.disk_number_start == 0xffff){
+
+ if (G.crec.disk_number_start == Z64FLGS)
+ {
+ if (offset+ 4 > ef_len)
+ return PK_ERR;
+
G.crec.disk_number_start = (zuvl_t)makelong(offset + ef_buf);
- offset += sizeof(G.crec.disk_number_start);
+ offset += 4;
}
+#if 0
+ break; /* Expect only one EF_PKSZ64 block. */
+#endif /* 0 */
}
- /* Skip this extra field block */
+ /* Skip this extra field block. */
ef_buf += (eb_len + EB_HEADSIZE);
ef_len -= (eb_len + EB_HEADSIZE);
}
From: mancha <mancha1 AT zoho DOT com>
Subject: Info-ZIP UnZip buffer overflow
Bug-Debian: https://bugs.debian.org/776589
By carefully crafting a corrupt ZIP archive with "extra fields" that
purport to have compressed blocks larger than the corresponding
uncompressed blocks in STORED no-compression mode, an attacker can
trigger a heap overflow that can result in application crash or
possibly have other unspecified impact.
This patch ensures that when extra fields use STORED mode, the
"compressed" and uncompressed block sizes match.
--- a/extract.c
+++ b/extract.c
@@ -2228,6 +2228,7 @@
ulg eb_ucsize;
uch *eb_ucptr;
int r;
+ ush eb_compr_method;
if (compr_offset < 4) /* field is not compressed: */
return PK_OK; /* do nothing and signal OK */
@@ -2244,6 +2245,15 @@
((eb_ucsize > 0L) && (eb_size <= (compr_offset + EB_CMPRHEADLEN))))
return IZ_EF_TRUNC; /* no/bad compressed data! */
+ /* 2015-02-10 Mancha(?), Michal Zalewski, Tomas Hoger, SMS.
+ * For STORE method, compressed and uncompressed sizes must agree.
+ * http://www.info-zip.org/phpBB3/viewtopic.php?f=7&t=450
+ */
+ eb_compr_method = makeword( eb + (EB_HEADSIZE + compr_offset));
+ if ((eb_compr_method == STORED) &&
+ (eb_size != compr_offset + EB_CMPRHEADLEN + eb_ucsize))
+ return PK_ERR;
+
if (
#ifdef INT_16BIT
(((ulg)(extent)eb_ucsize) != eb_ucsize) ||
From: Jérémy Bobbio <lunar@debian.org>
Subject: Remove build date
Bug-Debian: https://bugs.debian.org/782851
In order to make unzip build reproducibly, we remove the
(already optional) build date from the binary.
--- a/unix/unix.c
+++ b/unix/unix.c
@@ -1705,7 +1705,7 @@
#endif /* Sun */
#endif /* SGI */
-#ifdef __DATE__
+#if 0
" on ", __DATE__
#else
"", ""
From: Petr Stodulka <pstodulk@redhat.com>
Date: Mon, 14 Sep 2015 18:23:17 +0200
Subject: Upstream fix for heap overflow
Bug-Debian: https://bugs.debian.org/802162
Bug-RedHat: https://bugzilla.redhat.com/show_bug.cgi?id=1260944
Origin: https://bugzilla.redhat.com/attachment.cgi?id=1073002
Forwarded: yes
---
crypt.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
--- a/crypt.c
+++ b/crypt.c
@@ -465,7 +465,17 @@
GLOBAL(pInfo->encrypted) = FALSE;
defer_leftover_input(__G);
for (n = 0; n < RAND_HEAD_LEN; n++) {
- b = NEXTBYTE;
+ /* 2012-11-23 SMS. (OUSPG report.)
+ * Quit early if compressed size < HEAD_LEN. The resulting
+ * error message ("unable to get password") could be improved,
+ * but it's better than trying to read nonexistent data, and
+ * then continuing with a negative G.csize. (See
+ * fileio.c:readbyte()).
+ */
+ if ((b = NEXTBYTE) == (ush)EOF)
+ {
+ return PK_ERR;
+ }
h[n] = (uch)b;
Trace((stdout, " (%02x)", h[n]));
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment