Can't find the possible buffer overflow in a piece of code

Question

I've been writing the following bit of code, and someone had informed me that it runs of the risk of having a buffer overflow.

Now admittedly my knowledge of buffer overflows may be not be as robust as I would like, but I thought that a buffer overflow is when the data being written will not fit within the bounderies of the buffer and will spill over to adjacent memory location.

I assumed that the problem may be related to the fread, that it wouldn't be a safe function to use, but reading through the documentation doesn't seem to tell me that its unsafe like say strcpy() is compared to strncpy(). So I'm rather uncertain on where the problem could be located or how to handle it. And also if anyone has any suggestions on where I can go (or what book to read) that would help expand my knowledge on this subject or other vulnerability weaknesses, I would be appreciative.

bool readLong(FILE *f, long *n)
{
    unsigned char *ptr,tmp;

    if (fread(n,8,1,f) != 1)
        return false;

    ptr = (unsigned char *)n;
    tmp = ptr[0];
    ptr[0] = ptr[7];
    ptr[7] = tmp;
    tmp = ptr[1];
    ptr[1] = ptr[6];
    ptr[6] = tmp;
    tmp = ptr[2];
    ptr[2] = ptr[5];
    ptr[5] = tmp;
    tmp = ptr[3];
    ptr[3] = ptr[4];
    ptr[4] = tmp;

    return true;
}

To begin with, what makes you so sure long is 8 bytes on your system? — Lundin
– Lundin, Commented Oct 23, 2020 at 13:26
We don't know what will be passed as f and n. If one-byte buffers are passed to them, there will be buffer overflow because multi-byte read and written will be performed to them. — MikeCAT
– MikeCAT, Commented Oct 23, 2020 at 13:26
if (fread(n,sizeof *n,1,f) != 1) is a first improvement. Rest of code should not assume 8 bytes for a long. And should not assume a certain endian. — chux
– chux, Commented Oct 23, 2020 at 13:43

Daniel Walker · Accepted Answer · 2020-10-23 13:29:42Z

2

You're attempting to fread 8 bytes into a long. While a long might be 8 bytes on your system, this is by no means guaranteed. If it's only 4 bytes long, then you're definitely corrupting the surrounding memory. This also comes up when you assign a value to ptr[7].

What you should be doing is fread(n,sizeof(*n),1,f). You can also replace ptr[7] with ptr[sizeof(long)-1].

answered Oct 23, 2020 at 13:29

Daniel Walker

6,9487 gold badges25 silver badges66 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

MikeCAT Over a year ago

Not only ptr[7] but also ptr[6], ptr[5], and ptr[4] are out-of-range with 4-byte long.

Daniel Walker Over a year ago

Oh, yeah. Thanks for the catch.

Collectives™ on Stack Overflow

Can't find the possible buffer overflow in a piece of code

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related