realloc problem, corrupt last item

[Igal]

hay, i have this werid problem with my book adding function, this how
it looks

book* AddBook(book *bp, unsigned *size) {
....
//then i use realloc to allocate space for the new item in the bp
pointer
bp = (book*)realloc(bp, sizeof(book));
....
}

when i do this, the next them is added in this right space. but let's
say *bp hold 2 book type items, first item will be ok, second one will
get garbage content, then the third item (the new reallocated one)
will get the new data.
i just have no idea will the last item gets corruped, this also
happens with more items in array.

i know that to use linked lists is a lot better solution, but i still
dunno how to use them right just yet.

 

[Igal]

You have not provided sufficient information for me to help you

effectively. On general principles, lose the cast, replace sizeof(book)
with sizeof *bp, use a temp to catch the result of realloc in case it
fails and returns NULL, and make sure you have #included <stdlib.h>. Note
that bp is a copy of the calling function's pointer; the original will not
be updated automatically, so if you want to keep the new value you must
provide a mechanism for the caller to assign this new value to the
original pointer.

here's the full function.
i call it like this:

bp = AddBook(bp, &BOOK_SIZE);

/*#############################
# Add Book Function #
#############################*/
book* AddBook(book *bp, unsigned *size)
{
book temp;
unsigned n = *size; //n = size of book array
bool check = FALSE;
char str_cn[80];
int i;

printf("%d", (int)(sizeof(bp)/sizeof(bp[0])));

n++;
bp = (book*)realloc(bp, sizeof(book));
if (bp == NULL) { perror("ERROR [realloc - AddBook()]");
exit(ERR_REALOC); }


printf("\n[Add Book to Catalog]\n");

//get catalog number
while(check != TRUE)
{
printf("Book Catalog Number: ");
_flushall();
gets(str_cn);

//check if string is longer then 9 digits. if longer then error.
if(strlen(str_cn) > 9) { check = FALSE; printf("[E] catalog number
too long, 9 digit max.\n"); continue; }

//check each letter in string if a digit, if all are then check =
TRUE, else check = FALSE
i=0;
while(str_cn)
{
if(isdigit(str_cn)) { check = TRUE; i++; continue; }
else { check = FALSE; printf("[E] catalog number not valid, try
again.\n"); break; }
}

if(check == TRUE) { temp.cn = atol(str_cn); } //if all OK, put value
in str_cn
}
check = FALSE;

... here i get data into temp, and check validation ...

*size = n;

bp[n-1].cn = temp.cn;
bp[n-1].Units = temp.Units;
bp[n-1].Year = temp.Year;
bp[n-1].Price.CostPrice = temp.Price.CostPrice;
bp[n-1].Price.RetailPrice = temp.Price.RetailPrice;

strcpy(bp[n-1].Publisher, temp.Publisher);
strcpy(bp[n-1].BookName, temp.BookName);
strcpy(bp[n-1].Author.Author1, temp.Author.Author1);
strcpy(bp[n-1].Author.Author2, temp.Author.Author2);
strcpy(bp[n-1].Author.Author3, temp.Author.Author3);

return bp;
}

 

[Jim Langston]

hay, i have this werid problem with my book adding function, this how
it looks

book* AddBook(book *bp, unsigned *size) {
...
//then i use realloc to allocate space for the new item in the bp
pointer
bp = (book*)realloc(bp, sizeof(book));

realloc accepts the pointer to the prevoiusly allocated block, and the new
size. Since you are passing sizeof( book ) you are only allocating enough
space for 1 item.

You probably meant something like:
bp = realloc( bp, sizeof( book ) * *size );
or
bp = realloc( bp, sizeof( book ) * (*size + 1) )
or something. I'm not sure how many additional items you want to allocate
for nor what size represents (new size or old size).

 

[Igal]

You have not provided sufficient information for me to help you

effectively. On general principles, lose the cast, replace sizeof(book)
with sizeof *bp, use a temp to catch the result of realloc in case it
fails and returns NULL, and make sure you have #included <stdlib.h>. Note
that bp is a copy of the calling function's pointer; the original will not
be updated automatically, so if you want to keep the new value you must
provide a mechanism for the caller to assign this new value to the
original pointer.

here's the full function.
i call it like this:

bp = AddBook(bp, &BOOK_SIZE);

/*#############################
# Add Book Function #
#############################*/
book* AddBook(book *bp, unsigned *size)
{
book temp;
unsigned n = *size; //n = size of book array
bool check = FALSE;
char str_cn[80];
int i;

n++;
bp = (book*)realloc(bp, sizeof(book));
if (bp == NULL) { perror("ERROR [realloc - AddBook()]");
exit(ERR_REALOC); }

.... here i get data into temp, and check validation ...

*size = n;

bp[n-1].cn = temp.cn;
bp[n-1].Units = temp.Units;
bp[n-1].Year = temp.Year;
bp[n-1].Price.CostPrice = temp.Price.CostPrice;
bp[n-1].Price.RetailPrice = temp.Price.RetailPrice;

strcpy(bp[n-1].Publisher, temp.Publisher);
strcpy(bp[n-1].BookName, temp.BookName);
strcpy(bp[n-1].Author.Author1, temp.Author.Author1);
strcpy(bp[n-1].Author.Author2, temp.Author.Author2);
strcpy(bp[n-1].Author.Author3, temp.Author.Author3);

return bp;
}

 

[Igal]

realloc accepts the pointer to the prevoiusly allocated block, and the new
size. Since you are passing sizeof( book ) you are only allocating enough
space for 1 item.

You probably meant something like:
bp = realloc( bp, sizeof( book ) * *size );
or
bp = realloc( bp, sizeof( book ) * (*size + 1) )
or something. I'm not sure how many additional items you want to allocate
for nor what size represents (new size or old size).

in this function i need to reallocate only space for one new item.
*size is the number of items i have in my array.
correct me if i'm wrong, but if i realloc the size of my array+1,
won't this allocate + n items to the original array?

 

[Jim Langston]

You have not provided sufficient information for me to help you
effectively. On general principles, lose the cast, replace
sizeof(book) with sizeof *bp, use a temp to catch the result of
realloc in case it fails and returns NULL, and make sure you have
#included <stdlib.h>. Note that bp is a copy of the calling
function's pointer; the original will not be updated automatically,
so if you want to keep the new value you must provide a mechanism
for the caller to assign this new value to the original pointer.

here's the full function.
i call it like this:

bp = AddBook(bp, &BOOK_SIZE);

/*#############################
# Add Book Function #
#############################*/
book* AddBook(book *bp, unsigned *size)
{
book temp;
unsigned n = *size; //n = size of book array
bool check = FALSE;
char str_cn[80];
int i;

printf("%d", (int)(sizeof(bp)/sizeof(bp[0])));

bp is a book*. Therefore, it's size is the size of a pointer (most likely 4
bytes). This is most likely not giving you what you want. You need to keep
track of the size for dynamically allocated arrays.

n++;
bp = (book*)realloc(bp, sizeof(book));

And here you are allocating space for *one* book. You need to allocate size
+ 1 (which is in n at this point) so this line should become:

bp = realloc( bp, sizeof(book) * n );

if (bp == NULL) { perror("ERROR [realloc - AddBook()]");
exit(ERR_REALOC); }

printf("\n[Add Book to Catalog]\n");

//get catalog number
while(check != TRUE)
{
printf("Book Catalog Number: ");
_flushall();
gets(str_cn);

//check if string is longer then 9 digits. if longer then error.
if(strlen(str_cn) > 9) { check = FALSE; printf("[E] catalog number
too long, 9 digit max.\n"); continue; }

//check each letter in string if a digit, if all are then check =
TRUE, else check = FALSE
i=0;
while(str_cn)
{
if(isdigit(str_cn)) { check = TRUE; i++; continue; }
else { check = FALSE; printf("[E] catalog number not valid, try
again.\n"); break; }
}

if(check == TRUE) { temp.cn = atol(str_cn); } //if all OK, put value
in str_cn
}
check = FALSE;

... here i get data into temp, and check validation ...

*size = n;

bp[n-1].cn = temp.cn;
bp[n-1].Units = temp.Units;
bp[n-1].Year = temp.Year;
bp[n-1].Price.CostPrice = temp.Price.CostPrice;
bp[n-1].Price.RetailPrice = temp.Price.RetailPrice;

strcpy(bp[n-1].Publisher, temp.Publisher);
strcpy(bp[n-1].BookName, temp.BookName);
strcpy(bp[n-1].Author.Author1, temp.Author.Author1);
strcpy(bp[n-1].Author.Author2, temp.Author.Author2);
strcpy(bp[n-1].Author.Author3, temp.Author.Author3);

return bp;
}

 

[Jim Langston]

in this function i need to reallocate only space for one new item.
*size is the number of items i have in my array.
correct me if i'm wrong, but if i realloc the size of my array+1,
won't this allocate + n items to the original array?

No, realloc accpets the *new size* Total new size. How big you want the
array to be. Reguardless of how big it currently is. Which is current size
+ 1, or in your case (*size + 1) or with the source you gave before, n.

 

[Kenneth Brody]

Igal wrote:
[...]

bp = (book*)realloc(bp, sizeof(book));

Here, you allocate room for one book.

I assume you really want:

bp = realloc(bp, sizeof(*bp) * n);

which will allocate room for n books.

[...]

bp[n-1].cn = temp.cn;
bp[n-1].Units = temp.Units;
bp[n-1].Year = temp.Year;
bp[n-1].Price.CostPrice = temp.Price.CostPrice;
bp[n-1].Price.RetailPrice = temp.Price.RetailPrice;

Unless n==1, all of the above invoke UB by acessing memory
that does not belong to bp.

[...]

--
+-------------------------+--------------------+-----------------------+
| Kenneth J. Brody | www.hvcomputer.com | #include |
| kenbrody/at\spamcop.net | www.fptech.com | <std_disclaimer.h> |
+-------------------------+--------------------+-----------------------+
Don't e-mail me at: <mailto:[email protected]>

 

[K. Jennings]

Igal said:


You have not provided sufficient information for me to help you
effectively. On general principles, lose the cast, replace sizeof(book)
with sizeof *bp,

Why? Under what circumstances they would not return the same
value?

 

[Kenneth Brody]

Why? Under what circumstances they would not return the same
value?

In six months, when bp is changed to "book_v2 *".

And, removing the unnecessary cast can uncover a hidden problem if
the proper header file isn't included.

--
+-------------------------+--------------------+-----------------------+
| Kenneth J. Brody | www.hvcomputer.com | #include |
| kenbrody/at\spamcop.net | www.fptech.com | <std_disclaimer.h> |
+-------------------------+--------------------+-----------------------+
Don't e-mail me at: <mailto:[email protected]>

 

[Antoninus Twink]

The OP replied to the above, quoting a little of it, and then Chuck replied
to that reply:

I'm curious to know what information Chuck thinks he added here.

He added a syntax error in the if line, and gave the OP an example of
hideous C style: his idiotic inistence on one-line if statements that
makes code hard-to-read and debugging even harder, plus a completely
vacuous comment.

 

[Eligiusz Narutowicz]

He added a syntax error in the if line, and gave the OP an example of
hideous C style: his idiotic inistence on one-line if statements that
makes code hard-to-read and debugging even harder, plus a completely
vacuous comment.

Does he really help people? But I must be in agreement with you that
this style would not be allowed on my projects. It is horrible and very
amateur. Possibly Chuck is a new C programmer and learning though it is
better to be patient.