I have a problem to allocate memory.(maybe)

Question

There is input file; unsorted long type numbers.(about 1 million) I want to sort numbers in input files. To allocate the memory of array, I used fseek and ftell. But segmentation fault is occurred. How do i fix the code?

int main( int argc, char *argv[] )
{
    long *arr;
    FILE *fp = NULL;
    FILE *fp2 = NULL;
    int   i = 0;
    long  size, count;

    fp  = fopen( "argv[1]", "r" );
    fp2 = fopen( "sorted",  "w" );
    if ( fp == NULL || fp2 == NULL )
        printf( "error \n" );
    else
    {
        fseek( fp, 0, SEEK_END );
        size = ftell( fp );
        arr = ( long * ) malloc( size );
        count = fread( arr, size, 4, fp );

        Quicksort( arr, 0, sizeof( arr ) / sizeof( int ) - 1 );

        while ( i != ( sizeof( arr ) / sizeof( int ) - 1 ) )
        {
            int i = 0;
            int j;
            for ( j = 0; j < 5; j++ )
                fwrite( arr, size, 1, fp2 );
        }
    }
    fclose( fp );
    free( arr );
    fclose( fp2 );
    return 0;
}

I think you mean `fp = fopen( "argv[1]", "r" );` should be `fp = fopen( argv[1], "r" );` (i.e. no quotes). — Kingsley, Apr 08 '19 at 05:03
@Andreas Then how do i fix the code? I want to read all numbers in the file. — su00, Apr 08 '19 at 05:06
This `sizeof( arr ) / sizeof( int )` won't do what you expect, as `arr` is pointer and *an* an array! — alk, Apr 08 '19 at 06:34

score 4 · Answer 1 · answered Apr 08 '19 at 05:07

The issue is that the code is finding the number of bytes in the file with ftell(), and then allocating whatever size amount that data is.

However, when then reading from the file, it's trying to read four times as much data, because of the count-of-elements, and size-of-elements passed to fread().

If the input file was a big block of binary longs (in the same format as the local hardware), it's possible to just fread() the lot in a single go. This means your size is the ftell() result, but the count-of-elements is one.

fseek( fp, 0, SEEK_END );
size = ftell( fp );
arr = ( long * ) malloc( size );
count = fread( arr, size, 1, fp );  // read the whole block in a single go

score 3 · Answer 2 · answered Apr 08 '19 at 05:07

There are a lot of issues here and I think this isn’t the actual code even. The ones immediately noticeable:

you’re trying to open a file called argv[1], not the filename given in command line. Drop the quotes.
you allocate memory for size bytes, then try to read size * 4 bytes and don’t check how many were read
don’t cast the return value of malloc
sizeof arr only gives you the size of a pointer, not the data you have. You have size, use it.
your while loop has a new variable with the same name as outside, neither is ever changed so the loop is eternal
if you can’t open a file you free an unassigned pointer

So check your compiler’s warnings. Think about what you’re doing, what the parameters you give to functions mean. Check return values. Use a debugger to step through the code line by line.

The exact cause can only be determined after you fix the other problems, and might even get fixed by that. If this is the exact code then the last one is probably the reason. A debugger will tell you.

score 3 · Answer 3 · answered Apr 08 '19 at 05:34

There are a bunch of things that needs attention in no particular order of severity

always validate arguments

your program doesn't seem to validate arguments which is kind of bad, a quick statement at the start will do the job

 /* Always do some argument checking, Assuming in your case command line is 2 */
    if ( argc != 2 ) 
    {
        printf("Invalid arguments ");
        printf("Usage %s <filename>\n", argv[0]);
        exit(0);
    }

always check return values

Almost always its a good Idea to check for return values and validate them, there are of course some exceptions printf for example but cases like fopen, malloc its a crime to not check for return values

/* Return Type Validations are mandatory */
    fp  = fopen( argv[1], "r" );
    if( fp == NULL )
    {
        printf("Error Opening file %s\n", argv[1]);
        exit(0);
    }

Do not cast malloc result

This is a popular one Do I cast the result of malloc the answer is no

fread

This probably down to misunderstanding function behavior, from man fread

The function fread() reads nmemb items of data, each size bytes long,
       from the stream pointed to by stream, storing them at the location
       given by ptr.

you are essentially reading 4 times the data your need by writing statement like

count = fread( arr, size, 4, fp );

mixing local and global scopes

There are multiple declarations of variable i in local ( inside for loop) and global scope do not do that.
Careful while using sizeof() for poniters

this statement sizeof( arr ) always gives the size of pointer and not the sizeof data pointed by the pointer, the below statement

Quicksort( arr, 0, sizeof( arr ) / sizeof( int ) - 1 );

did not make much sense to me, and there is no way I can evaluate if that is really a valid in context of your program.

I have a problem to allocate memory.(maybe)

3 Answers3