8098 Some xdr_simple(3nsl) and xdr_complex(3nsl) functions can succeed for undefined xdrs->x_op values

Reviews
Diff

- Download Diff

Information
Submitter:	Marcel Telka
Repository:	illumos-gate
Branch:	master
Bugs:	8098
Depends On:
Commit:	eaab405...
Reviewers
Groups:	general
People:

Description

This modifies the following XDR functions from libnsl(3nsl) to fail in a case
an invalid value of xdrs->x_op is passed to them:

xdr_hyper(3nsl)
xdr_opaque(3nsl)
xdr_reference(3nsl)
xdr_vector(3nsl)

Testing Done

I ran the test attached to the bug report:

$ ./test 
xdr_hyper: OK
xdr_opaque: OK
xdr_reference: OK
xdr_vector: OK
xdr_vector: OK
$ LD_PRELOAD=$CODEMGR_WS/proto/root_i386/usr/lib/libnsl.so.1 ./test 
xdr_hyper: FAIL
xdr_opaque: FAIL
xdr_reference: FAIL
xdr_vector: FAIL
xdr_vector: FAIL
$

usr/src/lib/libnsl/rpc/xdr.c (Diff revision 1)

May be return TRUE/FALSE here depending on x_op being valid if cnt is 0? This would be an easier to understand logic change.

Marcel Telka April 24, 2017, 3:30 p.m.

Yes, I agree that the change would be easier to understand with your suggestion, but from a long term view it will just add some bloatware with no real added value.

The cnt == 0 case is so rare that it does not deserve the special handling here.  Actually, the cnt == 0 case is usually a programming error.  Thus the cnt == 0 check for every xdr_opaque(3nsl) call makes the code very likely a bit slower as it will be with this fix. :-)

Yuri Pankov April 24, 2017, 3:41 p.m.

Both suggestions (not marked as "fix it") were just to made xdr_opaque() to looks the same as other 2 functions:
if !valid return false;
if cnt == 0 return true;


I'm fine with how it's done currently though if you don't want more involved changes. Ship it!

usr/src/lib/libnsl/rpc/xdr_array.c (Diff revision 1)

Worth creating a macro?

Marcel Telka April 24, 2017, 3:30 p.m.

For just two uses in two different files?  It would need to go to some header file.  Hmmmm, I'd like to avoid headers changes for this fix.  It is not worth doing, I believe.

Ship it!

Ship It!

Ship it!

It probably would be good to create some XDR tests under usr/src/tests. Would you be willing to create a followup ticket for that?

Marcel Telka April 25, 2017, 1:29 p.m.

Sorry, no.  I do not plan to implement it, so it does not make sense for me to create such a ticket.

Ship it!

Ship It!

Vitaliy Gusev April 25, 2017, 1:54 p.m.

BTW, Are you going to create ticket for uts verstion of the xdr functions?

Marcel Telka April 25, 2017, 1:57 p.m.
```
Sorry, I do not plan to do so now.
```

Status: Closed (submitted)

Change Summary:

commit 22cc57556161a28b2141976ff578db2558def3e1
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Sun Apr 23 22:04:04 2017 +0200
Commit:     Robert Mustacchi <rm@joyent.com>
CommitDate: Wed May 3 21:21:34 2017 +0000

    8098 Some xdr_simple(3nsl) and xdr_complex(3nsl) functions can succeed for undefined xdrs->x_op values
    Reviewed by: Yuri Pankov <yuri.pankov@gmail.com>
    Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
    Reviewed by: Vitaliy Gusev <gusev.vitaliy@icloud.com>
    Approved by: Robert Mustacchi <rm@joyent.com>

:100644 100644 6d5095f... dd13140... M	usr/src/lib/libnsl/rpc/xdr.c
:100644 100644 c3b4508... 80e49cf... M	usr/src/lib/libnsl/rpc/xdr_array.c
:100644 100644 e3be8f2... c20ee18... M	usr/src/lib/libnsl/rpc/xdr_refer.c

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Status: Closed (submitted)

Change Summary: