Skip to content
Snippets Groups Projects
Unverified Commit a2f3c3ee authored by van Hauser's avatar van Hauser Committed by GitHub
Browse files

Merge pull request #1478 from AFLplusplus/dev

Push to stable
parents c57988e6 6056d4b1
Branches
Tags
No related merge requests found
Showing
with 214 additions and 57 deletions
......@@ -367,15 +367,18 @@ help:
@echo Known build environment options:
@echo "=========================================="
@echo STATIC - compile AFL++ static
@echo ASAN_BUILD - compiles with memory sanitizer for debug purposes
@echo ASAN_BUILD - compiles AFL++ with memory sanitizer for debug purposes
@echo UBSAN_BUILD - compiles AFL++ tools with undefined behaviour sanitizer for debug purposes
@echo DEBUG - no optimization, -ggdb3, all warnings and -Werror
@echo PROFILING - compile afl-fuzz with profiling information
@echo INTROSPECTION - compile afl-fuzz with mutation introspection
@echo NO_PYTHON - disable python support
@echo NO_SPLICING - disables splicing mutation in afl-fuzz, not recommended for normal fuzzing
@echo NO_NYX - disable building nyx mode dependencies
@echo "NO_CORESIGHT - disable building coresight (arm64 only)"
@echo NO_UNICORN_ARM64 - disable building unicorn on arm64
@echo AFL_NO_X86 - if compiling on non-intel/amd platforms
@echo "LLVM_CONFIG - if your distro doesn't use the standard name for llvm-config (e.g. Debian)"
@echo "LLVM_CONFIG - if your distro doesn't use the standard name for llvm-config (e.g., Debian)"
@echo "=========================================="
@echo e.g.: make ASAN_BUILD=1
......
......@@ -352,7 +352,7 @@ uint8_t afl_custom_queue_get(my_mutator_t *data, const uint8_t *filename) {
* @return if the file contents was modified return 1 (True), 0 (False)
* otherwise
*/
uint8_t afl_custom_queue_new_entry(my_mutator_t * data,
uint8_t afl_custom_queue_new_entry(my_mutator_t *data,
const uint8_t *filename_new_queue,
const uint8_t *filename_orig_queue) {
......
......@@ -72,6 +72,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "alloc-inl.h"
/* Header that must be present at the beginning of every test case: */
......@@ -127,9 +128,11 @@ size_t afl_custom_post_process(post_state_t *data, unsigned char *in_buf,
}
/* Allocate memory for new buffer, reusing previous allocation if
possible. */
possible. Note we have to use afl-fuzz's own realloc!
Note that you should only do this if you need to grow the buffer,
otherwise work with in_buf, and assign it to *out_buf instead. */
*out_buf = realloc(data->buf, len);
*out_buf = afl_realloc(out_buf, len);
/* If we're out of memory, the most graceful thing to do is to return the
original buffer and give up on modifying it. Let AFL handle OOM on its
......@@ -142,9 +145,9 @@ size_t afl_custom_post_process(post_state_t *data, unsigned char *in_buf,
}
/* Copy the original data to the new location. */
memcpy(*out_buf, in_buf, len);
if (len > strlen(HEADER))
memcpy(*out_buf + strlen(HEADER), in_buf + strlen(HEADER),
len - strlen(HEADER));
/* Insert the new header. */
......
......@@ -29,8 +29,8 @@
#include <stdint.h>
#include <string.h>
#include <zlib.h>
#include <arpa/inet.h>
#include "alloc-inl.h"
/* A macro to round an integer up to 4 kB. */
......@@ -70,9 +70,6 @@ size_t afl_custom_post_process(post_state_t *data, const unsigned char *in_buf,
unsigned int len,
const unsigned char **out_buf) {
unsigned char *new_buf = (unsigned char *)in_buf;
unsigned int pos = 8;
/* Don't do anything if there's not enough room for the PNG header
(8 bytes). */
......@@ -83,6 +80,22 @@ size_t afl_custom_post_process(post_state_t *data, const unsigned char *in_buf,
}
/* This is not a good way to do it, if you do not need to grow the buffer
then just work with in_buf instead for speed reasons.
But we want to show how to grow a buffer, so this is how it's done: */
unsigned int pos = 8;
unsigned char *new_buf = afl_realloc(out_buf, UP4K(len));
if (!new_buf) {
*out_buf = in_buf;
return len;
}
memcpy(new_buf, in_buf, len);
/* Minimum size of a zero-length PNG chunk is 12 bytes; if we
don't have that, we can bail out. */
......@@ -111,33 +124,6 @@ size_t afl_custom_post_process(post_state_t *data, const unsigned char *in_buf,
if (real_cksum != file_cksum) {
/* First modification? Make a copy of the input buffer. Round size
up to 4 kB to minimize the number of reallocs needed. */
if (new_buf == in_buf) {
if (len <= data->size) {
new_buf = data->buf;
} else {
new_buf = realloc(data->buf, UP4K(len));
if (!new_buf) {
*out_buf = in_buf;
return len;
}
data->buf = new_buf;
data->size = UP4K(len);
memcpy(new_buf, in_buf, len);
}
}
*(uint32_t *)(new_buf + pos + 8 + chunk_len) = real_cksum;
}
......
......@@ -9,6 +9,12 @@ Want to stay in the loop on major new features? Join our mailing list by
sending a mail to <afl-users+subscribe@googlegroups.com>.
### Version ++4.02a (dev)
- afl-fuzz:
- change post_process hook to allow returning NULL and 0 length to
tell afl-fuzz to skip this mutated input
- afl-cc:
- important fix for the default pcguard mode when LLVM IR vector
selects are produced, thanks to @juppytt for reporting!
- gcc_plugin:
- Adacore submitted CMPLOG support to the gcc_plugin! :-)
- llvm_mode:
......
......@@ -79,19 +79,23 @@ make STATIC=1
These build options exist:
* STATIC - compile AFL++ static
* ASAN_BUILD - compiles with memory sanitizer for debug purposes
* ASAN_BUILD - compiles AFL++ with memory sanitizer for debug purposes
* UBSAN_BUILD - compiles AFL++ tools with undefined behaviour sanitizer for
debug purposes
* DEBUG - no optimization, -ggdb3, all warnings and -Werror
* PROFILING - compile with profiling information (gprof)
* PROFILING - compile afl-fuzz with profiling information
* INTROSPECTION - compile afl-fuzz with mutation introspection
* NO_PYTHON - disable python support
* NO_SPLICING - disables splicing mutation in afl-fuzz, not recommended for
normal fuzzing
* NO_NYX - disable building nyx mode dependencies
* NO_CORESIGHT - disable building coresight (arm64 only)
* NO_UNICORN_ARM64 - disable building unicorn on arm64
* AFL_NO_X86 - if compiling on non-intel/amd platforms
* LLVM_CONFIG - if your distro doesn't use the standard name for llvm-config
(e.g., Debian)
e.g.: `make ASAN_BUILD=1`
e.g.: `make LLVM_CONFIG=llvm-config-14`
## MacOS X on x86 and arm64 (M1)
......
......@@ -38,6 +38,11 @@ performed with the custom mutator.
## 2) APIs
**IMPORTANT NOTE**: If you use our C/C++ API and you want to increase the size
of an **out_buf buffer, you have to use `afl_realloc()` for this, so include
`include/alloc-inl.h` - otherwise afl-fuzz will crash when trying to free
your buffers.
C/C++:
```c
......@@ -159,6 +164,10 @@ def deinit(): # optional for Python
This can return any python object that implements the buffer protocol and
supports PyBUF_SIMPLE. These include bytes, bytearray, etc.
You can decide in the post_process mutator to not send the mutated data
to the target, e.g. if it is too short, too corrupted, etc. If so,
return a NULL buffer and zero length (or a 0 length string in Python).
- `queue_new_entry` (optional):
This methods is called after adding a new test case to the queue. If the
......
......@@ -116,7 +116,7 @@ ifndef OS
$(error "Operating system unsupported")
endif
GUM_DEVKIT_VERSION=15.1.27
GUM_DEVKIT_VERSION=15.2.1
GUM_DEVKIT_FILENAME=frida-gumjs-devkit-$(GUM_DEVKIT_VERSION)-$(OS)-$(ARCH).tar.xz
GUM_DEVKIT_URL="https://github.com/frida/frida/releases/download/$(GUM_DEVKIT_VERSION)/$(GUM_DEVKIT_FILENAME)"
......
......@@ -151,6 +151,7 @@ instances run CMPLOG mode and instrumentation of the binary is less frequent
* `AFL_FRIDA_INST_DEBUG_FILE` - File to write raw assembly of original blocks
and their instrumented counterparts during block compilation.
```
Creating block for 0x7ffff7953313:
0x7ffff7953313 mov qword ptr [rax], 0
0x7ffff795331a add rsp, 8
......@@ -166,7 +167,7 @@ Generated block 0x7ffff75e98e2
***
```
```
* `AFL_FRIDA_INST_CACHE_SIZE` - Set the size of the instrumentation cache used
as a look-up table to cache real to instrumented address block translations.
Default is 256Mb.
......@@ -177,6 +178,8 @@ Default is 256Mb.
a file.
* `AFL_FRIDA_INST_NO_OPTIMIZE` - Don't use optimized inline assembly coverage
instrumentation (the default where available). Required to use
* `AFL_FRIDA_INST_REGS_FILE` - File to write raw register contents at the start
of each block.
`AFL_FRIDA_INST_TRACE`.
* `AFL_FRIDA_INST_NO_CACHE` - Don't use a look-up table to cache real to
instrumented address block translations.
......
......@@ -850,6 +850,14 @@ class Afl {
static setInstrumentNoOptimize() {
Afl.jsApiSetInstrumentNoOptimize();
}
/**
* See `AFL_FRIDA_INST_REGS_FILE`. This function takes a single `string` as
* an argument.
*/
public static setInstrumentRegsFile(file: string): void {
const buf = Memory.allocUtf8String(file);
Afl.jsApiSetInstrumentRegsFile(buf);
}
/*
* See `AFL_FRIDA_INST_SEED`
*/
......
......@@ -19,6 +19,7 @@
js_api_set_instrument_libraries;
js_api_set_instrument_instructions;
js_api_set_instrument_no_optimize;
js_api_set_instrument_regs_file;
js_api_set_instrument_seed;
js_api_set_instrument_trace;
js_api_set_instrument_trace_unique;
......
......@@ -13,6 +13,7 @@ extern gboolean instrument_unique;
extern guint64 instrument_hash_zero;
extern char *instrument_coverage_unstable_filename;
extern gboolean instrument_coverage_insn;
extern char * instrument_regs_filename;
extern gboolean instrument_use_fixed_seed;
extern guint64 instrument_fixed_seed;
......@@ -66,6 +67,8 @@ void instrument_cache_config(void);
void instrument_cache_init(void);
void instrument_cache_insert(gpointer real_address, gpointer code_address);
void instrument_cache(const cs_insn *instr, GumStalkerOutput *output);
void instrument_write_regs(GumCpuContext *cpu_context, gpointer user_data);
void instrument_regs_format(int fd, char *format, ...);
#endif
#include <fcntl.h>
#include <unistd.h>
#include <sys/shm.h>
#include <sys/mman.h>
......@@ -20,6 +21,8 @@
#include "stats.h"
#include "util.h"
#define FRIDA_DEFAULT_MAP_SIZE (64UL << 10)
gboolean instrument_tracing = false;
gboolean instrument_optimize = false;
gboolean instrument_unique = false;
......@@ -30,6 +33,7 @@ gboolean instrument_use_fixed_seed = FALSE;
guint64 instrument_fixed_seed = 0;
char *instrument_coverage_unstable_filename = NULL;
gboolean instrument_coverage_insn = FALSE;
char * instrument_regs_filename = NULL;
static GumStalkerTransformer *transformer = NULL;
......@@ -37,6 +41,8 @@ static GumAddress previous_rip = 0;
static GumAddress previous_end = 0;
static u8 *edges_notified = NULL;
static int regs_fd = -1;
__thread guint64 instrument_previous_pc;
__thread guint64 *instrument_previous_pc_addr = NULL;
......@@ -230,6 +236,10 @@ static void instrument_basic_block(GumStalkerIterator *iterator,
}
if (unlikely(instrument_regs_filename != NULL)) {
gum_stalker_iterator_put_callout(iterator, instrument_write_regs,
(void *)(size_t)regs_fd, NULL);
}
}
}
......@@ -240,8 +250,6 @@ static void instrument_basic_block(GumStalkerIterator *iterator,
}
instrument_debug_instruction(instr->address, instr->size, output);
if (likely(!excluded)) {
asan_instrument(instr, iterator);
......@@ -266,7 +274,6 @@ static void instrument_basic_block(GumStalkerIterator *iterator,
instrument_flush(output);
instrument_debug_end(output);
instrument_coverage_end(instr->address + instr->size);
}
void instrument_config(void) {
......@@ -279,6 +286,7 @@ void instrument_config(void) {
instrument_coverage_unstable_filename =
(getenv("AFL_FRIDA_INST_UNSTABLE_COVERAGE_FILE"));
instrument_coverage_insn = (getenv("AFL_FRIDA_INST_INSN") != NULL);
instrument_regs_filename = getenv("AFL_FRIDA_INST_REGS_FILE");
instrument_debug_config();
instrument_coverage_config();
......@@ -290,6 +298,8 @@ void instrument_config(void) {
void instrument_init(void) {
if (__afl_map_size == MAP_SIZE) __afl_map_size = FRIDA_DEFAULT_MAP_SIZE;
if (!instrument_is_coverage_optimize_supported()) instrument_optimize = false;
FOKF(cBLU "Instrumentation" cRST " - " cGRN "optimize:" cYEL " [%c]",
......@@ -390,6 +400,23 @@ void instrument_init(void) {
instrument_hash_seed);
instrument_hash_zero = instrument_get_offset_hash(0);
FOKF(cBLU "Instrumentation" cRST " - " cGRN "regs:" cYEL " [%s]",
instrument_regs_filename == NULL ? " " : instrument_regs_filename);
if (instrument_regs_filename != NULL) {
char *path =
g_canonicalize_filename(instrument_regs_filename, g_get_current_dir());
FOKF(cBLU "Instrumentation" cRST " - " cGRN "path:" cYEL " [%s]", path);
regs_fd = open(path, O_RDWR | O_CREAT | O_TRUNC,
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
if (regs_fd < 0) { FFATAL("Failed to open regs file '%s'", path); }
g_free(path);
}
asan_init();
cmplog_init();
instrument_coverage_init();
......@@ -416,3 +443,19 @@ void instrument_on_fork() {
}
void instrument_regs_format(int fd, char *format, ...) {
va_list ap;
char buffer[4096] = {0};
int ret;
int len;
va_start(ap, format);
ret = vsnprintf(buffer, sizeof(buffer) - 1, format, ap);
va_end(ap);
if (ret < 0) { return; }
len = strnlen(buffer, sizeof(buffer));
IGNORED_RETURN(write(fd, buffer, len));
}
......@@ -80,5 +80,24 @@ void instrument_cache(const cs_insn *instr, GumStalkerOutput *output) {
}
void instrument_write_regs(GumCpuContext *cpu_context, gpointer user_data) {
int fd = (int)user_data;
instrument_regs_format(fd,
"r0 : 0x%08x, r1 : 0x%08x, r2 : 0x%08x, r3 : 0x%08x\n",
cpu_context->r[0], cpu_context->r[2],
cpu_context->r[1], cpu_context->r[3]);
instrument_regs_format(fd,
"r4 : 0x%08x, r5 : 0x%08x, r6 : 0x%08x, r7 : 0x%08x\n",
cpu_context->r[4], cpu_context->r[5],
cpu_context->r[6], cpu_context->r[7]);
instrument_regs_format(
fd, "r8 : 0x%08x, r9 : 0x%08x, r10: 0x%08x, r11: 0x%08x\n",
cpu_context->r8, cpu_context->r9, cpu_context->r10, cpu_context->r11);
instrument_regs_format(
fd, "r12: 0x%08x, sp : 0x%08x, lr : 0x%08x, pc : 0x%08x\n",
cpu_context->r12, cpu_context->sp, cpu_context->lr, cpu_context->pc);
instrument_regs_format(fd, "cpsr: 0x%08x\n\n", cpu_context->cpsr);
}
#endif
......@@ -272,9 +272,10 @@ void instrument_coverage_optimize(const cs_insn *instr,
GumAddressSpec spec = {.near_address = cw->code,
.max_distance = 1ULL << 30};
guint page_size = gum_query_page_size();
instrument_previous_pc_addr = gum_memory_allocate_near(
&spec, sizeof(guint64), 0x1000, GUM_PAGE_READ | GUM_PAGE_WRITE);
&spec, sizeof(guint64), page_size, GUM_PAGE_READ | GUM_PAGE_WRITE);
*instrument_previous_pc_addr = instrument_hash_zero;
FVERBOSE("instrument_previous_pc_addr: %p", instrument_previous_pc_addr);
FVERBOSE("code_addr: %p", cw->code);
......@@ -405,5 +406,41 @@ void instrument_cache(const cs_insn *instr, GumStalkerOutput *output) {
}
void instrument_write_regs(GumCpuContext *cpu_context, gpointer user_data) {
int fd = (int)(size_t)user_data;
instrument_regs_format(
fd, "x0 : 0x%016x, x1 : 0x%016x, x2 : 0x%016x, x3 : 0x%016x\n",
cpu_context->x[0], cpu_context->x[1], cpu_context->x[2],
cpu_context->x[3]);
instrument_regs_format(
fd, "x4 : 0x%016x, x5 : 0x%016x, x6 : 0x%016x, x7 : 0x%016x\n",
cpu_context->x[4], cpu_context->x[5], cpu_context->x[6],
cpu_context->x[7]);
instrument_regs_format(
fd, "x8 : 0x%016x, x9 : 0x%016x, x10: 0x%016x, x11: 0x%016x\n",
cpu_context->x[8], cpu_context->x[9], cpu_context->x[10],
cpu_context->x[11]);
instrument_regs_format(
fd, "x12: 0x%016x, x13: 0x%016x, x14: 0x%016x, x15: 0x%016x\n",
cpu_context->x[12], cpu_context->x[13], cpu_context->x[14],
cpu_context->x[15]);
instrument_regs_format(
fd, "x16: 0x%016x, x17: 0x%016x, x18: 0x%016x, x19: 0x%016x\n",
cpu_context->x[16], cpu_context->x[17], cpu_context->x[18],
cpu_context->x[19]);
instrument_regs_format(
fd, "x20: 0x%016x, x21: 0x%016x, x22: 0x%016x, x23: 0x%016x\n",
cpu_context->x[20], cpu_context->x[21], cpu_context->x[22],
cpu_context->x[23]);
instrument_regs_format(
fd, "x24: 0x%016x, x25: 0x%016x, x26: 0x%016x, x27: 0x%016x\n",
cpu_context->x[24], cpu_context->x[25], cpu_context->x[26],
cpu_context->x[27]);
instrument_regs_format(
fd, "x28: 0x%016x, fp : 0x%016x, lr : 0x%016x, sp : 0x%016x\n",
cpu_context->x[28], cpu_context->fp, cpu_context->lr, cpu_context->sp);
instrument_regs_format(fd, "pc : 0x%016x\n\n", cpu_context->pc);
}
#endif
......@@ -317,6 +317,12 @@ static void coverage_write_events(void *key, void *value, void *user_data) {
};
#if (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
evt.offset = __builtin_bswap32(evt.offset);
evt.length = __builtin_bswap16(evt.length);
evt.module = __builtin_bswap16(evt.module);
#endif
coverage_write(fd, &evt, sizeof(coverage_event_t));
}
......
......@@ -63,14 +63,12 @@ static void instrument_disasm(guint8 *start, guint8 *end,
count = cs_disasm(capstone, curr, size, GPOINTER_TO_SIZE(curr), 0, &insn);
if (insn == NULL) {
instrument_debug("\t0x%" G_GINT64_MODIFIER "x\t* 0x%016" G_GSIZE_MODIFIER
"x\n",
(uint64_t)curr, *(size_t *)curr);
(uint64_t)(size_t)curr, *(size_t *)curr);
len += sizeof(size_t);
continue;
}
for (i = 0; i != count; i++) {
......
......@@ -337,15 +337,14 @@ void instrument_coverage_optimize(const cs_insn *instr,
GumStalkerOutput *output) {
GumX86Writer *cw = output->writer.x86;
/* guint64 area_offset =
* instrument_get_offset_hash(GUM_ADDRESS(instr->address)); */
if (instrument_previous_pc_addr == NULL) {
GumAddressSpec spec = {.near_address = cw->code,
.max_distance = 1ULL << 30};
guint page_size = gum_query_page_size();
instrument_previous_pc_addr = gum_memory_allocate_near(
&spec, sizeof(guint64), 0x1000, GUM_PAGE_READ | GUM_PAGE_WRITE);
&spec, sizeof(guint64), page_size, GUM_PAGE_READ | GUM_PAGE_WRITE);
*instrument_previous_pc_addr = instrument_hash_zero;
FVERBOSE("instrument_previous_pc_addr: %p", instrument_previous_pc_addr);
FVERBOSE("code_addr: %p", cw->code);
......@@ -469,5 +468,22 @@ gpointer instrument_cur(GumStalkerOutput *output) {
}
void instrument_write_regs(GumCpuContext *cpu_context, gpointer user_data) {
int fd = (int)(size_t)user_data;
instrument_regs_format(
fd, "rax: 0x%016x, rbx: 0x%016x, rcx: 0x%016x, rdx: 0x%016x\n",
cpu_context->rax, cpu_context->rbx, cpu_context->rcx, cpu_context->rdx);
instrument_regs_format(
fd, "rdi: 0x%016x, rsi: 0x%016x, rbp: 0x%016x, rsp: 0x%016x\n",
cpu_context->rdi, cpu_context->rsi, cpu_context->rbp, cpu_context->rsp);
instrument_regs_format(
fd, "r8 : 0x%016x, r9 : 0x%016x, r10: 0x%016x, r11: 0x%016x\n",
cpu_context->r8, cpu_context->r9, cpu_context->r10, cpu_context->r11);
instrument_regs_format(
fd, "r12: 0x%016x, r13: 0x%016x, r14: 0x%016x, r15: 0x%016x\n",
cpu_context->r12, cpu_context->r13, cpu_context->r14, cpu_context->r15);
instrument_regs_format(fd, "rip: 0x%016x\n\n", cpu_context->rip);
}
#endif
......@@ -162,9 +162,10 @@ void instrument_coverage_optimize(const cs_insn *instr,
GumAddressSpec spec = {.near_address = cw->code,
.max_distance = 1ULL << 30};
guint page_size = gum_query_page_size();
instrument_previous_pc_addr = gum_memory_allocate_near(
&spec, sizeof(guint64), 0x1000, GUM_PAGE_READ | GUM_PAGE_WRITE);
&spec, sizeof(guint64), page_size, GUM_PAGE_READ | GUM_PAGE_WRITE);
*instrument_previous_pc_addr = instrument_hash_zero;
FVERBOSE("instrument_previous_pc_addr: %p", instrument_previous_pc_addr);
FVERBOSE("code_addr: %p", cw->code);
......@@ -269,5 +270,16 @@ void instrument_cache(const cs_insn *instr, GumStalkerOutput *output) {
}
void instrument_write_regs(GumCpuContext *cpu_context, gpointer user_data) {
int fd = (int)(size_t)user_data;
instrument_regs_format(
fd, "eax: 0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x\n",
cpu_context->eax, cpu_context->ebx, cpu_context->ecx, cpu_context->edx);
instrument_regs_format(
fd, "esi: 0x%08x, edi: 0x%08x, ebp: 0x%08x, esp: 0x%08x\n",
cpu_context->esi, cpu_context->edi, cpu_context->ebp, cpu_context->esp);
instrument_regs_format(fd, "eip: 0x%08x\n\n", cpu_context->eip);
}
#endif
......@@ -7,8 +7,8 @@ void intercept_hook(void *address, gpointer replacement, gpointer user_data) {
GumInterceptor *interceptor = gum_interceptor_obtain();
gum_interceptor_begin_transaction(interceptor);
GumReplaceReturn ret =
gum_interceptor_replace(interceptor, address, replacement, user_data);
GumReplaceReturn ret = gum_interceptor_replace(interceptor, address,
replacement, user_data, NULL);
if (ret != GUM_REPLACE_OK) { FFATAL("gum_interceptor_attach: %d", ret); }
gum_interceptor_end_transaction(interceptor);
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment