C pointers inconsistency

Question

I'm working on a program for a C class and I reached a point where I don't know what to do. We are implementing a String library type.

I have my header file (MyString.h)

typedef struct {
    char *buffer;
    int length;
    int maxLength;
} String;

String *newString(const char *str);

The file implementing the functions (MyString.c)

#include <stdlib.h>
#include <stdio.h>
#include "MyString.h"

String *newString(const char *str) {

// Allocate memory for the String
String *newStr = (String*)malloc(sizeof(String));

if (newStr == NULL) {
    printf("ERROR: Out of memory\n");
    return NULL;
}   

// Count the number of characters
int count;
for (count = 0; *(str + count) != '\0'; count++);
count++;

// Allocate memory for the buffer
newStr->buffer = (char*)malloc(count * sizeof(char));

if (newStr->buffer == NULL) {
    printf("ERROR: Out of memory\n");
    return NULL;
}

// Copy into the buffer
while (*str != '\0')
    *(newStr->buffer++) = *(str++);
*(++newStr->buffer) = '\0';

// Set the length and maximum length
newStr->length = count;
newStr->maxLength = count;

printf("newStr->buffer: %p\n",newStr->buffer); // For testing purposes

return newStr;
}

And a tester (main.c)

#include <stdio.h>
#include "MyString.h"

main() {
char str[] = "Test character array";

String *testString = newString(str);

printf("testString->buffer: %p\n",testString->buffer); // Testing only
}

The problem is that, even though testString is pointing to the String created in newString(), their buffers point to different memory addresses. Why is that?

Thanks in advance

Saul · Accepted Answer · 2013-07-20 14:16:00Z

3

By using *(++newStr->buffer) and *(newStr->buffer++), you're moving newStr->buffer to essentially point to the end of the string.. You need to modify your code as such:

#include <stdlib.h>
#include <stdio.h>
#include "MyString.h"

String *newString(const char *str) {
    // Allocate memory for the String
    String *newStr = (String*)malloc(sizeof(String));

    if (newStr == NULL) {
        printf("ERROR: Out of memory\n");
        return NULL;
    }

    // Count the number of characters
    int count;
    for (count = 0; *(str + count) != '\0'; count++);
    count++;

    // Allocate memory for the buffer
    newStr->buffer = (char*)malloc(count * sizeof(char));

    if (newStr->buffer == NULL) {
        printf("ERROR: Out of memory\n");
        return NULL;
    }

    char *pBuffer = newStr->buffer; // don't move newStr->buffer, have another pointer for that.

    // Copy into the buffer
    while (*str != '\0')
        *(pBuffer++) = *(str++);
    *pBuffer = '\0';

    // Set the length and maximum length
    newStr->length = count;
    newStr->maxLength = count;

    printf("newStr->buffer: %p\n", newStr->buffer); // For testing purposes

    return newStr;
}

edited Jul 20, 2013 at 14:16

answered Jun 2, 2012 at 17:54

Saul

1,0021 gold badge14 silver badges26 bronze badges

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

3 Comments

Saul Over a year ago

As suggested by the other answers I would also consider using strcpy instead of manually copying the string, too :)

Daniel Fischer Over a year ago

@fpele Also, *(++pBuffer) = '\0'; should be *pBuffer = '\0';. When the copying while ends, pBuffer already points one behind the last written character. With *(++pBuffer) = '\0';, you have one unspecified byte between the copied part and the 0-terminator you write, and, more importantly, you write past the end of the malloced memory. That is undefined behaviour.

Saul Over a year ago

Thanks Daniel, fixed in the solution.

Patrick Schlüter · Accepted Answer · 2019-10-21 07:22:52Z

As the other colleagues already pointed out, you modified your allocation pointer, which is a no no. Here your example but translated to a more "professional" way.

I would change your structure to:

typedef struct {
   char *buffer;
   size_t length;        /* strings and allocation in C are of type size_t not int */
   size_t alloclength;
} String;

String *newString(const char *str);

And the function would be changed to.

#include <stdlib.h>
#include <stdio.h>
#include "MyString.h"

String *newString(const char *str)
{
  // Allocate memory for the String
  String *newStr = malloc(sizeof (String));  /* No typecast of void * in C, it's bad taste. */

  if(!newStr) {
    fprintf(stderr, "ERROR: Out of memory\n");     /* Errors are meant to be printed on stderr, not stdio */
    return NULL;
  }   
  // Count the number of characters
  newStr->length = strlen(str);          /* Learn to use the stdlib, there are a lot of usefull functions */
  newStr->alloclength = newStr->length+1;
  // Allocate memory for the buffer
  newStr->buffer = malloc(newStr->alloclength);   /* sizeof (char) is by definition always 1 */
  if(!newStr->buffer) {
    fprintf(stderr, "ERROR: Out of memory\n");
    free(newStr);      
    return NULL;
  }
  // Copy into the buffer
  strcpy(newStr->buffer, str);   /* Because we already scaned the input with strlen, we can use safely the "unsafe" strcpy function. The strcpy will add the trailing 0 */
  printf("newStr->buffer: %p\n",newStr->buffer); // For testing purposes
  return newStr;
}

rgngl · Accepted Answer · 2012-06-02 18:45:36Z

2

You are modifying the buffer pointer inside the newly created String struct.

You should do:

char *newBuffer = newStr->buffer;
// Copy into the buffer
while (*str != '\0')
    *(newBuffer++) = *(str++);
*(++newBuffer) = '\0';

edited Jun 2, 2012 at 18:45

answered Jun 2, 2012 at 17:56

rgngl

5,4233 gold badges33 silver badges34 bronze badges

Comments

ChronoTrigger · Accepted Answer · 2013-07-20 16:04:00Z

2

The question is answered, but I think that there is a piece of code that you should add to avoid a subtle source of memory leak:

// Allocate memory for the buffer
newStr->buffer = (char*)malloc(count * sizeof(char));

if (newStr->buffer == NULL) {
    printf("ERROR: Out of memory\n");
    free(newStr); // free the memory allocated for newStr
    return NULL;
}

answered Jul 20, 2013 at 16:04

ChronoTrigger

8,6572 gold badges39 silver badges58 bronze badges

Comments

zakkak · Accepted Answer · 2012-06-02 19:41:28Z

1

The explanation is pretty simple: You are modifying the buffer pointer in the newString() function:

// Copy into the buffer
while (*str != '\0')
    *(newStr->buffer++) = *(str++);
*(++newStr->buffer) = '\0';

You could use a temporary pointer here (like suggested in the other answers), but I'd like to recommend using the standard functions provided within string.h:

// Count the number of characters
int count;
count = strlen(str) + 1;

// Copy into the buffer
memcpy(newString->buffer, str, count)

edited Jun 2, 2012 at 19:41

zakkak

2,0901 gold badge20 silver badges27 bronze badges

answered Jun 2, 2012 at 18:03

mjhennig

6811 gold badge5 silver badges11 bronze badges

Collectives™ on Stack Overflow

C pointers inconsistency

5 Answers 5

3 Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

3 Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related