-4

I'm having some trouble with memory allocation in C.

I seem to always write or read to or from a wrong spot here, there's probably something I'm missing here:

char *a = malloc(8+b+1+c+1+d+1+e+1+f+4+6);
*(a+8+b+1+c+1+d+1+e+1+f+4+6) = '\0';

snprintf(a, strlen(a), "xyz    \"%sx%sx%sx%sx%s\"     ",v, w, x, y, z);

Could anyone help me get back on track?

nacn
  • 25
  • 4
  • 3
    I don't know what you are missing, but I know for sure what your post is missing: a [mre]. – Marco Bonelli Apr 08 '21 at 23:19
  • 2
    An allocation of `n` elements has accessible elements `arr[0]` to `arr[n-1]`. – interjay Apr 08 '21 at 23:19
  • 1
    OT: Sorry to say but that code is very bad. All those hard coded numbers are only going to cause you grief in any non-trivial code. At a minimum that whole expression should be stored in a variable and not repeated anywhere. – kaylum Apr 08 '21 at 23:22
  • In the first line, `e+1+e+4` should probably be `e+1+f+4` to make the two lines match up. – Ken Y-N Apr 08 '21 at 23:22
  • 1
    `snprintf(a, strlen(a)` is most probably meant to be `snprintf(a, 8+b+1+c+1+d+1+e+1+e+4+6`. Why write zero byte? `snprintf` writes it anyway. (You may want to read about [dynamically allocating memory for snprintf](https://stackoverflow.com/questions/3774417/sprintf-with-automatic-memory-allocation)) – KamilCuk Apr 08 '21 at 23:23
  • Thanks, I'll try that. – nacn Apr 08 '21 at 23:24
  • Tip: Write `*(a + n)` as `a[n]`, it's way more readable. This converts to `a[8+b+...]`. – tadman Apr 08 '21 at 23:24
  • Tip: [DRY up](https://en.wikipedia.org/wiki/Don't_repeat_yourself) your code here, declare a variable that represents that size, like `size_t a_size = 8+b+... +1` (with +1 to include a NUL byte), then `malloc(a_size)` and `a[size-1] = 0`. – tadman Apr 08 '21 at 23:26
  • Thanks, I'll do that tadman. Doesn't seem to change much, though. – nacn Apr 08 '21 at 23:27

2 Answers2

2

I don't know what these variables are, but I think this line is wrong:

*(a+8+b+1+c+1+d+1+e+1+f+4+6) = '\0';

Try this instead:

*(a+8+b+1+c+1+d+1+e+1+f+4+6-1) = '\0';

Writing to the end of a string requires you to subtract one from the length to get the index of the last byte.

Depending on the string lengths of v, w, x, y, and z, you may also overflow the string you're writing to while using snprintf.

Grifball
  • 756
  • 1
  • 5
  • 14
  • These variables are arbitrary integers. Thanks a lot, I'll try it! – nacn Apr 08 '21 at 23:20
  • Also, I don't see any `+f` inside `malloc()`... so yeah, confusing. – Marco Bonelli Apr 08 '21 at 23:20
  • @MarcoBonelli oh, good catch, I just skimmed and thought they were the same series of numbers and variables. This feels like a crazy code golf puzzle lol. – Grifball Apr 08 '21 at 23:21
  • But, still, if there happens to be a zero byte, `strlen(a)` will return a lower value... – KamilCuk Apr 08 '21 at 23:21
  • The snprintf seems to still act up for whatever reason. The variables aren't actually called like that, I replaced the actual names. – nacn Apr 08 '21 at 23:23
  • @KamilCuk ah, yes, I missed that as well. Wow, I should've read this more carefully. nacn, try addressing the "+f" issue that Marco Bonelli mentioned – Grifball Apr 08 '21 at 23:25
1

Remember strlen() is the length of the C string and not the length of the buffer.

size_t a_size = 8+b+1+c+1+d+1+e+1+f+4+6 + 1;
char *a = calloc(a_size, sizeof(char)); // calloc() zeroes automatically

snprintf(a, a_size, "xyz    \"%sx%sx%sx%sx%s\"     ",v, w, x, y, z);

By using strlen(a) on a mostly uninitialized buffer you're basically going to get a random value between 0 and where you put that NUL byte.

You may want to get in the habit of using calloc() instead of malloc() as it will zero out the allocation for you automatically and the overhead is pretty nominal. malloc() can return all kinds of garbage which you will often need to clear out before you can use it.

tadman
  • 208,517
  • 23
  • 234
  • 262