locked
the program crashes?? RRS feed

  • Question

  • Hi All,

    this is my program code:

    #include <stdio.h>#include <stdlib.h>#include <stdbool.h>void getvals(char** *vals, int valscount, const char* input){    *vals = (char**)calloc(sizeof(char**), valscount);    int y;    for (y = 0; y<valscount; y++) (*vals)[y] = (char*)calloc(sizeof(char*), 20);    int pos = -1;    char* chunk;    char s[100];    char r[100];    char chunk2[20];    int i = valscount - 1;    int jpos = 0;    if (!input) return 0;    strcpy(s, input);    while (true){        chunk = calloc(sizeof(char), 100);        pos = strrchr(s, '/') - (int)(void*)s;        if (pos<0) break;        strncpy(chunk, s, pos);        strcpy(s, chunk);        free(chunk);        strcpy(r, strrev(s));        jpos = strchr(r, '/') - (int)(void*)r;        strncpy(chunk2, r, jpos);        strcpy(r, strrev(chunk2));        strcpy((*vals)[i], r);        i--;    }}int main(){    char** vals;    getvals(&vals, 3, "0/1/2");    getch();}

    when doing the debug, the program crashes,

     so what's wrong and how to fix the problem?

    thanks.


    • Edited by Yuri.B Wednesday, March 26, 2014 4:50 PM
    Wednesday, March 26, 2014 4:42 PM

Answers

  • On 3/26/2014 12:42 PM, Yuri.B wrote:

             strncpy(chunk2, r, jpos);
             strcpy(r, strrev(chunk2));

    strncpy does not NUL-terminate the destination string. strrev expects its input to be NUL-terminated. Hence, buffer overrun.

    Generally, the code is just horrendous. As far as I can tell, all it's trying to do is split the input string into tokens separated by slashes - but it does so in an awfully convoluted way. Why all these strrev() calls? A straightforward loop calling strchr or strtok would be way easier.


    Igor Tandetnik
    • Marked as answer by Yuri.B Wednesday, March 26, 2014 6:06 PM
    Wednesday, March 26, 2014 5:14 PM
  • On 3/26/2014 1:33 PM, Yuri.B wrote:

    can you suggest me a better code, so?

    See the example in strtok docs: http://msdn.microsoft.com/en-us/library/2c8d19sb.aspx

    Adapting it to your needs is left as an exercise for the reader.


    Igor Tandetnik
    • Marked as answer by Yuri.B Wednesday, March 26, 2014 10:12 PM
    Wednesday, March 26, 2014 7:13 PM

All replies

  • On 3/26/2014 12:42 PM, Yuri.B wrote:

             strncpy(chunk2, r, jpos);
             strcpy(r, strrev(chunk2));

    strncpy does not NUL-terminate the destination string. strrev expects its input to be NUL-terminated. Hence, buffer overrun.

    Generally, the code is just horrendous. As far as I can tell, all it's trying to do is split the input string into tokens separated by slashes - but it does so in an awfully convoluted way. Why all these strrev() calls? A straightforward loop calling strchr or strtok would be way easier.


    Igor Tandetnik
    • Marked as answer by Yuri.B Wednesday, March 26, 2014 6:06 PM
    Wednesday, March 26, 2014 5:14 PM
  • On 3/26/2014 12:42 PM, Yuri.B wrote:

             strncpy(chunk2, r, jpos);
             strcpy(r, strrev(chunk2));

    strncpy does not NUL-terminate the destination string. strrev expects its input to be NUL-terminated. Hence, buffer overrun.

    Generally, the code is just horrendous. As far as I can tell, all it's trying to do is split the input string into tokens separated by slashes - but it does so in an awfully convoluted way. Why all these strrev() calls? A straightforward loop calling strchr or strtok would be way easier.


    Igor Tandetnik

    OK, I will try that, thanks anyway.


    • Edited by Yuri.B Wednesday, March 26, 2014 6:05 PM
    Wednesday, March 26, 2014 5:33 PM
  • Since the program does not compile properly, how did you get it to link and why did you even try to execute it?

    What version of VS are you running?  VS 2010 Express does not have a stdbool.h.

    You are missing the include for string.h so there are no prototypes for all the string functions you use (strcpy, etc).

    getvals() is a void function yet you try to return 0 from it.

    You are coding in C but compiling as C++.  You are missing the mandatory cast on your call to calloc in the while loop.  It would be better if you stuck to one language instead of mixing features of both.

    This line of code requires a diagnostic:
        pos = strrchr(s, '/') - (int)(void*)s;
    pos is an int.   strrchr returns a char*.  Subtracting an int from a pointer results in pointer value.  There is no implicit conversion from pointer to int.  If you delete the two unnecessary casts, the statement should do what I think you intend.  The same is true for
       jpos = strchr(r, '/') - (int)(void*)r;

    However, both of the above will fail when the '/' is not found.  The functions return NULL in that case and the subtraction causes undefined behavior.

    You need to include the appropriate header for the call to getch in main.

    Wednesday, March 26, 2014 6:59 PM
  • On 3/26/2014 2:59 PM, Barry-Schwarz wrote:

    You are coding in C but compiling as C++.

    I suspect the OP is coding in C and compiling as C, and so the thing actually compiles.


    Igor Tandetnik
    Wednesday, March 26, 2014 7:12 PM
  • On 3/26/2014 1:33 PM, Yuri.B wrote:

    can you suggest me a better code, so?

    See the example in strtok docs: http://msdn.microsoft.com/en-us/library/2c8d19sb.aspx

    Adapting it to your needs is left as an exercise for the reader.


    Igor Tandetnik
    • Marked as answer by Yuri.B Wednesday, March 26, 2014 10:12 PM
    Wednesday, March 26, 2014 7:13 PM
  • On 3/26/2014 2:59 PM, Barry-Schwarz wrote:

    You are coding in C but compiling as C++.

    I suspect the OP is coding in C and compiling as C, and so the thing actually compiles.


    Igor Tandetnik

    He intersperses declarations within his executable statements which is a no-no in C and should result in a diagnostic.

    He casts the return from some calls to calloc but not others which led me to an assumption.

    Since he has no prototype for strchr and strrchr, a C compiler would assume they return int which explains why his cast sequence (int)(void*) might pass the syntax check but still invokes undefined behavior and usually results in a compiler diagnostic about missing prototypes.

    stdbool.h still does not exist which should cause a diagnostic.

    Compile yes, cleanly no.

    Thursday, March 27, 2014 5:48 AM
  • "He intersperses declarations within his executable statements which is a no-no in C and should result in a diagnostic."

    That's valid and has been so since '99. It's only VC++ that didn't know about this until recently (2013). Same for stdbool.h.

    Thursday, March 27, 2014 6:50 AM
  • Which is why I asked him which VS version he was running
    Friday, March 28, 2014 7:15 AM