Why am I getting this access violation?

Question

I'm probably doing something stupid here, but it's been a while since I've worked in C++ and for some reason I keep getting an access violation when sprintf is called. Anyways, here's the code I'm using:

char *value, *result;
int len;
result = "";
mgr.GetObjValue(0, value, len);

for (int i = 0; i < len; i++) 
{
  sprintf(result, "%s %X", result, value[i]);
}

printf("ObjVal: %s\n\n", result);

if anyone is curious what GetObjValue does, it just retrieves the SNMP object value from the API I am using. Here's it's declaration:

int SNMPMgr::GetObjValue(int iObjIndex, char *&lpObjValue, int &lenObjValue);

Any help would be much appreciated

In fact no space is reserved at all, `result` points to static memory … — filmor, Nov 29 '12 at 17:49
@chris - actually not even one character - result is just a dangling pointer. — user93353, Nov 29 '12 at 17:49
@filmor, You're right. I'm not thinking straight. Overwriting even that one character is bad. — chris, Nov 29 '12 at 17:49
Th pointer is pointing to a string literal, It cannot be modified, its Undefined Behavior. — Alok Save, Nov 29 '12 at 17:51
@chris - ah! I didn't see the `result = ""` - Anyway, that's a literal as Als says - it leads to undefined behaviour to modify it. — user93353, Nov 29 '12 at 17:55

Josh Kelley · Accepted Answer · 2012-11-29T17:55:10.960

sprintf doesn't do memory allocation. It expects to be given a pointer to a writable buffer of sufficient length to hold the data.

char *result;

At this point, result's contents are undefined.

result = "";

At this point, result points to a static, read-only string of 1 byte (the terminating null).

sprintf(result, "%s %X", result, value[i]);

At this point, you just tried to write an arbitrarily long string to a read-only area of size 1. Oops.

Do something like this instead:

char result[1024];
sprintf(result, "%s %X", result, value[i]);

Note that using snprintf or sprintf_s, to avoid even the possibility of overwriting your buffer, is probably a good idea. (Since you're using C++, you could also easily use one of the C++ formatting libraries, like Boost.Format, that does memory allocation for you - but that's a whole other topic.)

Even then, in the for loop, each iteration would overwrite the last iteration — user93353, Nov 29 '12 at 17:56
Thanks, as I said, I was sure I was doing something dumb and sure enough, not allocating memory is pretty bad. Thanks, for the detailed response though. — Brandon, Nov 29 '12 at 18:59

score 0 · Answer 2 · answered Nov 29 '12 at 17:49

The problem is here: char* result = "";

You're attempting to print to a constant string (the empty string). From the documentation for sprintf regarding the first parameter:

Pointer to a buffer where the resulting C-string is stored. The buffer should be large enough to contain the resulting string.

You need to allocate a buffer to print to.

Why am I getting this access violation?

2 Answers2