strcmp changing the value of a string

Question

void search(char*** p, int numOfWords, int* pNumOfDefArr){
int i, j, index;
char* word = (char*)malloc(WORD_SIZE * sizeof(char));
for (i = 0; i < N; i++) //just clearing the screen
    printf("\n");
printf("Hello and thank you for filling Dictionary 1.0 with new words!!\n");
printf("Which word are you looking for??\n");
gets(word);
fix_word(word, 0);
while (strcmp(word, "Exit")){
    index = (search_word(p, word, 0, numOfWords - 1, 0));
    if (index < 0)
        printf("Unknown word!!!!!!\n");
    else{
        for (j = 0; j < pNumOfDefArr[index]; j++)
            printf("%s\n", *(*(p + index) + 1 + j));
    }
    free(word);
    char* word = (char*)malloc(WORD_SIZE * sizeof(char));
    printf("Looking for another word?\n");
    gets(word);
    fix_word(word, 0);
}
printf("Farewell!!\n");

On the debugger I can see that on the 10th line: while (strcmp(word, "Exit")) the value of word is changing from "asd" to "Error reading characters of string." Why is that?

Here's the code for the fix_word() function:

void fix_word(char* pword, int j){
    if (*(pword + j) != '\0'){
        if (j == 0 && (*(pword + j) >= 'a' && *(pword + j) <= 'z')){
            *pword -= N;
            j++;
        }
        else if (*(pword + j) >= 'A' && *(pword + j) <= 'Z'){
            *(pword + j) += N;
            j++;
        }
        else
            j++;
        fix_word(pword, j);
    }
}

No, `strcmp` isn't modifying your data. You have a bug in your code. Sadly you did not show a complete and cutdown version of the program. Perhaps the bug is elsewhere in the code, the code we cannot see. Perhaps it is here. Who knows? — David Heffernan, Jan 15 '15 at 16:40
It almost certainly means that your `word` is pointing to local memory returned from a function, and `strcmp()` is innocently reusing the stack. Having said that, it isn't self-evident that this is the problem in this code. Returning a pointer to a local (non-static) variable and then using the pointer (e.g. to pass it to `strcmp()`) leads to undefined behaviour. (Also remember that [Three-Star Programmer](http://c2.com/cgi/wiki?ThreeStarProgrammer) is not a compliment.) — Jonathan Leffler, Jan 15 '15 at 16:41
.. Remove the `free` and `malloc` inside the loop (there is no good reason to do that!) to test @Jonathan's theory. — Jongware, Jan 15 '15 at 16:42
Also, never, *never*, **never**, ***never***, ***NEVER*** use `gets()`. See [Why is the `gets()` function dangerous?](http://stackoverflow.com/questions/1694036/why-is-the-gets-function-dangerous-why-should-it-not-be-used) — Jonathan Leffler, Jan 15 '15 at 16:44
Have you printed the contents of `word` after the `gets()` call? After the call to `fix_word()`? — Jonathan Leffler, Jan 15 '15 at 16:45
@JonathanLeffler This is hw and we are forced to use 3star pointers. If 'strcmp()' is not working, then what could I use? — James Giacomo Kintaro Sonnino, Jan 15 '15 at 16:46
@JonathanLeffler I have seen the values on the debugger and they are fine until they get to the 'strcmp' line — James Giacomo Kintaro Sonnino, Jan 15 '15 at 16:48
It isn't `strcmp()` that's at fault — forget about that as an idea; it is simply too fundamental a function for it to be the problem. You should assume that standard library functions are correct until further notice. The problem is in your code. Ideally, you need to provide compilable code — an MCVE ([How to create a Minimal, Complete, and Verifiable Example?](http://stackoverflow.com/help/mcve)) — with the values for N and WORD_SIZE specified. And having the outer test harness would help. Better, remove the banner printing and the screen clearing (they're not part of an MCVE). — Jonathan Leffler, Jan 15 '15 at 16:49
Never cast the return value of `malloc` or any other function returning a `void*` to anything else when you work in the language C. — harper, Jan 15 '15 at 17:58
The logic in `fix_word()` is flawed in that it converts an initial lower case letter to upper case, and an initial upper case letter to lower case (assuming `N == 32` and a code set where `'A' + 32 == 'a'`). It is tail recursive, and tail recursion can be replaced by a loop. Using `*(pword + j)` is a hard way of typing `pword[j]`, there's one place where you use `*pword` but fortunately `j == 0` when that's executed. However, it does not seem to be abusing the memory. I compiled it into a simple test program with `strdup()` and used [`valgrind`](http://valgrind.org/) gave no errors. — Jonathan Leffler, Jan 15 '15 at 17:58
Oh, and it would be better to use the `isupper()`, `islower()`, `toupper()` and `tolower()` functions (usually macros) from `` rather than manually changing case. — Jonathan Leffler, Jan 15 '15 at 18:37
I'm with @JonathanLeffler here... The only way likely way `strcmp()` could be at fault here would be if you wrote your own function called `strcmp()` that is masking the library one. Wait, you didn't do that, did you? — twalberg, Jan 15 '15 at 18:41
being a member of a foreign language I feel discriminated (uppercase ä would be Ä, but you do not convert that) :-) — Peter Miehle, Jan 16 '15 at 08:07

score 6 · Answer 1 · answered Jan 15 '15 at 16:45

6

this is called scope.

char* word = (char*)malloc(WORD_SIZE * sizeof(char));     /* 1 */
while (strcmp(word, "Exit")){                             /* 1 */
    free(word);                                           /* 1 and gone */
    char* word = (char*)malloc(WORD_SIZE * sizeof(char)); /* 2 */
}

you have declared the variable "word" in two different scopes, and you use it intermixed.

If you omit the second "char *", all would be fine.

answered Jan 15 '15 at 16:45

Peter Miehle

5,984
2
38
55

Not just the var; the entire inner-loop free-malloc strategy should be removed. The identical amount of memory is being requested as was initially allocated. Both the inner `free` and the ensuing inner `malloc` should be removed entirely. – WhozCraig Jan 15 '15 at 16:48
1

@JamesGiacomoKintaroSonnino: if you do it correctly, it is a performance problem rather than anything more serious. Allocating memory is slower than not allocating memory. On the other hand, a user typing at the terminal is way, way slower than memory allocation, but not all your programs will be run with a human typing the input. – Jonathan Leffler Jan 15 '15 at 16:52
1

If the compiler is GCC, using `-Wshadow` would highlight such problems automatically. – Jonathan Leffler Jan 15 '15 at 16:55
@JonathanLeffler I'm using VS2013 – James Giacomo Kintaro Sonnino Jan 15 '15 at 16:56
@JonathanLeffler I have omitted '"char*"' but it is still not working – James Giacomo Kintaro Sonnino Jan 15 '15 at 16:58
VS2013 might have an analogous option to `-Wshadow`; I don't know. I've not previously seen this exact pitfall — redeclaring a variable in the tail of a loop. 'Tis interesting! – Jonathan Leffler Jan 15 '15 at 16:59
@JonathanLeffler Yeah that was a silly mistake of mine, I feel kinda tired after 5 hours coding and debugging. But anyway, I have tried as you said but it did not solve the problem... – James Giacomo Kintaro Sonnino Jan 15 '15 at 17:01
1

@JamesGiacomoKintaroSonnino: still not working? Then there's something else also broken. But the something else isn't `strcmp()` — unless you wrote your own version of it, of course. Are you sure you recompiled before retesting? What happens if you don't call `fix_word()`? What happens if you don't call `search_word()` and simply set `index = -1;` instead? – Jonathan Leffler Jan 15 '15 at 17:01
@JonathanLeffler I did recompile before testing. `fiw_word()` just turns the first letter into uppercase if it's lowercase. If I dont call `search_word` and simply set `index = -1` instead it will enter the if `(index < 0)` statement obviously – James Giacomo Kintaro Sonnino Jan 15 '15 at 17:05
@JamesGiacomoKintaroSonnino: yes, I know, but does `word` still get corrupted if you don't call the functions? If it isn't corrupted when one of the functions isn't called, then the finger gets pointed at the function that isn't being called. If `fix_word()` does as you say, then I'd guess that `search_word()` is the trouble, especially if you've not tested it in isolation yet. But without seeing its code, no-one can tell for sure. And if it is the problem, that's a separate question. This one is answered correctly. – Jonathan Leffler Jan 15 '15 at 17:11
1

@JonathanLeffler You were right. Surprisingly the `fix_word()` is making the problem. I have added the code of the `fix_word()` function. I can't figure out what the problem might be. – James Giacomo Kintaro Sonnino Jan 15 '15 at 17:17
Unfortunately this didn't fix the problem. – James Giacomo Kintaro Sonnino Jan 18 '15 at 06:53

strcmp changing the value of a string

1 Answers1