segmentation fault while calling strlen with a previously allocated pointer

Question

Up until now I have had some code that worked perfectly regarding a variable called wfiles. wfiles is initialized within my main file:

char* wfiles = "";

Which as far as I can tell C has no complaints. Next the wfiles variable is allocated in a switch statement:

 switch (c) {
    case 't':
        /* the user wants a template */
        template = optarg;
        break;
    case 'f':
        wfiles = optarg;
        break;
    case 'v':
        vcs = optarg;
        break;
    case 'u':
        url = optarg;
        break;
    case 's':
        /* custom save location */
        save_loc = optarg;
        break;
    case '?':
        break;
    default:
        abort();
    }

finally I check for whether or not wfiles is empty:

if (!empty(wfiles))

empty is a macro that expands to (strlen(wfiles) == 0)

I cannot see any problems with this but when I run this code I get a segmentation fault. This had never happened before. When I ran the code in gdb with and without debugging symbols, I get one line pointed to the if statement earlier mentioned. Does anyone know why this is?

The problem is somewhere else. The code you've shown seems to have no problems. — Spikatrix, Jun 01 '15 at 07:05
Where do you get `optarg` from? Since the only assignment to `wfiles` is from `optarg`, it is very likely that the memory it pointed to when it was assigned is not valid anymore when you are calling `strlen` later — Andreas Fester, Jun 01 '15 at 07:09
@intricatedetail Simply output the string and its address and all will be clear.:) — Vlad from Moscow, Jun 01 '15 at 07:09
optarg is from getopt.h and I cannot output the string, it immediately segfaults regardless — intricatedetail, Jun 01 '15 at 07:13
However commenting out the option parsing while loop solves the problem... I suspect I may have parsed the options wrong — intricatedetail, Jun 01 '15 at 07:21
`char* wfiles = "";` is poor style; string literals are not modifiable. `const char *wfiles = "";` should be used instead. The original version is grandfathered in from pre-ANSI C. — M.M, Jun 01 '15 at 07:59
"Next the wfiles variable is allocated" - actually it is *assigned* here. You make the already-existing pointer `wfiles` point to a different location . No memory is allocated by `wfiles = optarg;`; `optarg` must already exist. Another potential problem might be if the memory pointed to by `optarg` is released but you then still try to use `wfiles`. — M.M, Jun 01 '15 at 08:00

Sourav Ghosh · Answer 1 · 2015-06-01T07:52:08.873

EDIT:

As mentioned in the other answers, truly you need to check for NULL before passing a pointer to strlen() . In your case, optarg may very well contain NULL. So, better put a NULL check against the incoming pointer in empty() MACRO.

^{However, the below point is still valid, so keeping it as it is.}

I think, your issue maybe with the way you assign optarg to wfiles. optarg may change on different iterations. So, only pointing to optarg may give you faultly result. You need to copy the content of optarg for that particular iteration.

Instead of assigning (storing) the pointer, try

allocating memory to wfiles.
strcpy() the optarg to wfiles.

Or, you can make use of strdup() directly.

score 2 · Accepted Answer · edited May 23 '17 at 11:43

2

strlen() will dereference a pointer, which may cause some problems when this pointer,i.e optarg, is not valid.

For example, in case of optarg=NULL and c='f', your code will be executed as:

case 'f':
    wfiles = optarg;
    //Now wfiles=NULL

strlen(wfiles) will be: strlen(NULL) and segmentation fault happens.

See this related question for more reference

edited May 23 '17 at 11:43

Community

1
1

answered Jun 01 '15 at 07:22

blindpirate

114
1
5

Thanks I never knew about this, I tweaked my macro and it worked fine. – intricatedetail Jun 01 '15 at 07:33

score 2 · Answer 3 · answered Jun 01 '15 at 07:23

2

You might want to secure the macro empty by 1^st testing whether its argument points to NULL before calling strlen().

#define empty(s) (s) ?(0 == strlen(s)) :1

answered Jun 01 '15 at 07:23

alk

69,737
10
105
255

1

Or safer, faster & more readable: `inline bool is_empty (const char* str) { if(str == NULL){ return true; } return *str == '\0'; }`. But of course if the rest of the program was bug-free, the pointer would never be NULL anyhow. – Lundin Jun 01 '15 at 08:16

segmentation fault while calling strlen with a previously allocated pointer

3 Answers3