0

Below is the code for a method i made called inputReader it reads input from a textfile and copies it to a struct wordz and from that struct i retrieve the 3 most frequent words which is what is displayed below.

I am trying to concatenate all 3 words as one pointer so i can return it to the main method but whenever i use any method to do with w1, w2, w3, that copies to a new struct or array or pointer i ALWAYS get this error "Segmentation fault(core dumped)"

Any idea why this happens or how I can work around it?

Struct Code:

#define maxLetters 101

typedef struct {
  char word[maxLetters];
  int  freq;
} WordArray; //struct type

Code:

char * w1; // most frequent word 
char * w2; // second most frequent word
char * w3; // third most frequent word

// finds w1
for(j = 0; j < uniqueWords; j++)
  if(wordz[j].freq == freqz[uniqueWords-2]+1)//excludes whitespace frequency
     w1 = wordz[j].word;

// finds w2
for(j = 0; j < uniqueWords; j++)
  if(wordz[j].freq == freqz[uniqueWords-3]+1)//excludes whitespace frequency
     w2 = wordz[j].word;

// finds w3
for(j = 0; j < uniqueWords; j++)
  if(wordz[j].freq == freqz[uniqueWords-4]+1)//excludes whitespace frequency
     w3 = wordz[j].word;

 char *p;

 // if i dont include strcat methods the method runs fine and outputs fine
 strcat(p, w1);  // once this operation is executed i get the error
 strcat(p, " ");
 strcat(p, w2);
 strcat(p, " ");
 strcat(p, w3);
  • there is no guarantee that the max freq words will be at size-2,size-3, size-4. ( also do not see any indication that size-1 is actually the white space freq counter.) suggest keeping tract of which entry in the freq[] array was already extracted when checking for second and third most frequent words and having a if on the offsets for already extracted works so those already selected words are ignored. – user3629249 Jan 17 '15 at 05:14
  • unless this array: freq[] is a sorted array of frequencies, this may not work and probably will not work and will not work when the most frequent word counts are of duplicate values, as it will always select the same word for each entry – user3629249 Jan 17 '15 at 05:17
  • this pointer 'p' needs to be defined with some memory area to receive the words. suggest something like: 'char *p = malloc( 3*(maxLetters)+4) );' Of course, check the returned value from malloc to assure the operation was successful. +4 for the 3 inserted spaces + the final nul char. Note: by convention, #define names are written like this: MAX_LETTERS all caps, words separated by underscores – user3629249 Jan 17 '15 at 05:18

3 Answers3

0

You're trying to concatenate to an uninitialized pointer. Allocate memory to 'p'.

char *p = malloc(size)
Adam Lamers
  • 1,183
  • 7
  • 8
  • I always find it helpful to remind people to use strncat when they show me code with strcat. Its a source of a significant number of security issues in software! – Brian Tompsett - 汤莱恩 Jan 16 '15 at 19:56
  • And also `if (p == 0) { ...error... }` and also `p[0] = '\0';` before trying to use `strcat()` -- the memory returned by `malloc()` is not always zeroed. – Jonathan Leffler Jan 16 '15 at 19:58
  • `strcat()`ing to a pointer which had been initialised via `malloc()` is a bad idea, as the memory referenced had **not** been initialised. **If** doing so the first operation against this pointer shall be `strcpy()`. – alk Jan 16 '15 at 20:07
  • Also, this sort of error is often easily localizable by running under gdb, then doing a bt (backtrace) command. – jamesqf Jan 16 '15 at 20:31
0

It's a good idea to read the documentation of strcat thoroughly to check you are using it properly. You can look here http://man7.org/linux/man-pages/man3/strcat.3.html for example. There are also plenty of answers to similar questions on stackoverflow that you would have found helpful:

However, to save you some anguish, the fault is because to have not allocated any space for the results string p. strcat does not do this in C like it might in C# or other languages.

You would need:

char p[maxletters];

Better still, you should use strncpy with the size limit to prevent memory corruption:

strncpy(p, w1, maxletters);
Community
  • 1
  • 1
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
0

The root cause leading to your issue is that p does not point anywhere, so trying to copy data to it invokes the infamous Undefined Behaviour.

So allocate some memory to p, either from the heap or from the stack.

To get it from the heap do:

char * p = malloc(some_size);

When then trying to strcat() data to this pointer be aware that strcat() expects its 1st argument to already point to a valid C-"string", that is a 0-teminated char array, to that it can concatenate what the 2nd argument is pointing to.

To take care this, make sure the data p is pointing to is does at least has its first bytes set to 0:

p[0] = '\0';

if not all bytes:

memset(p, 0, some_size);

If going for the latter you might also like to do:

char * p = calloc(some_size, sizeof *p);

As calloc() does the same as malloc() and addtionally initilaises all memory allocated with 0s.

If you feel initalising is to much effort then you cannot start concatenating using strcat(), but nered to start with strcpy() which does not rely on its 1st argument pointing to a valid C-"string", but simply copies what its 2nd argument is pointing to to where its 1st argement is pointing to.

chart * p = malloc(some_size);
strcpy(p, w1);  
strcat(p, " ");
...

Finally:

  • Do not forget to call free() on p if the memory isn't needed anymore, to avoids a memory leak.

  • Also always check the outcome of memory allocation, that is check the result of malloc/calloc() against NULL, before using it.

alk
  • 69,737
  • 10
  • 105
  • 255