evbuffer_add_file broken when evbuffer_get_length(outbuf) > 0 and offset > 0 #306

scottlamb · 2015-12-23T05:32:25Z

Documentation and common sense suggest that evbuffer_add_file(outbuf, fd, offset, length) should strictly append to outbuf, as other evbuffer_add_* methods do, and that the offset should be the offset within the file.

However, at least when using mmap, evbuffer_add_file actually skips a prefix of the buffer instead. The difference is significant when the buffer is already non-empty.

I'm attaching a program to reproduce this behavior.

This occurs both on libevent 2.0.21-stable and the latest master head (d56efd9, roughly 2.1.5-beta).

$ g++ -Wall evbuffer_add_file_bug.c -o evbuffer_add_file_bug -levent
$ ./evbuffer_add_file_bug
evbuffer_add_filebug: Mismatch, expected: "0123456789efghij" (16 bytes), got: "456789abcdefghij" (16 bytes)

evbuffer_add_file_bug.c.txt

The text was updated successfully, but these errors were encountered:

errzey · 2015-12-23T07:20:31Z

Working as expected using master

Breakpoint 1 at 0x401fb6: file evbuffer_add_file_bug.c, line 21.
(gdb) r
Starting program: /home/mthomas/Code/libevent/evbuffer_add_file_bug

Breakpoint 1, main (argc=1, argv=0x7fffffffe8c8) at evbuffer_add_file_bug.c:21
21  main(int argc, char ** argv) {
(gdb) n
23      char in_filename[]  = "/tmp/evbuffer_add_file_bug.in.XXXXXX";
(gdb)
24      char out_filename[] = "/tmp/evbuffer_add_file_bug.out.XXXXXX";
(gdb)
25      int  in_fd          = mkstemp(in_filename);
(gdb)
27      if (in_fd < 0) {
(gdb)
31      if (unlink(in_filename) != 0) {
(gdb)
35      int out_fd = mkstemp(out_filename);
(gdb)
37      if (out_fd < 0) {
(gdb)
41      if (unlink(out_filename) != 0) {
(gdb)
46      ssize_t ret = write(in_fd, kFileData, strlen(kFileData));
(gdb)
48      if (ret != strlen(kFileData)) {
(gdb) print kFileData
$1 = "abcdefghij"
(gdb) n
51      if (lseek(in_fd, 0, SEEK_SET) != 0) {
(gdb)
56      struct evbuffer * buf       = evbuffer_new();
(gdb)
58      if (evbuffer_add(buf, kExistingData, strlen(kExistingData)) != 0) {
(gdb) print *buf
$2 = {
  first = 0x0,
  last = 0x0,
  last_with_datap = 0x631010,
  total_len = 0,
  n_add_for_cb = 0,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
62      ev_off_t file_length = strlen(kFileData) - kFileOffset;
(gdb) print *buf
$3 = {
  first = 0x6310a0,
  last = 0x6310a0,
  last_with_datap = 0x631010,
  total_len = 10,
  n_add_for_cb = 0,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
64      if (evbuffer_add_file(buf, in_fd, kFileOffset, file_length) != 0) {
(gdb) s
evbuffer_add_file (buf=0x631010, fd=3, offset=4, length=6) at buffer.c:3276
3276        unsigned flags = EVBUF_FS_CLOSE_ON_FREE;
(gdb) print *buf
$4 = {
  first = 0x6310a0,
  last = 0x6310a0,
  last_with_datap = 0x631010,
  total_len = 10,
  n_add_for_cb = 0,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
3279        seg = evbuffer_file_segment_new(fd, offset, length, flags);
(gdb) s
evbuffer_file_segment_new (fd=3, offset=4, length=6, flags=1) at buffer.c:2946
2946        struct evbuffer_file_segment *seg =
(gdb) n
2948        if (!seg)
(gdb)
2950        seg->refcnt = 1;
(gdb)
2951        seg->fd = fd;
(gdb)
2952        seg->flags = flags;
(gdb) print flags
$5 = 1
(gdb) n
2953        seg->file_offset = offset;
(gdb) print offset
$6 = 4
(gdb) n
2954        seg->cleanup_cb = NULL;
(gdb) n
2955        seg->cleanup_cb_arg = NULL;
(gdb)
2967        if (length == -1) {
(gdb)
2973        seg->length = length;
(gdb)
2975        if (offset < 0 || length < 0 ||
(gdb)
2977            (ev_uint64_t)offset > (ev_uint64_t)(EVBUFFER_CHAIN_MAX - length))
(gdb) list
2972        }
2973        seg->length = length;
2974
2975        if (offset < 0 || length < 0 ||
2976            ((ev_uint64_t)length > EVBUFFER_CHAIN_MAX) ||
2977            (ev_uint64_t)offset > (ev_uint64_t)(EVBUFFER_CHAIN_MAX - length))
2978            goto err;
2979
2980    #if defined(USE_SENDFILE)
2981        if (!(flags & EVBUF_FS_DISABLE_SENDFILE)) {
(gdb) n
2976            ((ev_uint64_t)length > EVBUFFER_CHAIN_MAX) ||
(gdb) n
2981        if (!(flags & EVBUF_FS_DISABLE_SENDFILE)) {
(gdb) n
2982            seg->can_sendfile = 1;
(gdb) n
2983            goto done;
(gdb)
2993        if (!(flags & EVBUF_FS_DISABLE_LOCKING)) {
(gdb) n
2994            EVTHREAD_ALLOC_LOCK(seg->lock, 0);
(gdb)
2996        return seg;
(gdb)
3000    }
(gdb) print seg
$7 = (struct evbuffer_file_segment *) 0x6314b0
(gdb) print * seg
$8 = {
  lock = 0x0,
  refcnt = 1,
  flags = 1,
  can_sendfile = 1,
  is_mapping = 0,
  fd = 3,
  mapping = 0x0,
  contents = 0x0,
  file_offset = 4,
  mmap_offset = 0,
  length = 6,
  cleanup_cb = 0x0,
  cleanup_cb_arg = 0x0
}
(gdb) n
evbuffer_add_file (buf=0x631010, fd=3, offset=4, length=6) at buffer.c:3280
3280        if (!seg)
(gdb)
3282        r = evbuffer_add_file_segment(buf, seg, 0, length);
(gdb) s
evbuffer_add_file_segment (buf=0x631010, seg=0x6314b0, offset=0, length=6) at buffer.c:3178
3178        int can_use_sendfile = 0;
(gdb) n
3180        EVBUFFER_LOCK(buf);
(gdb)
3181        EVLOCK_LOCK(seg->lock, 0);
(gdb)
3182        if (buf->flags & EVBUFFER_FLAG_DRAINS_TO_FD) {
(gdb)
3185            if (!seg->contents) {
(gdb) list
3180        EVBUFFER_LOCK(buf);
3181        EVLOCK_LOCK(seg->lock, 0);
3182        if (buf->flags & EVBUFFER_FLAG_DRAINS_TO_FD) {
3183            can_use_sendfile = 1;
3184        } else {
3185            if (!seg->contents) {
3186                if (evbuffer_file_segment_materialize(seg)<0) {
3187                    EVLOCK_UNLOCK(seg->lock, 0);
3188                    EVBUFFER_UNLOCK(buf);
3189                    return -1;
(gdb) print *seq
No symbol "seq" in current context.
(gdb) print *seg
$9 = {
  lock = 0x0,
  refcnt = 1,
  flags = 1,
  can_sendfile = 1,
  is_mapping = 0,
  fd = 3,
  mapping = 0x0,
  contents = 0x0,
  file_offset = 4,
  mmap_offset = 0,
  length = 6,
  cleanup_cb = 0x0,
  cleanup_cb_arg = 0x0
}
(gdb) n
3186                if (evbuffer_file_segment_materialize(seg)<0) {
(gdb) s
evbuffer_file_segment_materialize (seg=0x6314b0) at buffer.c:3021
3021        const unsigned flags = seg->flags;
(gdb) n
3022        const int fd = seg->fd;
(gdb)
3023        const ev_off_t length = seg->length;
(gdb)
3024        const ev_off_t offset = seg->file_offset;
(gdb)
3026        if (seg->contents)
(gdb)
3030        if (!(flags & EVBUF_FS_DISABLE_MMAP)) {
(gdb)
3031            off_t offset_rounded = 0, offset_leftover = 0;
(gdb)
3033            if (offset) {
(gdb) print offset
$10 = 4
(gdb) n
3036                long page_size = get_page_size();
(gdb) n
3037                if (page_size == -1)
(gdb) print pag_size
No symbol "pag_size" in current context.
(gdb) print page_size
$11 = 4096
(gdb) n
3039                offset_leftover = offset % page_size;
(gdb) n
3040                offset_rounded = offset - offset_leftover;
(gdb)
3042            mapped = mmap(NULL, length + offset_leftover,
(gdb)
3052            if (mapped == MAP_FAILED) {
(gdb)
3056                seg->mapping = mapped;
(gdb)
3057                seg->contents = (char*)mapped+offset_leftover;
(gdb)
3058                seg->mmap_offset = 0;
(gdb) print *seg
$12 = {
  lock = 0x0,
  refcnt = 1,
  flags = 1,
  can_sendfile = 1,
  is_mapping = 0,
  fd = 3,
  mapping = 0x7ffff7ff7000,
  contents = 0x7ffff7ff7004 "efghij",
  file_offset = 4,
  mmap_offset = 0,
  length = 6,
  cleanup_cb = 0x0,
  cleanup_cb_arg = 0x0
}
(gdb) n
3059                seg->is_mapping = 1;
(gdb)
3060                goto done;
(gdb)
3120        return 0;
(gdb)
3123    }
(gdb)
evbuffer_add_file_segment (buf=0x631010, seg=0x6314b0, offset=0, length=6) at buffer.c:3193
3193        ++seg->refcnt;
(gdb)
3194        EVLOCK_UNLOCK(seg->lock, 0);
(gdb)
3196        if (buf->freeze_end)
(gdb)
3199        if (length < 0) {
(gdb) print lngth
No symbol "lngth" in current context.
(gdb) print length
$13 = 6
(gdb) n
3206        if (offset+length > seg->length)
(gdb) print offset
$14 = 0
(gdb) print offset+length
$15 = 6
(gdb) print seg->length
$16 = 6
(gdb) n
3209        chain = evbuffer_chain_new(sizeof(struct evbuffer_chain_file_segment));
(gdb) n
3210        if (!chain)
(gdb) print * chain
$17 = {
  next = 0x0,
  buffer_len = 976,
  misalign = 0,
  off = 0,
  flags = 0,
  refcnt = 1,
  buffer = 0x631540 ""
}
(gdb) n
3212        extra = EVBUFFER_CHAIN_EXTRA(struct evbuffer_chain_file_segment, chain);
(gdb) s
3214        chain->flags |= EVBUFFER_IMMUTABLE|EVBUFFER_FILESEGMENT;
(gdb) print * chain
$18 = {
  next = 0x0,
  buffer_len = 976,
  misalign = 0,
  off = 0,
  flags = 0,
  refcnt = 1,
  buffer = 0x631540 ""
}
(gdb) ls extra
Undefined command: "ls".  Try "help".
(gdb) print ext
extend_buffers      ext_match           extra               extra_entry         extra_entry_module  ext_wmatch
(gdb) print extra
$19 = (struct evbuffer_chain_file_segment *) 0x631540
(gdb) print *extra
$20 = {
  segment = 0x0
}
(gdb) n
3215        if (can_use_sendfile && seg->can_sendfile) {
(gdb)
3220        } else if (seg->is_mapping) {
(gdb)
3247            chain->buffer = (unsigned char*)(seg->contents + offset);
(gdb) print *seg->conents
There is no member named conents.
(gdb) print *seg->contents
$21 = 101 'e'
(gdb) print *seg->contents + offset
$22 = 101
(gdb) print seg->contents + offset
$23 = 0x7ffff7ff7004 "efghij"
(gdb) print chain->buffer
$24 = (unsigned char *) 0x631540 ""
(gdb) n
3248            chain->buffer_len = length;
(gdb) print chain->buffer
$25 = (unsigned char *) 0x7ffff7ff7004 "efghij"
(gdb) print length
$26 = 6
(gdb) n
3249            chain->off = length;
(gdb)
3257        extra->segment = seg;
(gdb) print *chain
$27 = {
  next = 0x0,
  buffer_len = 6,
  misalign = 0,
  off = 6,
  flags = 9,
  refcnt = 1,
  buffer = 0x7ffff7ff7004 "efghij"
}
(gdb) n
3258        buf->n_add_for_cb += length;
(gdb)
3259        evbuffer_chain_insert(buf, chain);
(gdb) pint * buf
Undefined command: "pint".  Try "help".
(gdb) print *buf
$28 = {
  first = 0x6310a0,
  last = 0x6310a0,
  last_with_datap = 0x631010,
  total_len = 10,
  n_add_for_cb = 6,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) i s
#0  evbuffer_add_file_segment (buf=0x631010, seg=0x6314b0, offset=0, length=6) at buffer.c:3259
#1  0x0000000000409910 in evbuffer_add_file (buf=0x631010, fd=3, offset=4, length=6) at buffer.c:3282
#2  0x0000000000402222 in main (argc=1, argv=0x7fffffffe8c8) at evbuffer_add_file_bug.c:64
(gdb) s
evbuffer_chain_insert (buf=0x631010, chain=0x631510) at buffer.c:311
311     ASSERT_EVBUFFER_LOCKED(buf);
(gdb) n
312     if (*buf->last_with_datap == NULL) {
(gdb)
319         chp = evbuffer_free_trailing_empty_chains(buf);
(gdb)
320         *chp = chain;
(gdb)
321         if (chain->off)
(gdb)
322             buf->last_with_datap = chp;
(gdb)
323         buf->last = chain;
(gdb) print *buf
$29 = {
  first = 0x6310a0,
  last = 0x6310a0,
  last_with_datap = 0x6310a0,
  total_len = 10,
  n_add_for_cb = 6,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
325     buf->total_len += chain->off;
(gdb)
326 }
(gdb)
evbuffer_add_file_segment (buf=0x631010, seg=0x6314b0, offset=0, length=6) at buffer.c:3261
3261        evbuffer_invoke_callbacks_(buf);
(gdb) PRINT *BUF
No symbol "BUF" in current context.
(gdb) print * buf
$30 = {
  first = 0x6310a0,
  last = 0x631510,
  last_with_datap = 0x6310a0,
  total_len = 16,
  n_add_for_cb = 6,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
3263        EVBUFFER_UNLOCK(buf);
(gdb)
3265        return 0;
(gdb)
3270    }
(gdb)
evbuffer_add_file (buf=0x631010, fd=3, offset=4, length=6) at buffer.c:3283
3283        if (r == 0)
(gdb)
3284            evbuffer_file_segment_free(seg);
(gdb)
3285        return r;
(gdb)
3286    }
(gdb)
main (argc=1, argv=0x7fffffffe8c8) at evbuffer_add_file_bug.c:69
69      size_t buf_len = evbuffer_get_length(buf);
(gdb) print * buf
$31 = {
  first = 0x6310a0,
  last = 0x631510,
  last_with_datap = 0x6310a0,
  total_len = 16,
  n_add_for_cb = 0,
  n_del_for_cb = 0,
  lock = 0x0,
  own_lock = 0,
  freeze_start = 0,
  freeze_end = 0,
  deferred_cbs = 0,
  flags = 0,
  cb_queue = 0x0,
  refcnt = 1,
  deferred = {
    evcb_active_next = {
      tqe_next = 0x0,
      tqe_prev = 0x0
    },
    evcb_flags = 0,
    evcb_pri = 0 '\000',
    evcb_closure = 0 '\000',
    evcb_cb_union = {
      evcb_callback = 0x0,
      evcb_selfcb = 0x0,
      evcb_evfinalize = 0x0,
      evcb_cbfinalize = 0x0
    },
    evcb_arg = 0x0
  },
  callbacks = {
    lh_first = 0x0
  },
  parent = 0x0
}
(gdb) n
70      int    written = evbuffer_write(buf, out_fd);
(gdb) 2s
Undefined command: "2s".  Try "help".
(gdb) s
evbuffer_write (buffer=0x631010, fd=4) at buffer.c:2553
2553        return evbuffer_write_atmost(buffer, fd, -1);
(gdb) s
evbuffer_write_atmost (buffer=0x631010, fd=4, howmuch=-1) at buffer.c:2506
2506        int n = -1;
(gdb) n
2508        EVBUFFER_LOCK(buffer);
(gdb)
2510        if (buffer->freeze_start) {
(gdb)
2514        if (howmuch < 0 || (size_t)howmuch > buffer->total_len)
(gdb)
2515            howmuch = buffer->total_len;
(gdb)
2517        if (howmuch > 0) {
(gdb)
2519            struct evbuffer_chain *chain = buffer->first;
(gdb)
2520            if (chain != NULL && (chain->flags & EVBUFFER_SENDFILE))
(gdb)
2525            n = evbuffer_write_iovec(buffer, fd, howmuch);
(gdb)
2542        if (n > 0)
(gdb)
2543            evbuffer_drain(buffer, n);
(gdb) print n
$32 = 16
(gdb) n
2546        EVBUFFER_UNLOCK(buffer);
(gdb)
2547        return (n);
(gdb)
2548    }
(gdb)
evbuffer_write (buffer=0x631010, fd=4) at buffer.c:2554
2554    }
(gdb)
main (argc=1, argv=0x7fffffffe8c8) at evbuffer_add_file_bug.c:72
72      if (written < 0 || (size_t)written != buf_len) {
(gdb) n
77      if (lseek(out_fd, 0, SEEK_SET) != 0) {
(gdb)
81      char    actual[1024] = { 0 };
(gdb)
82      ssize_t bytes_read   = read(out_fd, actual, sizeof(actual) - 1);
(gdb) print actual
$33 = '\000' <repeats 1023 times>
(gdb) n
84      if (bytes_read < 0) {
(gdb) print bytes_read
$34 = 16
(gdb) n
88      if (bytes_read != strlen(kExpectedResult) ||
(gdb) prnt kExpectdResult
Undefined command: "prnt".  Try "help".
(gdb) print kExpectedResult
$35 = "0123456789efghij"
(gdb) print strlen(kExpectedResult)
$36 = 16
(gdb) print bytes_read
$37 = 16
(gdb) n
89          memcmp(kExpectedResult, actual, bytes_read) != 0) {
(gdb) print kExpectedResult
$38 = "0123456789efghij"
(gdb) print actual
$39 = "0123456789efghij", '\000' <repeats 1007 times>
(gdb) n
88      if (bytes_read != strlen(kExpectedResult) ||
(gdb) n
95      return 0;
(gdb)
96  } /* main */
(gdb)
__libc_start_main (main=0x401f9d <main>, argc=1, argv=0x7fffffffe8c8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe8b8) at libc-start.c:321
321 libc-start.c: No such file or directory.
(gdb)
[Inferior 1 (process 1140) exited normally]
(gdb)
The program is not being run.
(gdb) q

Though it may be a timing thing maybe? Maybe EVBUFFER_FLAG_DRAINS_TO_FD?

errzey · 2015-12-23T07:22:37Z

Oh, additionally I statically linked libevent.

02:17:00) ○ [mthomas@EAGAIN] ~/Code/libevent  gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.8.4-2ubuntu1~14.04' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04)

(02:26:09) ○ [mthomas@EAGAIN] ~/Code/libevent  uname -a
Linux EAGAIN 3.13.0-68-generic #111-Ubuntu SMP Fri Nov 6 18:17:06 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux

Compiled with gcc -O0 -ggdb -I/usr/local/include/ evbuffer_add_file_bug.c -o evbuffer_add_file_bug /usr/local/lib/libevent.a

scottlamb · 2015-12-23T15:00:31Z

Oh, wow, thanks for looking at this so quickly.

And...I was mixed up. It doesn't actually happen with current master. Sorry for the waste of time. :(

Are bugfixes still being made to the 2.0 branch, or should I just considered it fixed by 2.1?

I added a version printf:

  printf("compiled with %s, running with %s\n",
         LIBEVENT_VERSION, event_get_version());

[slamb@ubuntu ~]$ g++ -g -Wall evbuffer_add_file_bug.c -o evbuffer_add_file_bug /usr/local/lib/libevent.a 
[slamb@ubuntu ~]$ ./evbuffer_add_file_bug 
compiled with 2.1.5-beta, running with 2.1.5-beta
[slamb@ubuntu ~]$

With 2.0.21-stable:

[slamb@ubuntu ~]$ g++ -g -Wall evbuffer_add_file_bug.c -o evbuffer_add_file_bug /usr/local/lib/libevent.a 
[slamb@ubuntu ~]$ ./evbuffer_add_file_bug 
compiled with 2.0.21-stable, running with 2.0.21-stable
evbuffer_add_file_bug: Mismatch, expected: "0123456789efghij" (16 bytes), got: "456789abcdefghij" (16 bytes)
[slamb@ubuntu ~]$

gdb shows it takes this path involving evbuffer_drain (which just doesn't make sense here):

(gdb) break evbuffer_drain
Breakpoint 1 at 0x40c240: file buffer.c, line 952.
(gdb) run
Starting program: /home/slamb/evbuffer_add_file_bug 
compiled with 2.0.21-stable, running with 2.0.21-stable

Breakpoint 1, evbuffer_drain (buf=buf@entry=0x622010, len=len@entry=4) at buffer.c:952
952     {
(gdb) bt
#0  evbuffer_drain (buf=buf@entry=0x622010, len=len@entry=4) at buffer.c:952
#1  0x000000000040ecc3 in evbuffer_add_file (outbuf=0x622010, fd=3, offset=4, length=6) at buffer.c:2828
#2  0x0000000000401fe1 in main (argc=1, argv=0x7fffffffe6e8) at evbuffer_add_file_bug.c:58
(gdb) frame 1
#1  0x000000000040ecc3 in evbuffer_add_file (outbuf=0x622010, fd=3, offset=4, length=6) at buffer.c:2828
2828                            evbuffer_drain(outbuf, offset);
(gdb) list 2828
2823                            outbuf->n_add_for_cb += length;
2824
2825                            evbuffer_chain_insert(outbuf, chain);
2826
2827                            /\* we need to subtract whatever we don't need */
2828                            evbuffer_drain(outbuf, offset);
2829                    }
2830            } else
2831    #endif
2832            {

errzey · 2015-12-23T20:22:25Z

We still do 2.0 patches, will look this afternoon
On Wed, Dec 23, 2015 at 7:00 AM Scott Lamb notifications@github.com wrote:

Oh, wow, thanks for looking at this so quickly.

And...I was mixed up. It doesn't actually happen with current master.
Sorry for the waste of time. :(

Are bugfixes still being made to the 2.0 branch, or should I just
considered it fixed by 2.1?

I added a version printf:

printf("compiled with %s, running with %s\n",
LIBEVENT_VERSION, event_get_version());

[slamb@ubuntu ~]$ g++ -g -Wall evbuffer_add_file_bug.c -o
evbuffer_add_file_bug /usr/local/lib/libevent.a
[slamb@ubuntu ~]$ ./evbuffer_add_file_bug
compiled with 2.1.5-beta, running with 2.1.5-beta
[slamb@ubuntu ~]$

With 2.0.21-stable:

[slamb@ubuntu ~]$ g++ -g -Wall evbuffer_add_file_bug.c -o
evbuffer_add_file_bug /usr/local/lib/libevent.a
[slamb@ubuntu ~]$ ./evbuffer_add_file_bug
compiled with 2.0.21-stable, running with 2.0.21-stable
evbuffer_add_file_bug: Mismatch, expected: "0123456789efghij" (16 bytes),
got: "456789abcdefghij" (16 bytes)
[slamb@ubuntu ~]$

gdb shows it takes this path involving evbuffer_drain (which just doesn't
make sense here):

(gdb) break evbuffer_drain
Breakpoint 1 at 0x40c240: file buffer.c, line 952.
(gdb) run
Starting program: /home/slamb/evbuffer_add_file_bug
compiled with 2.0.21-stable, running with 2.0.21-stable

Breakpoint 1, evbuffer_drain (buf=buf@entry=0x622010, len=len@entry=4) at
buffer.c:952
952 {
(gdb) bt
#0 evbuffer_drain (buf=buf@entry=0x622010, len=len@entry=4) at
buffer.c:952
#1 #1 0x000000000040ecc3 in
evbuffer_add_file (outbuf=0x622010, fd=3, offset=4, length=6) at
buffer.c:2828
#2 #2 0x0000000000401fe1 in
main (argc=1, argv=0x7fffffffe6e8) at evbuffer_add_file_bug.c:58
(gdb) frame 1
#1 #1 0x000000000040ecc3 in
evbuffer_add_file (outbuf=0x622010, fd=3, offset=4, length=6) at
buffer.c:2828
2828 evbuffer_drain(outbuf, offset);
(gdb) list 2828
2823 outbuf->n_add_for_cb += length;
2824
2825 evbuffer_chain_insert(outbuf, chain);
2826
2827 /* we need to subtract whatever we don't need */
2828 evbuffer_drain(outbuf, offset);
2829 }
2830 } else
2831 #endif
2832 {

—
Reply to this email directly or view it on GitHub
#306 (comment).

errzey · 2015-12-24T01:51:14Z

Yep, definitely some wrong logic here:

{
                        outbuf->n_add_for_cb += length;

                        evbuffer_chain_insert(outbuf, chain);

                        /* we need to subtract whatever we don't need */
                        evbuffer_drain(outbuf, offset);
}

If I recall, there may have been a note in the documentation about this, but yeah.

This basically means Libevent is assuming your file was first thing added to a newly created bufferevent.

offset is supposed to be "offset inside the file", but it's clear here that the expectation of the content is the file only, this is a logical fail, since it just drains offset of data blindly.

when evbuffer_add_file is called and mmap is used, if the offset argument is >0, a mistake happens: add_file removes "offset" byts from the front of the evbuffer. So that means any data that was previously on the buffer is trimmed off by "offset" bytes. Whoops. A onelinter fix: don't use evbuffer_drain for the offset, instead, just modify the misalign variable on the newly created chain.

See <libevent/libevent#306> for details.

errzey · 2016-04-06T04:28:41Z

afca576 Attemps to fix the issue, but I don't think it fully fixes the issue.

Refs: libevent#306

Refs: libevent#306 Fixes: 20d6d44

Refs: libevent#306 Refs: libevent#340

Refs: libevent#306 Refs: libevent#340 Fixes: 20d6d44

Refs: libevent#306 Fixes: libevent#340 Fixes: 20d6d44

Refs: #306 Refs: #340

Refs: #306 Fixes: #340 Fixes: 20d6d44

paulfariello · 2017-07-31T13:27:45Z

I can confirm that afca576 fixes the issue.
I've tested it with https://gist.github.com/paulfariello/0403f5be9b6b07303cccc9aa3362b9fc

My comprehension of the bug was that buffer was drained as if evbuffer_add_file was the first thing appended to it. But my gist showed that offset was broken even when evbuffer_add_file was first. I must be missing something…

azat · 2017-08-13T22:57:45Z

Looking at this again, and in your case you are using evbuffer from bufferevent and it freezes output buffer for read, hence that evbuffer_drain() (that was removed in that patch) will fail in your case, and hence you got the whole content of the file. IOW there is another bug because of that incorrect evbuffer_drain()

P.S. during looking at this, it should mmap with length + offset after afca576, I will fix it later (also munmap args is wrong with this args to mmap)

azat · 2017-08-13T23:00:34Z

And even more, right now if offset > PAGE_SIZE then mmap will fail, and hence evbuffer_add_file() will fail too

azat · 2017-08-13T23:03:38Z

Also I'm suggesting you to switch to 2.1.8, it should be compatible with 2.0.

paulfariello · 2017-08-21T07:26:08Z

Thanks for the insight. I'll switch to 2.1.8 but right now I'm not using add_file anymore :)

Bunch of fixes for evbuffer_add_file(). * evbuffer_add_file-2.0-fixes: Cover evbuffer_add_file() with offset evbuffer_add_file: do not use evbuffer_remove(), instead calc offset for mmap() evbuffer_add_file: munmap() correct size on mmap() failure evbuffer_add_file: fix endless loop when file does not have such amount of data Fixes: #306

azat · 2017-12-17T23:30:41Z

Hi everyone!

I just pushed a few fixes for evbuffer_add_file(), and now it should for this case too. It will be great if somebody will have time to test them!

errzey added type:bug prio:high status:unconfirmed labels Dec 23, 2015

errzey self-assigned this Dec 23, 2015

errzey added status:awaiting for reply prio:low and removed prio:high status:unconfirmed type:bug labels Dec 23, 2015

errzey added libevent-2.0 and removed status:awaiting for reply labels Dec 23, 2015

scottlamb added a commit to scottlamb/moonfire-nvr that referenced this issue Jan 10, 2016

Work around libevent bug 306 in evbuffer_add_file.

eed2a69

See <libevent/libevent#306> for details.

azat added a commit to azat/libevent that referenced this issue Jun 22, 2016

test/buffer: cover evbuffer_expand() for overflow

3e629ca

Refs: libevent#306

azat added a commit to azat/libevent that referenced this issue Jun 22, 2016

buffer: fix overflow check in evbuffer_expand_singlechain()

7a5d947

Refs: libevent#306 Fixes: 20d6d44

azat added a commit to azat/libevent that referenced this issue Jun 22, 2016

test/buffer: cover evbuffer_expand() for overflow

b947986

Refs: libevent#306 Refs: libevent#340

azat added a commit to azat/libevent that referenced this issue Jun 22, 2016

buffer: fix overflow check in evbuffer_expand_singlechain()

baea3d6

Refs: libevent#306 Refs: libevent#340 Fixes: 20d6d44

azat added a commit to azat/libevent that referenced this issue Jun 24, 2016

buffer: fix overflow check in evbuffer_expand_singlechain()

650a67f

Refs: libevent#306 Refs: libevent#340 Fixes: 20d6d44

azat added a commit to azat/libevent that referenced this issue Jun 25, 2016

buffer: fix overflow check in evbuffer_expand_singlechain()

0e63161

Refs: libevent#306 Fixes: libevent#340 Fixes: 20d6d44

azat added a commit that referenced this issue Jun 25, 2016

test/buffer: cover evbuffer_expand() for overflow

48dab7a

Refs: #306 Refs: #340

azat added a commit that referenced this issue Jun 25, 2016

buffer: fix overflow check in evbuffer_expand_singlechain()

a3f4ccd

Refs: #306 Fixes: #340 Fixes: 20d6d44

errzey added a commit that referenced this issue Jun 28, 2016

Backport for #340 (fix overflow check in expand_singlechain()

7dedc07

Refs: #306 Fixes: #340 Fixes: 20d6d44

paulfariello unassigned errzey Jul 31, 2017

hyperblast mentioned this issue Aug 25, 2017

evbuffer_add_file does not work in 32-bit builds hyperblast/beefweb#22

Closed

azat closed this as completed Dec 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evbuffer_add_file broken when evbuffer_get_length(outbuf) > 0 and offset > 0 #306

evbuffer_add_file broken when evbuffer_get_length(outbuf) > 0 and offset > 0 #306

scottlamb commented Dec 23, 2015

errzey commented Dec 23, 2015 •

edited by azat

Loading

errzey commented Dec 23, 2015

scottlamb commented Dec 23, 2015 •

edited by azat

Loading

errzey commented Dec 23, 2015

errzey commented Dec 24, 2015

errzey commented Apr 6, 2016

paulfariello commented Jul 31, 2017

azat commented Aug 13, 2017

azat commented Aug 13, 2017

azat commented Aug 13, 2017

paulfariello commented Aug 21, 2017

azat commented Dec 17, 2017

evbuffer_add_file broken when evbuffer_get_length(outbuf) > 0 and offset > 0 #306

evbuffer_add_file broken when evbuffer_get_length(outbuf) > 0 and offset > 0 #306

Comments

scottlamb commented Dec 23, 2015

errzey commented Dec 23, 2015 • edited by azat Loading

errzey commented Dec 23, 2015

scottlamb commented Dec 23, 2015 • edited by azat Loading

errzey commented Dec 23, 2015

errzey commented Dec 24, 2015

errzey commented Apr 6, 2016

paulfariello commented Jul 31, 2017

azat commented Aug 13, 2017

azat commented Aug 13, 2017

azat commented Aug 13, 2017

paulfariello commented Aug 21, 2017

azat commented Dec 17, 2017

errzey commented Dec 23, 2015 •

edited by azat

Loading

scottlamb commented Dec 23, 2015 •

edited by azat

Loading