Project

General

Profile

Bug #5111

smb_common_rename uses uninitialized variable

Added by Dan McDonald about 5 years ago. Updated about 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
cifs - CIFS server and client
Start date:
2014-08-21
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage

Description

An OmniOS customer reported some SMB coredumps. The first of which was this:

debugging crash dump vmcore.0 (64-bit) from Olympus
operating system: 5.11 omnios-8c08411 (i86pc)
image uuid: 331b57b0-7aa4-e892-b710-ae1c59c6fe6a
panic message: 
BAD TRAP: type=e (#pf Page fault) rp=ffffff0011a03450 addr=ffffff03197b10c8
dump content: kernel pages only
> $c
smb_fsop_lookup+0x118(ffffff03197b0990, ffffff02dbfca018, 0, ffffff03197b0db8, 
ffffff02da252970, ffffff03197b0ea8)
smb_common_rename+0xd9(ffffff03197b0990, ffffff03197b0bc8, ffffff03197b0db8)
smb_trans2_rename+0x136(ffffff03197b0990, ffffff02e60c9ca0, ffffff02dc90b628, 1
)
smb_set_rename_info+0xb8(ffffff03197b0990, ffffff0011a03840)
smb_set_fileinfo+0xed(ffffff03197b0990, ffffff0011a03840)
smb_set_by_fid+0xb0(ffffff03197b0990, ffffff030f2ff400, 3f2)
smb_com_trans2_set_file_information+0x58(ffffff03197b0990, ffffff030f2ff400)
smb_trans2_dispatch+0x313(ffffff03197b0990, ffffff030f2ff400)
smb_com_transaction2+0x1a7(ffffff03197b0990)
smb_dispatch_request+0x662(ffffff03197b0990)
smb_session_worker+0xa0(ffffff03197b0990)
taskq_d_thread+0xb7(ffffff0331430760)
thread_start+8()

The faulty addr looks like arg3 (4th one) to smb_fsop_lookup(). The calling function calls smb_fsop_lookup in two places. Both use local variable "tnode" for it.

Only one of them, however, has tnode initialized. Here's a subset of the code.

    smb_node_t *tnode;  /* XXX KEBE SAYS UNINITIALIZED */

    .  .  .

    /* Find destination dnode and last_comp */
352    if (dst_fqi->fq_dnode) {
              /* XXX WHAT IF I TAKE THIS PATH... */
353        smb_node_ref(dst_fqi->fq_dnode);
354    } else {
355        tnode = sr->tid_tree->t_snode;
356        rc = smb_pathname_reduce(sr, sr->user_cr, path, tnode, tnode,
357            &dst_fqi->fq_dnode, dst_fqi->fq_last_comp);
358        if (rc != 0) {
359            smb_rename_release_src(sr);
360            return (rc);
361        }
362    }

         .  .  .

        /* AND HIT HERE?!?!? */
375    /* Lookup destination node */
376    rc = smb_fsop_lookup(sr, sr->user_cr, 0, tnode,
377        dst_dnode, new_name, &dst_fqi->fq_fnode);

Consultation with CIFS expert Gordon Ross suggests we move the initialization of "tnode" out of the else-clause, and have it always initialize.

History

#1

Updated by Electric Monk about 5 years ago

  • % Done changed from 80 to 100
  • Status changed from New to Closed

git commit 896cab575ec8a9c845350d470dabb5a0997efebf

commit  896cab575ec8a9c845350d470dabb5a0997efebf
Author: Dan McDonald <danmcd@omniti.com>
Date:   2014-08-21T18:28:27.000Z

    5111 smb_common_rename uses uninitialized variable
    Reviewed by: Richard Lowe <richlowe@richlowe.net>
    Reviewed by: Thomas Keiser <tkeiser@gmail.com>
    Reviewed by: Andy Stormont <AStormont@racktopsystems.com>
    Approved by: Gordon Ross <gwr@nexenta.com>

Also available in: Atom PDF