|
From: David H. (D. Design) <dt...@dt...> - 2013-02-11 06:23:36
|
I am new to C and Valgrind, so I can't understand why I am getting an error
message on what seems like a pretty standard if statement.
Basically I want to strip the newline character, '\n', from the value fgets
returns to me. Pretty much everywhere I could find said the recommended way
to do this was something like this: (getInput is a function I have written
where fgets resides and it does some validation)
char *getInputNoNewline(int max, char message[]) { static char* input; int
maxchars = max+ADDCHARS input = (char*)malloc(maxchars); input =
getInput(max, message); if (input[strlen(input) - 1] == '\n') {
input[strlen(input) - 1] = '\0'; } return input; }
The if statement is giving me 57 errors from 6 contexts.
Running in verbose mode a typical error is:
==18533== 3 errors in context 3 of 18:
==18533== Invalid read of size 1
==18533== at 0x100000BF0: getInputNoNewline (start.c:44)
==18533== by 0x100000D2A: main (start.c:61)
==18533== Address 0x10027f434 is 4 bytes inside a block of size 6 free'd
==18533== at 0x10000FE9F: free (vg_replace_malloc.c:366)
==18533== by 0x100000B88: getInput (start.c:35)
==18533== by 0x100000BCD: getInputNoNewline (start.c:43)
==18533== by 0x100000D2A: main (start.c:61)
The statement replacing '\n' with '\o' is giving me 75 errors from 12
contexts.
Typical error message:
==18533== 18 errors in context 17 of 18:
==18533== Invalid read of size 1
==18533== at 0x100011503: strlen (mc_replace_strmem.c:282)
==18533== by 0x100000C0D: getInputNoNewline (start.c:45)
==18533== by 0x100000CF8: main (start.c:59)
==18533== Address 0x10027f361 is 1 bytes inside a block of size 50 free'd
==18533== at 0x10000FE9F: free (vg_replace_malloc.c:366)
==18533== by 0x100000B88: getInput (start.c:35)
==18533== by 0x100000BCD: getInputNoNewline (start.c:43)
==18533== by 0x100000CF8: main (start.c:59)
Are these errors that valgrind is giving me likely to cause any errors down
the track as programs become more complex?
|
|
From: Brian B. <bri...@gm...> - 2013-02-11 06:55:12
|
On Sun, Feb 10, 2013 at 9:55 PM, David Hourn (DTH Design)
<dt...@dt...> wrote:
> I am new to C and Valgrind, so I can't understand why I am getting an error
> message on what seems like a pretty standard if statement.
>
> Basically I want to strip the newline character, '\n', from the value fgets
> returns to me. Pretty much everywhere I could find said the recommended way
> to do this was something like this: (getInput is a function I have written
> where fgets resides and it does some validation)
>
> char *getInputNoNewline(int max, char message[]) { static char* input; int
> maxchars = max+ADDCHARS input = (char*)malloc(maxchars); input =
> getInput(max, message); if (input[strlen(input) - 1] == '\n') {
> input[strlen(input) - 1] = '\0'; } return input; }
Hi David -
First you assign a malloced address to input, and then on the
following line, you assign to input from getInput. At the very least,
you should be leaking memory there. My guess (only a guess, since we
don't have the code for getInput), is that getInput does not provide a
valid null-terminated string for strlen, which is why there are
invalid reads when calling strlen.
Brian
|
|
From: David C. <dcc...@ac...> - 2013-02-11 07:34:36
|
On 2/10/2013 9:55 PM, David Hourn (DTH Design) wrote:
> I am new to C and Valgrind, so I can't understand why I am getting an
> error message on what seems like a pretty standard if statement.
>
> Basically I want to strip the newline character, '\n', from the value
> fgets returns to me. Pretty much everywhere I could find said the
> recommended way to do this was something like this: (getInput is a
> function I have written where fgets resides and it does some validation)
>
> char *getInputNoNewline(int max, char message[]) { static char* input;
> int maxchars = max+ADDCHARS input = (char*)malloc(maxchars); input =
> getInput(max, message); if (input[strlen(input) - 1] == '\n') {
> input[strlen(input) - 1] = '\0'; } return input; }
We prefer that you submit a full listing of the failing program if you
can; otherwise your discussion is vulnerable to cut and paste errors.
For example, you are missing a semicolon after the maxchars declaration
in the snippet above (a syntax error). There could be many more
differences between what you've sent and what you are actually running.
I see a possible memory leak, which may also be due to cut and paste -
every time you call getInputNoNewline(), you are allocating a new buffer
and storing a pointer to it in a static variable. Then you overwrite
that pointer with the return value from getInput(), losing track of the
memory just allocated. Usually static variables are used for objects
that will have a long lifetime; you allocate it once (perhaps
reallocating it when it needs to grow) and then pass around a pointer to
use. The pointer will usually not be overwritten each time.
> The if statement is giving me 57 errors from 6 contexts.
>
> Running in verbose mode a typical error is:
>
> ==18533== 3 errors in context 3 of 18:
> ==18533== Invalid read of size 1
> ==18533== at 0x100000BF0: getInputNoNewline (start.c:44)
> ==18533== by 0x100000D2A: main (start.c:61)
> ==18533== Address 0x10027f434 is 4 bytes inside a block of size 6 free'd
> ==18533== at 0x10000FE9F: free (vg_replace_malloc.c:366)
> ==18533== by 0x100000B88: getInput (start.c:35)
> ==18533== by 0x100000BCD: getInputNoNewline (start.c:43)
> ==18533== by 0x100000D2A: main (start.c:61)
The call stack ("context") here says that getInput() is releasing the
memory being read by getInputNoNewline(). main() is calling
getInputNoNewline() which is calling getInput() which is calling free()
in file start.c, line 35. Given the question about a possible memory
leak above, it sounds like you are not reading from/writing into the
buffer that getInputNoNewline() is creating; you are writing into
something else that is freed before getInputNoNewline() regains control.
>
> The statement replacing '\n' with '\o' is giving me 75 errors from 12
> contexts.
>
> Typical error message:
>
> ==18533== 18 errors in context 17 of 18:
> ==18533== Invalid read of size 1
> ==18533== at 0x100011503: strlen (mc_replace_strmem.c:282)
> ==18533== by 0x100000C0D: getInputNoNewline (start.c:45)
> ==18533== by 0x100000CF8: main (start.c:59)
> ==18533== Address 0x10027f361 is 1 bytes inside a block of size 50 free'd
> ==18533== at 0x10000FE9F: free (vg_replace_malloc.c:366)
> ==18533== by 0x100000B88: getInput (start.c:35)
> ==18533== by 0x100000BCD: getInputNoNewline (start.c:43)
> ==18533== by 0x100000CF8: main (start.c:59)
>
> Are these errors that valgrind is giving me likely to cause any errors
> down the track as programs become more complex?
>
The call stack is the same; most likely this is the exact same problem.
You have a pointer to freed memory and you are dereferencing it - first
a read and then a write.
Very definitely you are going to have problems with this code.
--
David Chapman dcc...@ac...
Chapman Consulting -- San Jose, CA
Software Development Done Right.
www.chapman-consulting-sj.com
|
|
From: Eliot M. <mo...@cs...> - 2013-02-11 14:24:20
|
On 2/11/2013 12:55 AM, David Hourn (DTH Design) wrote:
> char *getInputNoNewline(int max, char message[]) { static char* input; int maxchars = max+ADDCHARS
> input = (char*)malloc(maxchars); input = getInput(max, message); if (input[strlen(input) - 1] ==
> '\n') { input[strlen(input) - 1] = '\0'; } return input; }
Just looking at this code, I see these issues:
- You allocate space for input then overwrite the pointer to that space,
as other correspondents have noted.
- You call strlen more than once. Wasteful, though not *harmful*.
- You are counting on getInput to return a string that has at least
one character in it; if it has no characters then you will access
input[-1], which is out of range (and you might potentially damage
some other data structure, such as a header that malloc uses to
track items and to handle freeing).
If you expect no more than one \n in the string, then strchr may be
what you want, or strrchr to find the last \n regardless. If they
return non-null, you can examine the next byte to see if it is \0
and then clear the \n safely.
Regards -- Eliot Moss
|
|
From: David H. (D. Design) <dt...@dt...> - 2013-02-11 23:04:50
|
Cheers guys, After reviewing the code the general issues I was having was memory management, and trying to access memory that had been freed. Having only started coding in C two days ago it's probably a fair common problem. This is the full code that I am using (I should've done this yesterday. Didn't think to pastebin) http://pastebin.com/tewJSX3W Compiling and running that code through valgrind gives me no errors which is great, however there is still 12 non-freed blocks at the end. 9 definitely lost - Am I right in saying these are getInput.in (allocated line 25), getInputNoNewline.input (allocated line 39) and main.IntIn(initialised line 51). And there are 3 still reachable blocks, which i'm guessing are the three blocks that make up the players array (allocated line 53) ? Dave On Tue, Feb 12, 2013 at 1:24 AM, Eliot Moss <mo...@cs...> wrote: > On 2/11/2013 12:55 AM, David Hourn (DTH Design) wrote: > > char *getInputNoNewline(int max, char message[]) { static char* input; >> int maxchars = max+ADDCHARS >> input = (char*)malloc(maxchars); input = getInput(max, message); if >> (input[strlen(input) - 1] == >> '\n') { input[strlen(input) - 1] = '\0'; } return input; } >> > > Just looking at this code, I see these issues: > > - You allocate space for input then overwrite the pointer to that space, > as other correspondents have noted. > - You call strlen more than once. Wasteful, though not *harmful*. > - You are counting on getInput to return a string that has at least > one character in it; if it has no characters then you will access > input[-1], which is out of range (and you might potentially damage > some other data structure, such as a header that malloc uses to > track items and to handle freeing). > > If you expect no more than one \n in the string, then strchr may be > what you want, or strrchr to find the last \n regardless. If they > return non-null, you can examine the next byte to see if it is \0 > and then clear the \n safely. > > Regards -- Eliot Moss > |
|
From: Eliot M. <mo...@cs...> - 2013-02-12 02:56:59
|
On 2/11/2013 6:04 PM, David Hourn (DTH Design) wrote: > Cheers guys, > > After reviewing the code the general issues I was having was memory management, and trying to access > memory that had been freed. Having only started coding in C two days ago it's probably a fair common > problem. > > This is the full code that I am using (I should've done this yesterday. Didn't think to pastebin) > http://pastebin.com/tewJSX3W > > Compiling and running that code through valgrind gives me no errors which is great, however there is > still 12 non-freed blocks at the end. 9 definitely lost - Am I right in saying these are getInput.in > (allocated line 25), getInputNoNewline.input (allocated line 39) and main.IntIn(initialised line > 51). And there are 3 still reachable blocks, which i'm guessing are the three blocks that make up > the players array (allocated line 53) ? Well, you make 9 input calls, each of which allocates a buffer, and you allocate the players array (it is one malloc object, even though it is 3 structs). You free the players array, but none of the strings it points to. The 3 IntIn assignments are blocks just dropped. In getInput, input and in point to the same place, so should not be a problem. This code will not necessarily work right if there is an I/O error or fgets reads NO chars at all. Likewise, getInputNoNewline may make an improper access: you should check len > 0 *before* accessing in[len - 1]. The still-reachable blocks mystify me a little. The only one I see is the last IntIn, since players is freed and the rest won't be reachable from any accessible variable. Of course there are block in library routines, such as an I/O buffer for stdout, etc. Regards -- Eliot Moss |