Yes, that's possible too.  But in that case if xdr_bytes() above failed and didn't allocate anything (so objp->buf would be NULL) we would overwrite the maxlen too, but we do not need to do so.  Such verbatim (not overwritten) maxlen value could be of some use for some debugging, or something (even such usage would be a bit strange, rare, and definitely a corner case).
OTOH, the xdr_bytes() function (not this one, but the one from libnsl) is a part of API and it should not (read: cannot) leak the passed in buffer.  If it does so we would have a lot of different and serious problems anyway.
In any case, if you really wants to see && buf_save == NULL there I have no problem to change it.  Just let me know.

No, I just suggested it to ease the reading for someone who isn't really familar with this code, but as we would touch maxlen when we shouldn't, please ignore it.

+1 for using "buf_save == NULL"

I'm not sure I fully understand the problem you are describing here.  But if you are trying to say that in the XDR_FREE case the xdr_u_int() function call in the xdr_netbuf() function can change the value of objp->maxlen then this assumption is wrong.  The value of objp->maxlen can change only in the XDR_DECODE case.  It is one of the basic concepts of the XDR library.  Without this assumption many things in the XDR implementation would not work.  This concept is also documented in the ONC+ Developer's Guide.
OTOH, we cannot remove the xdr_u_int() call for objp->maxlen as you suggests.  The maxlen field is needed in the XDR stream.  See RFC 1833 for details.  Yes, I would prefer to do not have the maxlen there, but the RFC requires it.  And yes, rpcbind is the main consumer of xdr_netbuf.

I meant that if we consider xdr_u_int() as blackbox (i.e. you don't know internals), following by xdr_bytes(&maxlen) looks suspicious. It is as general view of design.


@maxlen field in xdr stream is excessive because @opaque<> has length. Why do we need two lengths in xdr stream?. All things are in libnsl or in rpcmod module so @maxlen might be slightly dropped.


Anyway, if we want to keep @maxlen in xdr stream, then code can be the following and it is fully conformed for RFC1833 :
xdr_netbuf(XDR *xdrs, netbuf *objp)

{
/* optimized free */

  if (x_op == XDR_FREE) {

      return xdr_bytes(..., &objp->maxlen, objp->maxlen);

  } else { 

      uint_t dummy = ~0;
  if (!xdr_u_int(xdrs, &dummy)) {
     return FALSE;     
  }

  if (!xdr_bytes(.., &objp->buf, &objp->len, ~0)) {
      return FALSE;
  }

 /\* follow the netbuf semantics \*/
 if (x_op == XDR_DECODE && objp->maxlen == 0)
    objp->maxlen = objp->len;

}
return TRUE;

}

I meant that if we consider xdr_u_int() as blackbox (i.e. you don't know internals), following by xdr_bytes(&maxlen) looks suspicious. It is as general view of design.


If you consider every function as a blackbox (i.e. you don't know internals) then following any function with anything else looks susplicious.  The good thing is that you usually knows the interface of the blackbox and the behavior of the blackbox.  So you are safe to assume something.  That's also the case here.




@maxlen field in xdr stream is excessive because @opaque<> has length. Why do we need two lengths in xdr stream?. All things are in libnsl or in rpcmod module so @maxlen might be slightly dropped.


Because RFC 1833 requires maxlen.  Did you heard that there are some other systems in the wild implementing RFC 1833 and we would like to interoperate with them?



Anyway, if we want to keep @maxlen in xdr stream, then code can be the following and it is fully conformed for RFC1833 :

Yes, that's possible.  But I prefer to call xdr_u_int() in the XDR_FREE case for maxlen too, even it is a no-op.  It better follows the XDR usage and design patterns.  We do not need to optimize the XDR_FREE case for speed.  How do I know that?  The previous code had the same call there and nobody complained about the speed.

I suggested to use separate path for XDR_FREE for clarity, not for speed. 


BTW, There is a little vulnerable code for case that you described in the issue #8180:
r_net_state() in usr/src/uts/common/avs/ns/rdc/rdc_svc.c:

{
...
free_rdc_netbuf(&(state.netaddr));
free_rdc_netbuf(&(state.rnetaddr));

}
Here is freeing memory area with size "nbuf->maxlen" whereas area size is "nbuf->len".


Also don't forget to fix rdc_xdr_netbuf() :)

Also don't forget to fix rdc_xdr_netbuf() :)

Nice catch.  Thanks.  Done.  It is interesting how some people makes things more complicated than it is needed. :-)

Could you use buf_save != NULL ? Or something buf_prep != NULL.

No.  That would cause the maxlen overwrite in a case the buffer was preallocated.  That's something that we definitely do not want.

Sorry, I wanted to say  "&& buf_save == NULL" .

See above why I do not prefer buf_save == NULL and why I believe that objp->buf != buf_save is better.

Let's to copypaste here:
<Yes, that's possible too.  But in that case if xdr_bytes() above failed and didn't allocate anything (so objp->buf would be NULL)

<we would overwrite the maxlen too, but we do not need to do so.  Such verbatim (not overwritten) maxlen value could be of some

<use for some debugging, or something (even such usage would be a bit strange, rare, and definitely a corner case).
To protect maxlen in your case that you described it could be rewritten like:
if (!xdr_bytes(xdrs, &objp->buf, &objp->len, objp->maxlen))

     return FALSE;
if (x_op == XDR_DECODE && buf_saved == NULL)

     objp->maxlen = objp->len;
return TRUE;

Comparison "objp->buf != buf_saved" does not clearly say that "buffer was allocated before".

Yes, that would protect maxlen in the scenario I descrbed, but it would create a new problem in this scenario: buf was NULL, xdr_bytes() allocated it, but failed to decode the bytes.  Now the maxlen will have the value decoded from the XDR stream, len will be the allocated length and buf will point to the allocated buffer.  This is wrong.