8121 Dead RPC code can free unallocated memory

Review Request #450 — Created April 28, 2017 and submitted

marcel
illumos-gate
master
8121
e757831...
general
This removes some dead and buggy code from libnsl(3nsl) and kernel RPC
implementation.
I let the machine with this fix to run for a while and tested some basic NFS
functionality (both client and server) to make sure everything works as
expected.
  • 0
  • 0
  • 0
  • 3
  • 3
Description From Last Updated
marcel
marcel
jbk
  1. 
      
  2. usr/src/lib/libnsl/rpc/svc_auth_loopb.c (Diff revision 1)
     
     

    Does anything else ensure the buffer has at least 8 byes prior to doing these reads?

    1. This is another bug.  It will be fixed separately.  The work is already in progress.
  3. usr/src/lib/libnsl/rpc/svc_auth_sys.c (Diff revision 1)
     
     

    Similar question as above.

    1. This is another bug.  It will be fixed separately.  The work is already in progress.
  4. 
      
tsoome
  1. 
      
  2. usr/src/lib/libnsl/rpc/svc_auth_loopb.c (Diff revision 3)
     
     

    str_len and gid_len is set by IXDR_GET_U_INT32() which does type cast to uint32_t, in that light, wouldn't it be better to use uint32_t for the type?

    1. You are right, but...
      
      Historically, the 4-byte unsigned integers in the XDR (see Section 4.2 in RFC 4506) are usually typed as uint_t (or even u_int) in the sources, even they are specified explicitly as 32-bit in the XDR RFC.  This is probably because the exact-wide types like uint32_t are far newer than our XDR sources.  This is unfortunate, and strictly speaking not correct and we should probably revise all of the XDR code and use the exact-wide types where appropriate.  But this is definitely out of the scope of this change.
      
      Here, I just wanted to align types in __svcauth_loopback() with the similar code in __svcauth_sys().  If I would use uint32_t here then it might open a can of worms if somebody would ask: If you used uint32_t here, then why not to use uint32_t there and there and there and... too?
    2. Yea, makes sense. And the assumption about 32bit int is in so many places that if int != 32bits, most of the world will blow up anyhow:D

  3. 
      
tsoome
  1. Ship It!
  2. 
      
danmcd
  1. 
      
  2. usr/src/lib/libnsl/rpc/svc_auth_loopb.c (Diff revision 3)
     
     

    Looks like you and Toomas worked this out already. Was this code copy/pasted from __svcauth_sys() ?

    1. It was aligned with it, so you can say so.

  3. 
      
danmcd
  1. Ship It!
  2. 
      
marcel
Review request changed

Status: Closed (submitted)

Change Summary:

commit 5806135a39e4841f6f6b7281eaf59f34fffec72e
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Fri Apr 28 17:02:13 2017 +0200
Commit:     Hans Rosenfeld <hans.rosenfeld@joyent.com>
CommitDate: Wed May 10 17:02:32 2017 +0200

    8121 Dead RPC code can free unallocated memory
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Dan McDonald <danmcd@omniti.com>
    Approved by: Hans Rosenfeld <hans.rosenfeld@joyent.com>

:100644 100644 b1c4c06... 02f0e2c... M	usr/src/lib/libnsl/rpc/authsys_prot.c
:100644 100644 dbc6240... 49e226a... M	usr/src/lib/libnsl/rpc/svc_auth_loopb.c
:100644 100644 87e5462... 322b974... M	usr/src/lib/libnsl/rpc/svc_auth_sys.c
:100644 100644 df18090... 5f163d2... M	usr/src/uts/common/rpc/auth_sys.h
:100644 100644 224a8d3... df459e0... M	usr/src/uts/common/rpc/sec/svc_authu.c
Loading...