Issue with memory allocation and file reading

[paragon.john]

Hello all,

I am trying to read a file into some allocated memory as part of a
program and I am running into a problem. The program gets a
segmentation fault during fread. I have previously used this code on
64-bit RHEL4 without any problems but I am not having this issue on 32-
bit RHEL5. I have simplified the code to it's most basic form and I
am still seeing the issue. Below is a full program that causes the
error. If anybody knows what may be causing the problem, help would
be greatly appreciated....

#include <stdio.h>
#include <fcntl.h>

int main()
{
int data_size = 1024*1024;
u_char *data_buf;
FILE *data_file;
int data_num;

data_buf = (unsigned char *) valloc(data_size);
printf("Memory allocated.\n");
data_file = fopen("data_file","rb");
printf("File opened.");
data_num = fread(data_buf, sizeof(u_char), data_size, data_file);
printf("File read.");
return 0;
}

Thanks for your help!

 

[Jens Thoms Toerring]

I am trying to read a file into some allocated memory as part of a
program and I am running into a problem. The program gets a
segmentation fault during fread. I have previously used this code on
64-bit RHEL4 without any problems but I am not having this issue on 32-
bit RHEL5. I have simplified the code to it's most basic form and I
am still seeing the issue. Below is a full program that causes the
error. If anybody knows what may be causing the problem, help would
be greatly appreciated....

#include <stdio.h>
#include <fcntl.h>

int main()
{
int data_size = 1024*1024;

Why not make that a 'size_t' so that you can be sure that
the value actually fits in? An int doesn't need to be
able to hold more than 16 bits, which wouldn't be enough.

u_char *data_buf;

What's wrong with plain old 'unsigned char'?

FILE *data_file;
int data_num;

Also this value should be a 'size_t', that's the type
that fread() returns.

data_buf = (unsigned char *) valloc(data_size);

Rather likely the problem is here. First of all, valloc()
isn't a standard C function - and even the Linux man page
describes it as "obsolete", so why don't you use malloc()?

But the real problem is rather likely that there's no
declaration for the valloc() function in scope since you
didn't include <stdlib.h>. By using the cast in front of
valloc() you silenced the compiler but you didn't solve
the underlying problem: since there's no declaration the
compiler assumes that valloc() returns an int an will treat
the return value accordingly - it may store it somewhere
where an int but not a pointer fits (or, if you're on a
machine with dedicated data and address registers, the
return value gets passed back via a address register but
the caller, expecting an int, i.e. data, tries to pull it
from a data register). And this crippled or just random
value is then converted back into a pointer which doesn't
point to the memory that was allocated...

So never cast the return value of malloc() (and other
functions) unless you have a very good reason. While you
can silence the compiler that way you just keep it from
giving you valuable hints.

printf("Memory allocated.\n");

You can't be sure since you didn't check the return value
of valloc().

data_file = fopen("data_file","rb");
printf("File opened.");

That's also something you can't be sure about since you
also don't test the return value of fopen().

data_num = fread(data_buf, sizeof(u_char), data_size, data_file);
printf("File read.");

And again it would make sense to check the return value of
fread().

return 0;
}

Regards, Jens

 

[viza]

Hi

The program gets a segmentation fault during fread.
#include <stdio.h>
#include <fcntl.h>

int main()
{
int data_size = 1024*1024

* sizeof(u_char);

u_char *data_buf;
FILE *data_file;
int data_num;

data_buf = (unsigned char *) valloc(data_size);

if( data_buf )

printf("Memory allocated.\n");

else
printf("Memory NOT allocated.\n");

data_file = fopen("data_file","rb");
printf("File opened.");
data_num = fread(data_buf, sizeof(u_char), data_size, data_file);
printf("File read.");
return 0;
}

You need to include the size of whatever u_char is. I guess you think
that it is 1, but perhaps it isn't. You haven't included how you
typedef it.

You also must always check the return value of every function that can
fail. I've done the one that is most likely to cause segfault.

Also, valloc is documented as 'obsolete'. Use posix_memalign() if
your c library supports it.

HTH

viza

 

[paragon.john]

Hello all,

I am trying to read a file into some allocated memory as part of a
program and I am running into a problem. The program gets a
segmentation fault during fread. I have previously used this code on
64-bit RHEL4 without any problems but I am not having this issue on 32-
bit RHEL5. I have simplified the code to it's most basic form and I
am still seeing the issue. Below is a full program that causes the
error. If anybody knows what may be causing the problem, help would
be greatly appreciated....

#include <stdio.h>
#include <fcntl.h>

int main()
{
int data_size = 1024*1024;
u_char *data_buf;
FILE *data_file;
int data_num;

data_buf = (unsigned char *) valloc(data_size);
printf("Memory allocated.\n");
data_file = fopen("data_file","rb");
printf("File opened.");
data_num = fread(data_buf, sizeof(u_char), data_size, data_file);
printf("File read.");
return 0;

}

Thanks for your help!

Thank you for the help guys. I've figured it out. I feel dumb for
not checking the return values on the functions.

 

[Peter Nilsson]

<fcntl.h> doesn't seem to be needed at all.

It's not a standard C header either.

Why not make that a 'size_t' so that you can be sure that
the value actually fits in?

There is no guarantee that 1048576 will fit into a size_t.

An int doesn't need to be able to hold more than 16 bits,
which wouldn't be enough.

Since 1024 is just as int, then 1024*1024 can overflow,
irrespective of what you assign it to.

Rather likely the problem is here. First of all, valloc()
isn't a standard C function - and even the Linux man page
describes it as "obsolete", so why don't you use malloc()?

But the real problem is rather likely that there's no
declaration for the valloc() function in scope since you
didn't include <stdlib.h>.

Note that a mere delcaration is not sufficient. You may
need a prototype, especially if you're passing an int
argument for a size_t parameter.

So never cast the return value of malloc() (and other
functions) unless you have a very good reason.

Prevention is better than cure. Requiring prototypes to
be in scope is a better suggestion IMO. Sadly, the option
actually breaks implementation conformance, although
I find the cost to be well worth it.

<snip>