Bug 210798 - devel/gdb: Uninitialized variables found presenting possible security issues or bugs
Summary: devel/gdb: Uninitialized variables found presenting possible security issues ...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Tijl Coosemans
URL:
Keywords: needs-patch, needs-qa
Depends on: 214927
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-03 19:46 UTC by Mark Millard
Modified: 2016-12-18 16:10 UTC (History)
1 user (show)

See Also:
luca.pizzamiglio: maintainer-feedback+


Attachments
Fixing some warning (3.65 KB, patch)
2016-07-11 13:37 UTC, luca.pizzamiglio
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Millard 2016-07-03 19:46:05 UTC
-r417989 context:

compile/compile-loc2c.c's do_compile_dwarf_expr_to_c (. . .) has (note uoffset's initialization status):

  while (op_ptr < op_end)
    {
      enum dwarf_location_atom op = (enum dwarf_location_atom) *op_ptr;
      uint64_t uoffset, reg;
      int64_t offset;
. . . no assignments to uoffset . . .
      switch (op)
. . .
        case DW_OP_addr:
. . .
          if (op_ptr >= op_end || *op_ptr != DW_OP_GNU_push_tls_address)
            uoffset += dwarf2_per_cu_text_offset (per_cu);
          push (indent, stream, uoffset);
          break;

Note the "uoffset +=".

This was reported by the compiler during the build.
Comment 1 Mark Millard 2016-07-03 20:06:06 UTC
(In reply to Mark Millard from comment #0)

Another compiler-reported uninitialized value use follows.

/usr/obj/portswork/usr/ports/devel/gdb/work/gdb-7.11.1/gdb/rs6000-tdep.c (note ra initialzxiation status):

static int
ppc_process_record_op31 (struct gdbarch *gdbarch, struct regcache *regcache,
                           CORE_ADDR addr, uint32_t insn)
{
. . .
  ULONGEST rb, ra, xer;
. . .
  switch (ext & 0x1ff)
    {
. . .
    case 1014:          /* Data Cache Block set to Zero */
. . .
      if (PPC_RA (insn) != 0)
        regcache_raw_read_unsigned (regcache,
                                    tdep->ppc_gp0_regnum + PPC_RA (insn), &ra);
. . .
      ea = (ra + rb) & ~((ULONGEST) (at_dcsz - 1));
      record_full_arch_list_add_mem (ea, at_dcsz);
. . .

That last "ra" is always used in  case 1014 but was not potentially initialized unless the shown regcache_raw_read_unsigned call was made.
Comment 2 Mark Millard 2016-07-03 20:32:50 UTC
(In reply to Mark Millard from comment #0)

Another compiler-reported uninitialized value use follows. This one has some potential to have a complicated invariant that sidesteps the potential issue. If it does then there is the requirement that certain things be in a specific order so that fcn_aux_saved is ready for use for cs->c_sclass == C_FCN.

gdb/xcoffread.c's read_xcoff_symtab( . . .) has a variable that is effectively uninitialized (fcn_aux_saved) by being initialized from another at-the-time uninitialized variable (main_aux):

static void
read_xcoff_symtab (struct objfile *objfile, struct partial_symtab *pst)
{
. . .
  union internal_auxent main_aux;
. . . main_aux not initialized here . . .
  union internal_auxent fcn_aux_saved = main_aux;
. . .
  while (symnum < max_symnum)
    {
. . .
      if ((cs->c_sclass == C_EXT || cs->c_sclass == C_HIDEXT)
          && cs->c_naux == 1)
        {
. . .
          bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
                                0, cs->c_naux, &main_aux);

          switch (CSECT_SMTYP (&main_aux))
            {
. . . (The below is the only potential initialization of fcn_aux_saved) . . .
            case XTY_LD:

              switch (CSECT_SCLAS (&main_aux))
                {
                case XMC_PR:
                  /* a function entry point.  */
                function_entry_point:

                  fcn_start_addr = cs->c_value;
      
                  /* save the function header info, which will be used
                     when `.bf' is seen.  */
                  fcn_cs_saved = *cs;
                  fcn_aux_saved = main_aux;
                  continue;
. . .
      switch (cs->c_sclass)
        {
. . .
        case C_FCN:
          if (strcmp (cs->c_name, ".bf") == 0)
. . .
          else if (strcmp (cs->c_name, ".ef") == 0)
            {
              bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
                                    0, cs->c_naux, &main_aux);
. . . (The  below is the only use of fcn_aux_saved) . . .
              finish_block (newobj->name, &local_symbols, newobj->old_blocks,
                            NULL, newobj->start_addr,
                            (fcn_cs_saved.c_value
                             + fcn_aux_saved.x_sym.x_misc.x_fsize
                             + ANOFFSET (objfile->section_offsets,
                                         SECT_OFF_TEXT (objfile))));
              within_function = 0;
            }
          break;
. . .
    }

[The last "}" is for the while above.]
Comment 3 Mark Millard 2016-07-03 21:05:09 UTC
(In reply to Mark Millard from comment #0)

I probably should have been explicit that for the do_compile_dwarf_expr_to_c code as it is:

push (indent, stream, uoffset);

always involves uoffset's uninitialized value.

If the "+=" were just "=" then the lack of a else assigning a value to uoffset would still have a path in which uoffset involved an uninitialized value in the push call.

But it may be that an initialization of uoffset to something like 0 may take care of things without an explicit else: the above wording is not trying to point out the specific fix to make.
Comment 4 VK freebsd_triage 2016-07-05 08:24:06 UTC
Summarizing the issue, let's keep it simple, and elaborate in the comments. ;)
Comment 5 luca.pizzamiglio 2016-07-08 13:26:32 UTC
Hi Mark,

thanks to report those problems.
Some comments:

gdb/rs6000-tdep.c: is already fixed in the upstream, so I can merge it back soon

gdb/compile/compile-loc2c.c: initializing uoffset to zero would silent the warning, but I'm not sure if it's correct; I guess that the correct behavior is granted by well formed dwarf expressions.

gdb/xcoffread.c: a solution here is hard to find; the uninitialized path should happen only with malformed xcoff, so read_xcoff_symtab() run without issues with well formed xcoff.

Soon I'll provide a patch to fix the first two warnings; the last one require a feedback from gdb's people.
Comment 6 luca.pizzamiglio 2016-07-11 13:37:05 UTC
Created attachment 172379 [details]
Fixing some warning

The goal of this patch is to fix/silent as much as warning as possible, without affecting the stability of gdb.
The final goal is to compile warning-free.

This patch is still a draft, but it's already usable and testable.

readxcoff.c untouched, a fix is at the moment to hard to implement.
compile-loc2.c and rs6000-tdep.c warnings are fixed.
Unused function warning is disabled.
Macro redefinition in fsbd-kvm.c (kgdb) fixed
Some tautological checks are removed.

@Mark, please, give me a feedback!
Comment 7 Mark Millard 2016-07-12 08:38:19 UTC
[Other things have taken my time but here are the results of one test of the patch: amd64 context.]

I applied your patch on an amd64 11.0 -r302457 context with /usr/ports at -r4178253 and rebuilt gdb, generating a typescript file via script.

grep'ing that typescript file for "warning:" shows:

# grep warning: ~/ports_typescripts/gdb_patch_typescript
checking for memmem... inflate.c:1507:61: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
./simple-object-xcoff.c:330:12: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:332:39: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:335:29: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:342:12: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:344:39: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:347:29: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:475:32: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:477:30: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:482:32: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:484:30: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:598:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:600:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:603:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:608:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:610:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:613:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:663:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:665:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:670:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:672:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:683:19: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:685:19: warning: using extended field designator is an extension [-Wextended-offsetof]
./stack-limit.c:54:24: warning: comparison of integers of different signs: 'rlim_t' (aka 'long') and 'unsigned long' [-Wsign-compare]
checking whether logf is declared without a macro... rl78-dis.c:232:44: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
microblaze-tdep.c:94:28: warning: format string is not a string literal [-Wformat-nonliteral]
msp430-tdep.c:401:24: warning: comparison of constant 16 with expression of type 'MSP430_Size' is always false [-Wtautological-constant-out-of-range-compare]
score-tdep.c:819:12: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
sh64-tdep.c:977:42: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
v850-tdep.c:562:35: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
v850-tdep.c:562:66: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
remote-mips.c:500:37: warning: format string is not a string literal [-Wformat-nonliteral]
xcoffread.c:1027:41: warning: variable 'main_aux' is uninitialized when used here [-Wuninitialized]
./nat/x86-dregs.c:209:7: warning: variable 'i' is incremented both in the loop header and in the loop body [-Wfor-loop-analysis]
./tui/tui-stack.c:419:16: warning: expression which evaluates to zero treated as a null pointer constant of type 'CORE_ADDR *' (aka 'unsigned long *') [-Wnon-literal-null-conversion]
dtrace-probe.c:424:52: warning: while loop has empty body [-Wempty-body]
main.c:229:56: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
ada-lang.c:2489:50: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
ada-lang.c:2503:46: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
/usr/bin/ld: warning: libncurses.so.8, needed by /usr/local/lib/libreadline.so, may conflict with libncurses.so.6
/usr/bin/ld: warning: libncurses.so.8, needed by /usr/local/lib/libreadline.so, may conflict with libncurses.so.6

(libncurses.so.* issues are tied to /usr/lib and /lib vs. /usr/local/lib bindings.)

Most of that is C source code being reported on, which is the focus here. I list them all simply because you indicated you are starting to attempt to eliminate many types of compiler reports.

Certainly the various things that I reported earlier are not listed in the above: now gone, at least for amd64.

As for the make.conf in use:

# more /etc/make.conf
WANT_QT_VERBOSE_CONFIGURE=1
#
DEFAULT_VERSIONS+=perl5=5.22
WRKDIRPREFIX=/usr/obj/portswork
WITH_DEBUG=
WITH_DEBUG_FILES=
MALLOC_PRODUCTION=

An armv6 build might be somewhat different but I've not done that test yet. I will.

[I will not have access to powerpc64 or powerpc again for weeks or months yet. Currently I'm limited to amd64 and armv6 examples. So only little endian.]
Comment 8 Mark Millard 2016-07-12 09:59:58 UTC
(In reply to luca.pizzamiglio from comment #6)

Beyond the amd64 test info I listed in comment 5: building on/for armv6 (with -mcpu=cortex-a7 in use) reports things suggesting some 32-bit vs. 64-bit issues may exist when int/long are 32-bit but long long is 64: labs use that may truncate values. [powerpc (non-64) likely would get the same.] There are a couple of other types of differences as well.

[I tried to eliminate the escape sequences form the messages listed below.]

inflate.c:1507:61: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
checking stddef.h usability... ./simple-object-xcoff.c:330:12: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:332:39: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:335:29: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:342:12: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:344:39: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:347:29: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:475:32: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:477:30: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:482:32: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:484:30: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:598:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:600:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:603:21: warning: using extended field designator is an extension [-Wextended-offsetof
./simple-object-xcoff.c:608:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:610:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:613:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:663:21: warning: using extended field designator is an extension [-Wextended-offsetof
./simple-object-xcoff.c:665:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:670:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:672:21: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:683:19: warning: using extended field designator is an extension [-Wextended-offsetof]
./simple-object-xcoff.c:685:19: warning: using extended field designator is an extension [-Wextended-offsetof]
elf32-arm.c:10901:31: warning: absolute value function 'labs' given an argument of type 'bfd_signed_vma' (aka 'long long') but has parameter of type 'long'
elf32-arm.c:10986:31: warning: absolute value function 'labs' given an argument of type 'bfd_signed_vma' (aka 'long long') but has parameter of type 'long'
elf32-arm.c:11073:30: warning: absolute value function 'labs' given an argument of type 'bfd_signed_vma' (aka 'long long') but has parameter of type 'long'
checking whether wcsspn is declared without a macro... rl78-dis.c:232:44: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
microblaze-tdep.c:94:28: warning: format string is not a string literal [-Wformat-nonliteral]
msp430-tdep.c:401:24: warning: comparison of constant 16 with expression of type 'MSP430_Size' is always false [-Wtautological-constant-out-of-range-compare]
score-tdep.c:819:12: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
sh64-tdep.c:977:42: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
v850-tdep.c:562:35: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
v850-tdep.c:562:66: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
remote-mips.c:500:37: warning: format string is not a string literal [-Wformat-nonliteral]
xcoffread.c:1027:41: warning: variable 'main_aux' is uninitialized when used here [-Wuninitialized]
./tui/tui-stack.c:419:16: warning: expression which evaluates to zero treated as a null pointer constant of type 'CORE_ADDR * ' (aka 'unsigned long long *')
dtrace-probe.c:424:52: warning: while loop has empty body [-Wempty-body]
main.c:229:56: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
ada-lang.c:2489:50: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
ada-lang.c:2503:46: warning: shifting a negative signed value is undefined [-Wshift-negative-value]
Comment 9 Mark Millard 2016-07-13 01:28:08 UTC
(In reply to luca.pizzamiglio from comment #6)

By the way: It will be weeks or months before I again have access to the powerpc's that I use (32-bit or 64-bit).

This means I do not currently have my context for checking the specific compiles for TARGET_ARCH=powerpc or TARGET_ARCH=powerpc64 .
Comment 10 Mark Millard 2016-08-02 09:11:54 UTC
The original build was of gdb-7.11.1_1 --which is from /usr/ports/ -r417443 .

There have been updates since. But at least the xcoffread.c part of the uninitialized variable warnings still applies as of gdb-7.11.1_3 (which is still in place as of -r419343).