Problems with a malloc'd char* to String

[Axel]

Hiho,

here my Newbie question (Win32 GUI):

I'm reading a file in binary mode to a char* named buffer. I used
malloc(filesize) to make the needed space avaiable. The filedata in the
buffer seems to be ok..I can write the variable buffer back to a file
and the contents is ok. So I think until here all goes fine...

But now I want write the buffer contents to a Memo-Box for displaying.
It works...but only the first 4 or 5 letters are shown. I have no idea
how to get the whole buffer-variable into the String of the MemoBox
component for displaying.
Here is the part of my code:

..
..
..
int fsize = FileSizeByName(file); //Filesize
char *buffer = (char*)malloc(fsize); //allocate buffer for filedata
fseek(filereader, SEEK_SET, 0);
fread(buffer,fsize,1,filereader); //read binary data in buffer
Memo1->Text = buffer; //Memo1 displays buffer as String
free(buffer);
..
..
..

I hope I illustrated the problem right and somebody has an idea whats wrong.

Thanks in advance!!

Axel

 

[Peter van Merkerk]

Hiho,

here my Newbie question (Win32 GUI):

I'm reading a file in binary mode to a char* named buffer. I used
malloc(filesize) to make the needed space avaiable. The filedata in the
buffer seems to be ok..I can write the variable buffer back to a file
and the contents is ok. So I think until here all goes fine...

But now I want write the buffer contents to a Memo-Box for displaying.
It works...but only the first 4 or 5 letters are shown. I have no idea
how to get the whole buffer-variable into the String of the MemoBox
component for displaying.
Here is the part of my code:
.
int fsize = FileSizeByName(file); file://Filesize
char *buffer = (char*)malloc(fsize); file://allocate buffer for filedata
fseek(filereader, SEEK_SET, 0);
fread(buffer,fsize,1,filereader); file://read binary data in buffer
Memo1->Text = buffer; file://Memo1 displays buffer as String
free(buffer);
.
I hope I illustrated the problem right and somebody has an idea whats

wrong.

After seeing Memo1->Text I think you are using Borland C++ Builder with
VCL controls. The Memo control and the FileSizeByName() function are not
part of the C++ standard. Questions specific to the Borland compiler and
VCL library should go to one of the newsgroups on the
newsgroups.borland.com newsserver, as this group only discusses the
standard C++ language. See also: http://www.slack.net/~shiva/welcome.txt
and http://home.wanadoo.nl/efx/c++-faq/.

Though it is difficult to tell what goes wrong without knowing what
exactly is being read from the file I suspect your problem is that the
string read from the file is not zero terminated. The (untested) code
below should fix that:

int fsize = FileSizeByName(file);
char *buffer = new char[fsize+1];
fseek(filereader, SEEK_SET, 0);
fread(buffer,fsize,1,filereader);
buffer[fsize] = 0; // Zero terminate string.
Memo1->Text = buffer;
delete []buffer;

HTH

 

[Axel]

After seeing Memo1->Text I think you are using Borland C++ Builder with
VCL controls. The Memo control and the FileSizeByName() function are not
part of the C++ standard. Questions specific to the Borland compiler and
VCL library should go to one of the newsgroups on the
newsgroups.borland.com newsserver, as this group only discusses the
standard C++ language. See also: http://www.slack.net/~shiva/welcome.txt
and http://home.wanadoo.nl/efx/c++-faq/.

Though it is difficult to tell what goes wrong without knowing what
exactly is being read from the file I suspect your problem is that the
string read from the file is not zero terminated. The (untested) code
below should fix that:

int fsize = FileSizeByName(file);
char *buffer = new char[fsize+1];
fseek(filereader, SEEK_SET, 0);
fread(buffer,fsize,1,filereader);
buffer[fsize] = 0; // Zero terminate string.
Memo1->Text = buffer;
delete []buffer;

HTH

Hi,

thanks for your ideas. Please forget the Memo1->Text..it could be a
simple String-type variable too. Memo1 is only used for testing what
buffer contains. Later it will be something like:

String str = buffer;

And very later the end-goal is to code str with base64. But thats not
the problem at the moment.

But str only contains the first view chars of char*. Your solution
brings the same result. :-(

Axel

 

[Peter van Merkerk]

Hi,

thanks for your ideas. Please forget the Memo1->Text..it could be a
simple String-type variable too. Memo1 is only used for testing what
buffer contains. Later it will be something like:

String str = buffer;

And very later the end-goal is to code str with base64. But thats not
the problem at the moment.

But str only contains the first view chars of char*. Your solution
brings the same result. :-(

I cannot see obvious errors in the (incomplete) code you posted. Are you
sure the file doesn't have 0 bytes in it? Did you open the file in
binary or text mode? It would help if you post minimal but complete
(==compilable) code that exhibits the problem.

 

[Axel]

I cannot see obvious errors in the (incomplete) code you posted. Are you
sure the file doesn't have 0 bytes in it? Did you open the file in
binary or text mode? It would help if you post minimal but complete
(==compilable) code that exhibits the problem.

Ok..here is the complete codepart. The file is ok. Like written before I
can write it back with fwrite from the buffer-variable.

....
FILE *filereader = fopen("c:\\test\\original.doc","rb");
if(!filereader)
{
//Errormessage: File not found
}
else
{
int fsize = FileSizeByName(file);
char *buffer = (char*)malloc(fsize);
fseek(filereader, SEEK_SET, 0);
fread(buffer,fsize,1,filereader);

String s = (String)buffer;

//for testing only lets write the file back
//and check if it is equal to the origin
FILE *filewriter = fopen("c:\\test\\copy.doc","wb");
fwrite(buffer,fsize,1,filewriter);
fclose(filewriter);
free(buffer);
}
fclose(filereader);
....

String s contains not the whole buffer. Have I to allocate memory for
it? I have no idea.

Axel

 

[Default User]

Peter van Merkerk wrote:

FILE *filereader = fopen("c:\\test\\original.doc","rb");

You can write that filename as "c:/test/original.doc".

int fsize = FileSizeByName(file);

Nonstandard function. However, if you have the filename in the variable
file, why not use that above in the fopen() call?

char *buffer = (char*)malloc(fsize);

In C++, new is preferred to malloc(), although for a basic type it
doesn't matter much.

fseek(filereader, SEEK_SET, 0);

rewind() is shorter.

fread(buffer,fsize,1,filereader);

String s = (String)buffer;

String is nonstandard. It would be a good idea to use std::string
instead.

String s contains not the whole buffer.

As you were reading in binary mode, not text mode, it is likely that
there was a byte equal to 0, which will truncate the string. Why are you
opening in binary? If it doesn't contain text, why are you trying to put
it in a string?




Brian Rodenborn

 

[Kevin Goodsell]

Ok..here is the complete codepart. The file is ok. Like written before I
can write it back with fwrite from the buffer-variable.

This is not a complete program. I cannot copy/paste it into my editor,
save & compile it.

...
FILE *filereader = fopen("c:\\test\\original.doc","rb");

When there's an input file for the program, it helps if you supply it as
well.

if(!filereader)
{
//Errormessage: File not found
}
else
{
int fsize = FileSizeByName(file);
char *buffer = (char*)malloc(fsize);
fseek(filereader, SEEK_SET, 0); fread(buffer,fsize,1,filereader);
String s = (String)buffer;

"String" is not a standard C++ type, so we can't really help you with
that. I don't know the interface for it, so I don't know how one would
convert a C-style string to a String.

That cast looks highly suspicious, though. I strongly recommend getting
rid of it. If this causes errors, then it's almost certain that the cast
is only shutting up the compiler and forcing it to generate incorrect code.

If you were using std::string instead of this "String" thing, you could
do something like this:

std::string s(buffer, fsize);

I have no idea what the equivalent thing for "String" would be.

//for testing only lets write the file back
//and check if it is equal to the origin
FILE *filewriter = fopen("c:\\test\\copy.doc","wb");
fwrite(buffer,fsize,1,filewriter);
fclose(filewriter);
free(buffer);
}
fclose(filereader);
...

String s contains not the whole buffer. Have I to allocate memory for
it? I have no idea.

I'd say it's almost certain that you are using String incorrectly. But
if you want help with it, you'll have to ask on a group that supports
whatever library it comes from.

-Kevin

 

[Kevin Goodsell]

In C++, new is preferred to malloc(), although for a basic type it
doesn't matter much.

That's true, assuming that you use malloc() correctly. It just so
happens that people use it incorrectly quite often. 'new' avoids many of
the problems people have with malloc().

-Kevin

 

[Default User]

That's true, assuming that you use malloc() correctly. It just so
happens that people use it incorrectly quite often. 'new' avoids many of
the problems people have with malloc().

Mainly due to typechecking, or lack thereof.

The biggest problem really comes with mixing the two forms, because you
can end up deleting memory allocated with malloc() or freeing memory
allocated via new. Bad things.




Brian Rodenborn

 

[Kevin Goodsell]

Mainly due to typechecking, or lack thereof.

The size calculation, also. People get that wrong quite often.

The biggest problem really comes with mixing the two forms, because you
can end up deleting memory allocated with malloc() or freeing memory
allocated via new. Bad things.

I haven't really seen this problem arise a lot, but it certainly would
not come as a surprise in code using both.

-Kevin

 

[llewelly]

Mainly due to typechecking, or lack thereof.

The biggest problem really comes with mixing the two forms, because you
can end up deleting memory allocated with malloc() or freeing memory
allocated via new. Bad things.

In my experience, this is only common in projects that started out C
and were 'converted' to C++ .

 

[Default User]

Default User wrote:

The size calculation, also. People get that wrong quite often.

If you use the handy-dandy comp.lang.c method, it's harder to screw up:

char *str;

str = (char*) malloc (num * sizeof *str);

I haven't really seen this problem arise a lot, but it certainly would
not come as a surprise in code using both.

I've seen it. It's one of those things that'll probably work out ok, but
it's UB.



Brian Rodenborn

 

[Default User]

In my experience, this is only common in projects that started out C
and were 'converted' to C++ .

Which are the ones likely to have malloc() and friends.




Brian Rodenborn

 

[Noah Roberts]

Funny you should demo it with the one type with which it is usually a
mistake. Most people point a char* at a malloc'd block to have
dynamicly sized string. That block, if used for a string, would be
a very short string. ('\0' terminator only.)

Actually that depends on what num is. First off, I would like to say
that I object to the use of *str as the argument of sizeof. It is much
more confusing to say that rather than "sizeof char" (or in my taste:
"sizeof(char)") which is really what you want. It is also too easy to
make the mistake of leaving the '*' out and then you have a different
size all together. A third reason I don't like using *str is that it
looks to much like you are multiplying str by num and 'sizeof'. Of
course anyone who knows C knows that isn't the case but even then that
is what the mind wants to do with it. In reality the above code is
really bad in the readability area; it is totally correct and makes
sence but doesn't look right and doesn't taste well in my brain.

I would preffer:

char *str = NULL; // or 0 if you insist

str = (char*)malloc((num + 1) * sizeof(char)); // +1 because num is
probably the length of my array.

 

[Kevin Goodsell]

Actually that depends on what num is. First off, I would like to say
that I object to the use of *str as the argument of sizeof. It is much
more confusing to say that rather than "sizeof char"

That would be illegal. And in my opinion, it's not any more confusing.
Idioms are only confusing the first few times.

(or in my taste:
"sizeof(char)") which is really what you want.

Not me. I want n objects of the type str points to, whatever that may be.

I don't like having (artificial) dependencies between the variable and
it's type, nor do I like language features which require that. They tend
to cause errors. Examples: printf/scanf require explicit knowledge of
the type involved, and are frequently used incorrectly. <limits.h>
macros for min and max value also require explicit knowledge of the type
involved. As far as I can tell, hungarian notation was created primarily
to deal with this type of problem, but it takes the wrong approach by
tagging the extra information on rather than eliminating the dependency.
Note that C++ replaces both of the examples with things that
automatically work for whatever type you feed them (overloads for I/O,
std::numeric_limits requires the type, but at least it allows the type
to be aliased with a typedef). This is nice, because types can change
without much concern, maintenance is simplified, and errors are reduced
because there are simply fewer opportunities for them.

Now, the comp.lang.c-approved method for mallocing is not quite what
Brian posted, it's actually this:

// given:
char *str;

str = malloc(n * sizeof *str);

Without the cast, there is nothing that needs to be updated if str
changes to unsigned char *, or wchar_t *, or whatever.

Of course, this is not valid in C++. The cast is required. So you can't
escape having type information in the malloc call statement. That's part
of what makes 'new' better - no cast. And implicit type-checking. But it
does still require type information, which has always bugged me a
little. I'd like to use:

p = new typeof(*p)[n];

Well, maybe not, because it's kind of ugly. Instead, I could do this:

typedef int mytype;

mytype *p;
// ...
p = new mytype[n];

Which handles changing the type - you just update the typedef instead.
This generally isn't so bad - you can give the type a name that
describes its use, and drop in a replacement any time (maybe you need a
wider integer type, or you create a class that's more suited to the task).

I'm kind of straying from my main point, though. Basically, it's this: I
prefer to limit the number of related occurrences of a type name in a
module as much as possible, if there's a chance the type could be
updated later. More generally, the goal is to reduce points of
dependency within the source code, making simple maintenance a breeze.
Applying that idea to malloc, the comp.lang.c-approved method prevents
introducing points of dependency. 'new' makes it difficult, but aliasing
the type behind a typedef can fix it. The same fix would technically
work for malloc:

p = (mytype *)malloc(n * sizeof(mytype));

I don't see any maintenance problems or excessive points of dependency.
But it does tie the statement to the typedefed type, if not the
underlying type, so it's not quite as good as the comp.lang.c-approved
method, which ties the statement to absolutely no particular type at all.

It is also too easy to
make the mistake of leaving the '*' out and then you have a different
size all together.

It's easy to make a lot of typos. It's an idiom for a reason - people
should get used to using it, used to seeing it, and immediately spot a
problem like that if it occurs. It's not an invalid concern, but it's a
relatively minor one.

-Kevin

 

[Noah Roberts]

p = (mytype *)malloc(n * sizeof(mytype));

Yes, if there is any chance of p changing type this might be a good
idea. However, I don't think it would be good to do this a lot because
then you have your namespace riddled with a lot of typedefs. There are
work arounds, but I see it getting ugly fast. IMHO you would only want
to do this if your 'p' has a wide scope and gets allocated a lot, which
isn't too terribly often since we have classes like std::vector to
protect us from that need.

Of course we also have templates so I think it rather rare that you
would need to use a malloc where you are unaware of what the type is
going to be in which you need to know. But your point is well taken and
should be kept in mind for those rare cases.

NR

 

[llewelly]

Funny you should demo it with the one type with which it is usually a
mistake. Most people point a char* at a malloc'd block to have
dynamicly sized string. That block, if used for a string, would be
a very short string. ('\0' terminator only.)

Funny I completely missed 'num' ... *sigh*

 

[Default User]

Now, the comp.lang.c-approved method for mallocing is not quite what
Brian posted, it's actually this:

// given:
char *str;

str = malloc(n * sizeof *str);

Yeah, but I did want to post something topical for CLC++

It works a lot better in C case, as you mention later.


All in all, use new in C++. It's shorter and it's type safe. For a POD
type, a good compiler will likely produce virtually identical code
anyway.



Brian Rodenborn