Jump to content
  • Advertisement
Sign in to follow this  
IFooBar

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

This topic is 2577 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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!

Share this post


Link to post
Share on other sites
Advertisement
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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
[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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Hidden
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.

Share this post


Link to post
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!