ThoughtGang / opdis

libopcodes-based disassembler
GNU General Public License v2.0
35 stars 10 forks source link

opdis_buf_alloc / opdis_fill_buf off-by-one #2

Closed SimonKagstrom closed 11 years ago

SimonKagstrom commented 11 years ago

This short snippet fails:

        opdis_buf_t buf = opdis_buf_alloc(size, 0);

        int v = opdis_buf_fill(buf, 0, data, size);

        if (v != size) {
            // error
        }

i.e., I just want to allocate a buffer large enough to store my instructions, and fill that with data. It fails since the opdis_buf_fill code checks for this:

int LIBCALL opdis_buf_fill( opdis_buf_t buf, opdis_off_t offset,
                void * src, opdis_off_t len ) {
    if ( ! buf || ! buf->data || ! src || ! len || 
         offset + len > buf->len ) {
        return 0;
    }

and the code fails since offset + len (0 + size) is larger than buf->len. However, without spending too much time on this issue, I think you should check for >= instead, since this is the actual size of the data.

I work-around the issue by using +1 in the buf_allocate call for now.

mkfs commented 11 years ago

I don't see the problem.

If buf is of size 256, then a fill of 256 from offset 0 would pass the test (0 + 256 == buf.len == 256). Likewise, a fill of 254 from offset 2 would pass the test (2 + 254 == buf.len == 256), as would the same fill from offset 0 (0 + 254 < buf.len == 256). A fill of 256 bytes from offset 2 would fail the test (2 + 256 > buf.length == 256).

I used the following code to recreate your problem, and it worked as expected (printing "OK" and not "ERROR"):

#include <stdio.h>
#include <opdis/opdis.h>

int main( int argc, char ** argv ) {
        int size = 256;
        char data[256] = {0};
        opdis_buf_t buf = opdis_buf_alloc(size, 0);
        int v = opdis_buf_fill(buf, 0, data, size);
        if (v != size) {
                printf("ERROR\n");
        } else {
                printf("OK\n");
        }
        return 0;
}  
SimonKagstrom commented 11 years ago

Yes, you are right. However, I don't quite understand why I got the problem - I reproduced it with your test as well. That said, I recompiled and reinstalled opdis again, and with that it went away.

I saw this fix in the log,

https://github.com/mkfs/opdis/commit/24b161d0d15930bdb2c7f027ceb7cdb3a6aee49e

which could have explained my issue, but I only cloned your git repo a few weeks ago, so for now I'll attribute it to magic.