|
From: Robert C. <r.c...@gs...> - 2007-10-18 09:34:58
|
Hi, I have a function that contains the three lines below, snprintf takes only arguments involving fname and filename, the VALGRIND macros used below return no output i.e. I don't get: Uninitialised byte(s) found during client check request yet I still get the output on the Conditional jump or move shown below. What I don't understand is how this can be the case unless it refers to something internal to snprintf, as if fname and filename are not uninitialised, what causes the error? Is there something I have misunderstood? Many thanks, Rob. 1842 VALGRIND_CHECK_VALUE_IS_DEFINED(filename); 1843 VALGRIND_CHECK_VALUE_IS_DEFINED(fname); 1844 snprintf(filename, strlen(fname)+1, "%s.states", fname); ==28714== ==28714== Conditional jump or move depends on uninitialised value(s) ==28714== at 0x81C25CA: strlen (in /the/cussons/fmd/program/fmd.svn/src/continuumstates) ==28714== by 0x81B9177: vsnprintf (in /the/cussons/fmd/program/fmd.svn/src/continuumstates) ==28714== by 0x81B48F8: snprintf (in /the/cussons/fmd/program/fmd.svn/src/continuumstates) ==28714== by 0x8058DCE: writeMulticonfigfile (Projection.c:1844) ==28714== by 0x804A388: main (continuumstates.c:480) |
|
From: Tom H. <to...@co...> - 2007-10-18 09:39:34
|
In message <471...@gs...>
Robert Cussons <r.c...@gs...> wrote:
> I have a function that contains the three lines below, snprintf takes
> only arguments involving fname and filename, the VALGRIND macros used
> below return no output i.e. I don't get:
> Uninitialised byte(s) found during client check request
> yet I still get the output on the Conditional jump or move shown below.
> What I don't understand is how this can be the case unless it refers to
> something internal to snprintf, as if fname and filename are not
> uninitialised, what causes the error? Is there something I have
> misunderstood?
At a guess either filename or fname is a pointer to char rather
than an array of char, in which case the CHECK_DEFINED macro will
only check that the pointer is defined, and not that what it points
to is defined.
Basically CHECK_DEFINED(n) checks that sizeof(n) bytes at &x are
defined - sizeof(n) is either 4 or 8 for a pointer and &x is the
address of the pointer.
> 1842 VALGRIND_CHECK_VALUE_IS_DEFINED(filename);
> 1843 VALGRIND_CHECK_VALUE_IS_DEFINED(fname);
You probably want to do this:
VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename))
VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname))
Of course you may get some warnings from those strlen calls while
they try and work out how long your string is. It should tell you
which one is bogus though.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Ashley P. <api...@co...> - 2007-10-18 10:15:00
|
On Thu, 2007-10-18 at 10:39 +0100, Tom Hughes wrote:
> You probably want to do this:
>
> VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename))
> VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname))
Actually what you really want is this:
{
int len = strlen(fname);
VALGRIND_CHECK_VALUE_IS_DEFINED(filename);
VALGRIND_CHECK_VALUE_IS_DEFINED(fname);
VALGRIND_CHECK_MEM_IS_DEFINED(fname, len+1)
VALGRIND_CHECK_MEM_IS_DEFINED(filename, len+8)
}
Leaving in the VALGRIND_CHECK_VALUE_IS_DEFINED() calls is perfectly
reasonable, they will simply test for a different type of error.
Of course you have at least one bug in your code wrt the len paramater
to snprintf(), if you simply pass in the strlen() of the string you are
generating you might as well just use sprintf(), you should pass in the
allocated length of filename here, the intention is to prevent buffer
overruns after all. Your second bug is that the computed value that you
do pass in will truncate the result of filename to be exactly that of
fname, if you really want to append .states you should change the +1 to
be +8.
$ cat bob.c
int main () {
char *fname = "file";
char *filename = malloc(100);
snprintf(filename, strlen(fname)+1, "%s.states", fname);
printf("Result is \"%s\"\n",filename);
return 0;
}
$ cc bob.c
$ ./a.out
Result is "file"
Ashley,
|
|
From: Ashley P. <api...@co...> - 2007-10-18 10:08:48
|
On Thu, 2007-10-18 at 10:54 +0100, Ashley Pittman wrote:
> On Thu, 2007-10-18 at 10:39 +0100, Tom Hughes wrote:
> > You probably want to do this:
> >
> > VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename))
> > VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname))
>
> Actually what you really want is this:
>
> {
> int len = strlen(fname);
> VALGRIND_CHECK_VALUE_IS_DEFINED(filename);
> VALGRIND_CHECK_VALUE_IS_DEFINED(fname);
> VALGRIND_CHECK_MEM_IS_DEFINED(fname, len+1)
> VALGRIND_CHECK_MEM_IS_DEFINED(filename, len+8)
> }
Correcting myself here but the last one should be ...ADDRESSABLE, you'll
notice I've put the +8 in here rather than +1 as I rather expect that's
what you meant your code to do...
{
int len = strlen(fname);
VALGRIND_CHECK_VALUE_IS_DEFINED(filename);
VALGRIND_CHECK_VALUE_IS_DEFINED(fname);
VALGRIND_CHECK_MEM_IS_DEFINED(fname, len+1)
VALGRIND_CHECK_MEM_IS_ADDRESSABLE(filename, len+8)
}
Ashley,
|
|
From: Christoph B. <bar...@or...> - 2007-10-18 09:47:42
|
Am Donnerstag, 18. Oktober 2007 schrieb Robert Cussons: > 1842 VALGRIND_CHECK_VALUE_IS_DEFINED(filename); > 1843 VALGRIND_CHECK_VALUE_IS_DEFINED(fname); > 1844 snprintf(filename, strlen(fname)+1, "%s.states", fname); You are only checking the pointers and not the values pointed at. I guess that checking "filename" is not meaningful because it is only used as a buffer that is written to. Christoph |
|
From: Robert C. <r.c...@gs...> - 2007-10-18 09:49:06
|
Tom Hughes wrote: > In message <471...@gs...> > Robert Cussons <r.c...@gs...> wrote: > >> I have a function that contains the three lines below, snprintf takes >> only arguments involving fname and filename, the VALGRIND macros used >> below return no output i.e. I don't get: >> Uninitialised byte(s) found during client check request >> yet I still get the output on the Conditional jump or move shown below. >> What I don't understand is how this can be the case unless it refers to >> something internal to snprintf, as if fname and filename are not >> uninitialised, what causes the error? Is there something I have >> misunderstood? > > At a guess either filename or fname is a pointer to char rather > than an array of char, in which case the CHECK_DEFINED macro will > only check that the pointer is defined, and not that what it points > to is defined. > > Basically CHECK_DEFINED(n) checks that sizeof(n) bytes at &x are > defined - sizeof(n) is either 4 or 8 for a pointer and &x is the > address of the pointer. > >> 1842 VALGRIND_CHECK_VALUE_IS_DEFINED(filename); >> 1843 VALGRIND_CHECK_VALUE_IS_DEFINED(fname); > > You probably want to do this: > > VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename)) > VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname)) > > Of course you may get some warnings from those strlen calls while > they try and work out how long your string is. It should tell you > which one is bogus though. > > Tom > Absolutely right, thanks to Christoph as well for the same answer. Just found that the definition of these macros is given in memcheck.h, so will try to find answer there first next time, sorry. Cheers, Rob. -- ================================ Robert Cussons Office SB3 3.163, Theory Group Gesellschaft für Schwerionenforschung mbH (GSI) Planckstraße 1 64291 Darmstadt Germany. Tel: +49 (0)6159 71 2754 ================================ |
|
From: Robert C. <r.c...@gs...> - 2007-10-18 10:12:03
|
Robert Cussons wrote: > Tom Hughes wrote: >> In message <471...@gs...> >> Robert Cussons <r.c...@gs...> wrote: >> >>> I have a function that contains the three lines below, snprintf takes >>> only arguments involving fname and filename, the VALGRIND macros used >>> below return no output i.e. I don't get: >>> Uninitialised byte(s) found during client check request >>> yet I still get the output on the Conditional jump or move shown below. >>> What I don't understand is how this can be the case unless it refers to >>> something internal to snprintf, as if fname and filename are not >>> uninitialised, what causes the error? Is there something I have >>> misunderstood? >> At a guess either filename or fname is a pointer to char rather >> than an array of char, in which case the CHECK_DEFINED macro will >> only check that the pointer is defined, and not that what it points >> to is defined. >> >> Basically CHECK_DEFINED(n) checks that sizeof(n) bytes at &x are >> defined - sizeof(n) is either 4 or 8 for a pointer and &x is the >> address of the pointer. >> >>> 1842 VALGRIND_CHECK_VALUE_IS_DEFINED(filename); >>> 1843 VALGRIND_CHECK_VALUE_IS_DEFINED(fname); >> You probably want to do this: >> >> VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename)) >> VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname)) >> >> Of course you may get some warnings from those strlen calls while >> they try and work out how long your string is. It should tell you >> which one is bogus though. >> >> Tom >> > > Absolutely right, thanks to Christoph as well for the same answer. Just > found that the definition of these macros is given in memcheck.h, so > will try to find answer there first next time, sorry. > > Cheers, > Rob. > Thanks to your advice I tried VALGRIND_CHECK_MEM_IS_DEFINED(filename, strlen(filename)) VALGRIND_CHECK_MEM_IS_DEFINED(fname, strlen(fname)) what you didn't know is that fname was declared as: char fname[255] and then had a string read into it that was shorter than this, so the command above using strlen(fname) only check up to the string terminator, maybe that's why Julian suggested using fixed numbers. Anyway, if I use 255 instead, it then picks up that some of the characters haven't been initialised and gives an uninitialised error. I think I will just use asprintf() to read the string into fname, then the string will be the correct size with no 'wasted space' and I don't need to worry! Thanks for the help, Rob. |
|
From: Christoph B. <bar...@or...> - 2007-10-18 10:21:51
|
Am Donnerstag, 18. Oktober 2007 schrieb Robert Cussons: > Robert Cussons wrote: ... > what you didn't know is that fname was declared as: > > char fname[255] > > and then had a string read into it that was shorter than this, so the > command above using strlen(fname) only check up to the string > terminator, maybe that's why Julian suggested using fixed numbers. > Anyway, if I use 255 instead, it then picks up that some of the > characters haven't been initialised and gives an uninitialised error. You do not need to check the whole fname array. Only checking the filled characters is ok. The snprintf method should not access the unfilled ones because they are behind the termination character. Christoph |