[PATCH] Fix uninitialized use of variables.

[PATCH] Fix uninitialized use of variables.

[Carlos O'Donell]

Compiling gdb head with gcc head reveals two uninitialized uses of
variables. Analysis of the uninitialized uses reveals that gcc is
correct. The following patch fixes both uninitiailized uses of
variables.

In remote.c (unpack_nibble) the variable *val is not set if ishex fails
to find hex digits. Callers of unpack_nibble expect *val to be set. The
solution is to check the return of ishex, and call error appropriately.
In the case that *val is not set, the uninitialized use is never reached
because we call error.

In symtab.c (find_line_common) the variable *exact_match is not set if
no match is found. Callers of find_line_common expect *exact_match to be
set. The solution is to initialize *exact_match to zero, assuming an
inexact match. In the case that we don't find a match in
find_line_common, the statement `(best_index < 0 || !exact)' in
symtab.c:2267 is true, instead of undefined. The comment is adjusted to
indicate that one must look at another symtab if we failed to find a
match `best_index < 0' or we found an inexact match `!exact.'

No regressions on i686-pc-linux-gnu. 

OK to checkin?

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716

gdb/

2007-10-18  Carlos O'Donell  <carlos@codesourcery.com>

	* remote.c (unpack_nibble): error if buffer is not valid hex.
	* symtab.c (find_line_symtab): `no match' case is important.
	(find_line_common): Default *exact_match = 0 before search.

Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.271
diff -u -p -r1.271 remote.c
--- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
+++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
@@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
 static char *
 unpack_nibble (char *buf, int *val)
 {
-  ishex (*buf++, val);
+  if (!ishex (*buf++, val))
+    error (_("Unpacked nibble does not contain hex characters."));
   return buf;
 }
 
Index: gdb/symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.166
diff -u -p -r1.166 symtab.c
--- gdb/symtab.c	9 Oct 2007 06:59:27 -0000	1.166
+++ gdb/symtab.c	18 Oct 2007 16:34:05 -0000
@@ -2267,10 +2267,10 @@ find_line_symtab (struct symtab *symtab,
   best_index = find_line_common (best_linetable, line, &exact);
   if (best_index < 0 || !exact)
     {
-      /* Didn't find an exact match.  So we better keep looking for
-         another symtab with the same name.  In the case of xcoff,
-         multiple csects for one source file (produced by IBM's FORTRAN
-         compiler) produce multiple symtabs (this is unavoidable
+      /* Didn't find an exact match, or any match.  So we better keep 
+         looking for another symtab with the same name.  In the case of 
+         xcoff, multiple csects for one source file (produced by IBM's 
+         FORTRAN compiler) produce multiple symtabs (this is unavoidable
          assuming csects can be at arbitrary places in memory and that
          the GLOBAL_BLOCK of a symtab has a begin and end address).  */
 
@@ -2411,6 +2411,9 @@ find_line_common (struct linetable *l, i
   int best_index = -1;
   int best = 0;
 
+  /* Assume an inexact match.  */
+  *exact_match = 0;
+
   if (lineno <= 0)
     return -1;
   if (l == 0)
@@ -2436,8 +2439,6 @@ find_line_common (struct linetable *l, i
     }
 
   /* If we got here, we didn't get an exact match.  */
-
-  *exact_match = 0;
   return best_index;
 }
 

Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy]

Carlos O'Donell <carlos at codesourcery.com> writes:
> In symtab.c (find_line_common) the variable *exact_match is not set if
> no match is found. Callers of find_line_common expect *exact_match to be
> set. The solution is to initialize *exact_match to zero, assuming an
> inexact match. In the case that we don't find a match in
> find_line_common, the statement `(best_index < 0 || !exact)' in
> symtab.c:2267 is true, instead of undefined. The comment is adjusted to
> indicate that one must look at another symtab if we failed to find a
> match `best_index < 0' or we found an inexact match `!exact.'

As far as the symtab.c change is concerned: the specification of
find_line_common is that *EXACT_MATCH is set only if the function
returns a match (a non-negative value).  As far as I can see,
find_line_symtab doesn't actually use the value of 'exact' unless the
corresponding call to find_line_common returned a match.  So the
warning looks spurious to me.

(I don't mind initializing 'exact' in find_line_symtab to placate the
compiler.)

Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy]

Carlos O'Donell <carlos at codesourcery.com> writes:
> Index: gdb/remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.271
> diff -u -p -r1.271 remote.c
> --- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
> +++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
> @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
>  static char *
>  unpack_nibble (char *buf, int *val)
>  {
> -  ishex (*buf++, val);
> +  if (!ishex (*buf++, val))
> +    error (_("Unpacked nibble does not contain hex characters."));
>    return buf;
>  }

This looks fine to me, although Daniel has thoughts on error handling
in the remote protocol that I don't fully understand.

But the error message is going to be obscure to users.  It should at
least say something about the remote protocol packet being misformed.

Re: [PATCH] Fix uninitialized use of variables.

[Daniel Jacobowitz]

On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote:
> This looks fine to me, although Daniel has thoughts on error handling
> in the remote protocol that I don't fully understand.

Nothing recent or complicated.  I just want it to become more
consistent rather than less...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy]

Daniel Jacobowitz <dan at codesourcery.com> writes:
> On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote:
>> This looks fine to me, although Daniel has thoughts on error handling
>> in the remote protocol that I don't fully understand.
>
> Nothing recent or complicated.  I just want it to become more
> consistent rather than less...

Here's what I was thinking of:
http://sourceware.org/ml/gdb-patches/2007-07/msg00071.html
    From: Daniel Jacobowitz
    I would like for us to unify the error handling in the remote target
    along with this.  It's very jumbled as to what's a warning and what
    isn't.

I think I also remember you expressing concern about throwing 'error'
during the connection process.

Can you fill us in more on how you'd like remote.c to work?  Maybe we
can get started, and even get those textual error messages in.

Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy]

Jim Blandy <jimb at codesourcery.com> writes:
> Carlos O'Donell <carlos at codesourcery.com> writes:
>> Index: gdb/remote.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/remote.c,v
>> retrieving revision 1.271
>> diff -u -p -r1.271 remote.c
>> --- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
>> +++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
>> @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
>>  static char *
>>  unpack_nibble (char *buf, int *val)
>>  {
>> -  ishex (*buf++, val);
>> +  if (!ishex (*buf++, val))
>> +    error (_("Unpacked nibble does not contain hex characters."));
>>    return buf;
>>  }
>
> This looks fine to me, although Daniel has thoughts on error handling
> in the remote protocol that I don't fully understand.
>
> But the error message is going to be obscure to users.  It should at
> least say something about the remote protocol packet being misformed.

You know, I can't see any harm in this change, whatever the remote.c
big picture is.  If you can make the error message something more
helpful, please commit.

Re: [PATCH] Fix uninitialized use of variables.

[Carlos O'Donell]

On Tue, Oct 23, 2007 at 04:17:29PM -0700, Jim Blandy wrote:
> 
> Carlos O'Donell <carlos at codesourcery.com> writes:
> > In symtab.c (find_line_common) the variable *exact_match is not set if
> > no match is found. Callers of find_line_common expect *exact_match to be
> > set. The solution is to initialize *exact_match to zero, assuming an
> > inexact match. In the case that we don't find a match in
> > find_line_common, the statement `(best_index < 0 || !exact)' in
> > symtab.c:2267 is true, instead of undefined. The comment is adjusted to
> > indicate that one must look at another symtab if we failed to find a
> > match `best_index < 0' or we found an inexact match `!exact.'
> 
> As far as the symtab.c change is concerned: the specification of
> find_line_common is that *EXACT_MATCH is set only if the function
> returns a match (a non-negative value).  As far as I can see,
> find_line_symtab doesn't actually use the value of 'exact' unless the
> corresponding call to find_line_common returned a match.  So the
> warning looks spurious to me.
> 
> (I don't mind initializing 'exact' in find_line_symtab to placate the
> compiler.)

Sorry, yes, the boolean short circuit avoids the evaluation of !exact.
The compiler probably can't see that though and warns that it *may* be
an uninitialized use of the variable.

I'll whip up a patch that initializes exact in find_line_symtab.

Thanks for the review.

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716

Re: [PATCH] Fix uninitialized use of variables.

[Daniel Jacobowitz]

On Wed, Oct 24, 2007 at 09:51:43AM -0700, Jim Blandy wrote:
> Can you fill us in more on how you'd like remote.c to work?  Maybe we
> can get started, and even get those textual error messages in.

I don't have any bigger picture than I've already said (and I don't
want to pick up a new crusade just now; I am trying to finish the ones
I already have!)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Fix uninitialized use of variables.

[Carlos O'Donell]

On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote:
> 
> Carlos O'Donell <carlos at codesourcery.com> writes:
> > Index: gdb/remote.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/remote.c,v
> > retrieving revision 1.271
> > diff -u -p -r1.271 remote.c
> > --- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
> > +++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
> > @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
> >  static char *
> >  unpack_nibble (char *buf, int *val)
> >  {
> > -  ishex (*buf++, val);
> > +  if (!ishex (*buf++, val))
> > +    error (_("Unpacked nibble does not contain hex characters."));
> >    return buf;
> >  }
> 
> This looks fine to me, although Daniel has thoughts on error handling
> in the remote protocol that I don't fully understand.
> 
> But the error message is going to be obscure to users.  It should at
> least say something about the remote protocol packet being misformed.

In all truth I would have rather returned a status and let the caller
determine the error message.

Should the return of the function indicate status or number of nibbles
parsed as ishex does?

e.g.

static int
unpack_nibble (char **buf, int *val)
{
  int cnt;
  (*buf)++;
  cnt = ishex (**buf, val);
  return cnt;
}

...
  
  cnt = unpack_nibble (&pkt, &done);
  if (cnt != 1)
    error (_("Packet error: Unable to parse `done' from thread list response.!"));

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716

Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy]

Carlos O'Donell <carlos at codesourcery.com> writes:
> On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote:
>> 
>> Carlos O'Donell <carlos at codesourcery.com> writes:
>> > Index: gdb/remote.c
>> > ===================================================================
>> > RCS file: /cvs/src/src/gdb/remote.c,v
>> > retrieving revision 1.271
>> > diff -u -p -r1.271 remote.c
>> > --- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
>> > +++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
>> > @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
>> >  static char *
>> >  unpack_nibble (char *buf, int *val)
>> >  {
>> > -  ishex (*buf++, val);
>> > +  if (!ishex (*buf++, val))
>> > +    error (_("Unpacked nibble does not contain hex characters."));
>> >    return buf;
>> >  }
>> 
>> This looks fine to me, although Daniel has thoughts on error handling
>> in the remote protocol that I don't fully understand.
>> 
>> But the error message is going to be obscure to users.  It should at
>> least say something about the remote protocol packet being misformed.
>
> In all truth I would have rather returned a status and let the caller
> determine the error message.

I can see the appeal, but that sounds like a lot of work:

- Change all the 'unpack_' functions to take the text pointer by
  reference, update it in place, and return a status.
- Change all their callers to pass the text pointer by reference,
  check the error code, and return an error status.

And the win would be that you can say specifically what part of the
packet didn't parse, instead of just saying that the packet didn't
parse.  It would be cleaner overall, but speaking for myself, there
are other things I'd appreciate more.  :)

The callers of the 'unpack_' functions currently seem to assume that
the callees will raise errors as needed; it doesn't seem too bad to go
along with that.

Re: [PATCH] Fix uninitialized use of variables.

[Carlos O'Donell]

On Wed, Oct 24, 2007 at 11:20:44AM -0700, Jim Blandy wrote:
> > In all truth I would have rather returned a status and let the caller
> > determine the error message.
> 
> I can see the appeal, but that sounds like a lot of work:
> 
> - Change all the 'unpack_' functions to take the text pointer by
>   reference, update it in place, and return a status.
> - Change all their callers to pass the text pointer by reference,
>   check the error code, and return an error status.
> 
> And the win would be that you can say specifically what part of the
> packet didn't parse, instead of just saying that the packet didn't
> parse.  It would be cleaner overall, but speaking for myself, there
> are other things I'd appreciate more.  :)

Agreed.
 
> The callers of the 'unpack_' functions currently seem to assume that
> the callees will raise errors as needed; it doesn't seem too bad to go
> along with that.

Yes, that sounds good. Let's fix a real bug, and prevent a future
compiler warning.

OK to checkin?

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716

gdb/

2007-10-26  Carlos O'Donell  <carlos@codesourcery.com>

	* remote.c (unpack_nibble): error if buffer is not valid hex.
	* symtab.c (find_line_symtab): Initialize exact. Adjust
	comment to mention `no match' case.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.271
diff -u -p -r1.271 remote.c
--- remote.c	8 Oct 2007 12:55:09 -0000	1.271
+++ remote.c	26 Oct 2007 15:38:13 -0000
@@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
 static char *
 unpack_nibble (char *buf, int *val)
 {
-  ishex (*buf++, val);
+  if (!ishex (*buf++, val))
+    error (_("Packet contains invalid hex data."));
   return buf;
 }
 
Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.167
diff -u -p -r1.167 symtab.c
--- symtab.c	24 Oct 2007 13:25:16 -0000	1.167
+++ symtab.c	26 Oct 2007 15:38:13 -0000
@@ -2252,7 +2252,7 @@ find_pc_line (CORE_ADDR pc, int notcurre
 struct symtab *
 find_line_symtab (struct symtab *symtab, int line, int *index, int *exact_match)
 {
-  int exact;
+  int exact = 0;
 
   /* BEST_INDEX and BEST_LINETABLE identify the smallest linenumber > LINE
      so far seen.  */
@@ -2267,10 +2267,10 @@ find_line_symtab (struct symtab *symtab,
   best_index = find_line_common (best_linetable, line, &exact);
   if (best_index < 0 || !exact)
     {
-      /* Didn't find an exact match.  So we better keep looking for
-         another symtab with the same name.  In the case of xcoff,
-         multiple csects for one source file (produced by IBM's FORTRAN
-         compiler) produce multiple symtabs (this is unavoidable
+      /* Didn't find an exact match, or any match.  So we better keep 
+         looking for another symtab with the same name.  In the case of 
+         xcoff, multiple csects for one source file (produced by IBM's 
+         FORTRAN compiler) produce multiple symtabs (this is unavoidable
          assuming csects can be at arbitrary places in memory and that
          the GLOBAL_BLOCK of a symtab has a begin and end address).  */