Back to General and Gameplay Programming

Any buffer over runs in this char 'toHexStr' code?

General and Gameplay Programming Programming

Started by IFooBar November 16, 2011 08:47 AM

19 comments, last by SiCrane 12 years, 4 months ago

IFooBar

906

Author

November 16, 2011 08:47 AM

Hi guys, does this code have any safety issues?





const char *tohexstr1(const char *src, size_t srclen, char* dst, int dstlen)

{

    assert(dstlen > 1);

    char temp[3];

    int len = 0;

    *dst = 0;

    for (size_t i = 0; i < srclen; ++i) {

        sprintf(temp, "%02X", src);

        temp[2] = 0;

        len += 2;



        if (len < dstlen)

            strcat(dst, temp);

    }

    return dst;

}

Thanks in advance!

[size=2]aliak.net

BradBobak_68669

137

November 16, 2011 09:42 AM

looks ok if srclen doesn't include '\0' (but it looks like its raw data anyhow).

this won't build the entire string if dstlen is too small.. (if this isn't intentional, you could just 'if (srclen * 2 <= dstlen) doesnt_fit;' at the start.

you could avoid the temp, and just use a 'sprintf(dst + len, "%02x", src)' (sprintf will append a '\0').

maybe you meant 'assert(dstlen >= 1)', since srclen may be 0.

IFooBar

906

Author

November 16, 2011 10:07 AM

Thanks! Yeah the temp can be avoided indeed.
Entire string doesn't have to be built so that's ok.
Assert(dstlen >= 1) it should be.

Also, when srclen does include the \0 then that's considered a safer case then when it does not right?

[size=2]aliak.net

BradBobak_68669

137

November 16, 2011 10:08 AM

If srclen does include the '\0', you will have a "00" appended to dst.

rip-off

11,000

November 16, 2011 11:29 AM

You aren't checking if src or dst are null. You aren't checking the return value of sprintf() (though it would be very surprising for it to fail). The function doesn't indicate if it fails to convert the buffer to hex (e.g. src or dst null, not enough room in dst).

Using assert() rather than an unconditional test allows that test to be skipped in production code. If you're serious about buffer overruns and security, you're serious enough to always do the check.

I can't see anything else, but its been a while since I've been immersed in low level C. It does without saying that the function puts pressure on the callers to ensure that the sizes match the actual sizes of the buffers.

If srclen does include the '\0', you will have a "00" appended to dst.
[/quote]
Is that not what is desired? I'm not sure what your objection to the case there src includes a NUL character is?

blog | twitter

Adam_42

3,664

November 16, 2011 12:50 PM

[s]I think it can overflow dest by two entries. For example try this:



char test[5];

test[3] = 0;

test[4] = 'z';

tohexstr1("aa", 2, test, 3);

assert(test[3] == 0);

assert(test[4] == 'z');

[/s]

Edit: This won't make it overflow.

rip-off

11,000

November 16, 2011 03:32 PM

Your test seems to pass Adam_42?

blog | twitter

Adam_42

3,664

November 16, 2011 04:19 PM

Ahh yeah, oops. I didn't check it with a compiler and didn't quite work the logic through in my head correctly. Somehow I thought the +=2 was in the wrong place compared to the test.

SiCrane

11,840

November 16, 2011 05:34 PM

Fun fact: integral types like char, undergo integer promotion when passed to ellipses functions like sprintf(). That means that if char is signed on your platform (which is a popular choice), int is 32-bits, and if src is 0xFF, sprintf("%02X", src) won't try to write "FF" to the buffer, it'll try to write "FFFFFFFF" to the buffer. Your buffer has a length of 3.

Less of a concern is that if char isn't 8 bits on your platform, you may also get a buffer overflow. I'd still stick in a static assertion of some sort.

alvaro

21,604

November 16, 2011 06:17 PM

Yes, there is a possible buffer overflow. If the input contains negative chars (it is up to the compiler whether char is a signed or unsigned type), sprintf will write past the end of temp.

Any buffer over runs in this char 'toHexStr' code?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Any buffer over runs in this char 'toHexStr' code?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines