CODEWITHSUNDEEP
cunix
Aviral Kumar
Aviral Kumar

Reputation: 824

Segmentation Fault(core dump)

I am getting Segmentation error(core dump) in the following code:

void Update_Log( )
{
        struct logData update;
        int file;

        char *writeBuffer=NULL;

        if((file=creat("/home/user/Desktop/DMS/DMS/filename.txt",O_RDONLY|O_WRONLY))==-1)
                perror("file not opened");
        update.clientName="user";
        update.filename="user";
        update.timestamp="some time";
        sprintf(writeBuffer,"%s %s %s",update.clientName,update.filename,update.timestamp);


        if((write(file,writeBuffer,sizeof(writeBuffer)))==-1)
                perror("write unsuccessful");
        close(file);
}

My structure is as follows:

struct logData
{
        char *clientName;
        char *filename;
        char *timestamp;
};

Can anyone help in this?

Upvotes: 0

Views: 3130

Answers (7)

Werner Henze
Werner Henze

Reputation: 16781

As Binyamin Sharet wrote sprintf is not allocating the output buffer. The caller must provide an output buffer that is long enough. This is not always simple. Therefor I advise you to use fopen instead of open. Then you can code like that:

void Update_Log( )  
{  
        struct logData update;  
        FILE *file = fopen("/home/user/Desktop/DMS/DMS/filename.txt", "w");  

        if(file==NULL) {  
                perror("file not opened");  
                return;
        }
        update.clientName="user";  
        update.filename="user";  
        update.timestamp="some time";  
        if(fprintf(file,"%s %s %s",update.clientName,update.filename,update.timestamp) < 0)  
                perror("write unsuccessful");  
        fclose(file);

}

As you can see the code is shorter and it has no potential buffer overflow.

Upvotes: 0

Stephane Rouberol
Stephane Rouberol

Reputation: 4384

your struct store only pointers, no memory is allocated for the actual data. Either you should have

struct logData
{
        char clientName[SOMEDEFINE1NUMBERHERE];
        char filename[SOMEDEFINE2NUMBERHERE];
        char timestamp[SOMEDEFINE3NUMBERHERE];
};

or you allocate the memory (malloc)

update.clientname = (char *)malloc(SOMEDEFINE1NUMBERHERE); strncpy(update.clientname, "user");

Upvotes: 0

Eric
Eric

Reputation: 3172

You need to allocate space for writeBuffer.

You could use malloc, or if you have limits on the size of the data (eg if it will be less than 256 bytes) just declare it as 

char writeBuffer[256];

I'd also recommend looking up and using snprintf instead of sprintf to make sure data doesnt run off the end.

Upvotes: 0

Some programmer dude
Some programmer dude

Reputation: 409482

The error is these two lines:

char *writeBuffer=NULL;

/* ... */

sprintf(writeBuffer, /* ... */);

Here you write into a NULL pointer.

Create an array instead, e.g.

char writeBuffer[32];

Upvotes: 0

Kiril Kirov
Kiril Kirov

Reputation: 38193

It crashes, because writeBuffer is NULL.

You need to allocate memory for writeBuffer using malloc - use strlen (for example) to calculate the neccesary space (don't forget \0 and the spaces in the sprintf).

OR, you may allocate some fixed size array automatically:

char writeBuffer[ SOME_SIZE_BIG_ENOUGH ];

Also, you should not do this:

    update.clientName="user";
    update.filename="user";
    update.timestamp="some time";

You have several options:

  • use strcpy
  • use strdup (POSSIX)
  • make the pointers const char*, not just char*.

Upvotes: -1

bchurchill
bchurchill

Reputation: 1420

When 'update' is created it only allocates the space for the pointers of the strings clientName, filename, timestamp; it does not allocate space for the string itself. A quick hack would be to do something like update.clientName = (char*)malloc(sizeof(char)*len) where len is the length of what you want to allocate. Then you should really check the return code of that call...

This is exactly the same issue as with the write buffer... you just need to make sure you allocate space for all your strings.

Upvotes: 0

MByD
MByD

Reputation: 137442

You're trying to write to writeBuffer which is a null pointer, you should declare it as array(1), or allocate a memory on the heap(2) for it.

  1. char writeBuffer[100];
  2. char *writeBuffer=malloc(100)

in both cases you should not use sprintf, but snprintf to make sure you are not overflowing your buffer.

Upvotes: 2

Related Questions