Why does this code to randomize a list of song names generate a segmentation fault?

Question

I am trying to create a program that receives from the user 12 song titles and then forms a set list in a random order. I have used the gets() function and memory allocation so that the input is placed into an array like this:

 argv[0] = song1, argv[1] = song2, argv[2] = song3 (etc.).

There seems to be the common segmentation fault error when inputting the actual songs and then running it through the randomize and createSetList() functions. However, if one were to scrap the memory allocation and hard code instead it would work fine. By this I mean something like:

char *input[ SONG ] =
  { "song1", "song2", "song3", "song4",
      "song5", "song6", "song7", "song8",
        "song9", "song10", "song11", "song12", "song13" };

Although, my purpose is to have the user input the song titles during run time. What is the reason for a segmentation fault error?

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <string.h>
#define SONG 13
#define SETSIZE 5
// prototypes

void randomize( unsigned int wDeck[ SONG ] ); 
void createSetList( unsigned int wDeck[ SONG ], char *wFace[] ); 

int main( void ){

printf("Please enter the songs you want to play!\n");
printf("I will create the set list and randomize it for you!\n");


char input[100];
char *argv[ SONG ];
char *token;

 /*memory allocation for inputting song titles into a single array*/
 /*****memory allocation code starts here********/
 gets(input);
   token = strtok(input, " ");
     int i=0;
       while( token != NULL ) {
         argv[i] = malloc(strlen(token) + 1);
           strncpy(argv[i], token, strlen(token));
             i++;
      token = strtok(NULL, " ");
    } 
       argv[i] = NULL; //argv ends with NULL
         /***memory allocation code ends here*****/

         unsigned int setList[ SONG ] = { 0 };
           srand( time( NULL ) ); // seed random-number generator
             randomize( setList ); // shuffle the list
                createSetList( setList, argv ); // create the setlist
 } //end of main





/*SHUFFLE FUNCTION*/
   void randomize( unsigned int wDeck[ SONG ] ){

     size_t column;
      size_t c;

 for ( c = 1; c <= SETSIZE; ++c ) {
     do {
       column = rand() % SONG;
         } while( wDeck[ column ] != 0 );

               wDeck[ column ] = c;}}

    /* Create Set List Function */
 void createSetList( unsigned int wDeck[ SONG ],  char *wFace[] ){

    size_t c;
     size_t column;

      for ( c = 1; c <= SETSIZE; ++c ) {

        for ( column = 0; column < SONG; ++column ) {

          if ( wDeck[ column ] == c ) {

             printf( "%s\n", wFace[ column ]); 
                }}}}

Any? Remember that `gets` have no bounds checking and have been a big source of security infractions. It has been deprecated since C99, and in the latest C11 standard have been obsoleted. Don't use it. Use [`fgets`](http://en.cppreference.com/w/c/io/fgets) instead. — Some programmer dude, May 17 '15 at 19:03
Remember that `fgets` includes the final `newline` in the input string. I would add more delimiting chars to the `strtok` calls, as `token = strtok(input, " \r\n");` and ditto the matching one at the end of the loop. — Weather Vane, May 17 '15 at 19:17
Your indentation style is very confusing. Did you invent it or were you taught this way? — chqrlie, May 17 '15 at 19:25
See [Why is the `gets()` function so dangerous that it should never be used?](http://stackoverflow.com/questions/1694036) — Jonathan Leffler, May 17 '15 at 19:32

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

3

In your code,

strncpy(argv[i], token, strlen(token));

usage is wrong. You need to copy the terminating null byte also.

As we can see from the man page of strncpy()

The strncpy() function is similar, except that at most n bytes of src are copied.

Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

As mentioned in the comments my Mr @ chqrlie and explained in the nice article by @Keith Thompson, you should not be using strncpy(). To be on safer side, you can make use of strdup() to avoid the overhead (and pitfalls) of malloc()+ strcpy().

Pseduo-code

 argv[i] = strdup(token);

will do the job in an efficient way. Remember, you need to still free() the returned pointer by strdup().

edited Jun 20 '20 at 09:12

Community

1
1

answered May 17 '15 at 19:07

Sourav Ghosh

133,132
16
183
261

1

Fantastic. This post really helped. – Raoul Duke May 17 '15 at 19:20
3

Don't use `strncpy()`. Your example is a good illustration of why `strncpy()` is error prone. In your case, either use `memcpy(argv[i], token, strlen(token)+1);` or even simpler, use `argv[i] = strdup(token);` – chqrlie May 17 '15 at 19:24
@chqrlie really appreciate leaving the comment. Updated my answer. However, 1) the point was using `strlen()+1`, without which `memcpy()` is __as dangerous__ as `strncpy()`. 2) is that the only reason you marked my answer as _"This answer is not useful"_? – Sourav Ghosh May 17 '15 at 19:31
2

My rant on the subject of `strncpy()`: http://the-flat-trantor-society.blogspot.com/2012/03/no-strncpy-is-not-safer-strcpy.html – Keith Thompson May 17 '15 at 19:46
@Keith Thompson: kudos for this article, I fully agree with you, especially with the conclusion: *I see far more incorrect uses of it than correct uses*. Therefore, I never skip an occasion to warn newbie and experienced programmers about this bear-trap. Other ones on my list include: `strtok()`, `feof()`, `scanf()` and friends, `do/while`... so many incorrect uses of all these! – chqrlie May 17 '15 at 20:36
@chqrlie: Something else I see used incorrectly far more often than correctly is multi-character literals like `'ab'`. They're of type `int` and have an implementation-defined value; cases where they're actually useful are extremely rare. But I often see new C programmers confuse them with string literals. – Keith Thompson May 17 '15 at 20:39
`memcpy` is not as dangerous as `strncpy()`. Read Keith's article and appreciate why `strncpy()` is dangerous and should be avoided. `memcpy` can be used safely here, just as `strcpy(argv[i], token);` would have been. You know the destination is large enough since you just allocated to the correct size. Passing the length of the source string as a third argument to `strncpy()` shows a complete misunderstanding of this function. Adding 1 to that fixes the behaviour, but does not teach the OP anything useful. – chqrlie May 17 '15 at 20:41
@Keith Thompson: you are correct, multi character character constants should have been deprecated a long time ago. Java does not have them, and correctly warns about this in their spec, C++ still has them, beyond all logic. – chqrlie May 17 '15 at 20:46
@Sourav: I down voted your answer because you do not sufficiently deter the OP from using `strncpy()`, even as you give all the relevant information. From his reaction, he might still think `strncpy()` is the right tool, where it is not. Modify your answer to not use `strncpy()`, and I will remove the down vote. – chqrlie May 17 '15 at 20:50
@KeithThompson Thank you for updating me. Nice article, indeed. – Sourav Ghosh May 17 '15 at 21:01
@chqrlie I see the point. Stand corrected. Updated my answer. – Sourav Ghosh May 17 '15 at 21:01

Why does this code to randomize a list of song names generate a segmentation fault?

1 Answers1