Page MenuHomeFreeBSD
Differential D9303

Capsicumize traceroute
ClosedPublic
Actions

Authored by oshogbo on Jan 23 2017, 1:20 PM.
Tags
None
Referenced Files
F114886695: D9303.id25452.diff
Fri, Apr 18, 3:34 AM
Unknown Object (File)
Sun, Apr 13, 1:14 PM
Unknown Object (File)
Thu, Apr 10, 5:21 AM
Unknown Object (File)
Wed, Apr 9, 10:16 AM
Unknown Object (File)
Sat, Mar 22, 9:35 PM
Unknown Object (File)
Thu, Mar 20, 1:44 PM
Unknown Object (File)
Mar 17 2025, 4:41 PM
Unknown Object (File)
Mar 8 2025, 2:07 PM
View All Files
Subscribers
None

Details

Reviewers
emaste
cem
allanjude
bapt
Commits
rS314000: Capsicumize traceroute.
Summary

PR: 193973 (+ some of my changes)

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

oshogbo updated this revision to Diff 24345.Jan 23 2017, 1:20 PM
oshogbo retitled this revision from to Capsicumize traceroute.
oshogbo updated this object.
oshogbo edited the test plan for this revision. (Show Details)
oshogbo added reviewers: bapt, emaste, cem, allanjude.
emaste edited edge metadata.Jan 23 2017, 2:26 PM

Please upload with context, e.g. git diff -U9999 or the svn command from https://wiki.freebsd.org/Phabricator

contrib/traceroute/traceroute.c
561

Is this message correct in the current libcasper world order? Should it be an error?

oshogbo updated this revision to Diff 24350.Jan 23 2017, 2:33 PM
oshogbo edited edge metadata.

Thank you emaste@.

bapt added inline comments.Feb 17 2017, 8:40 PM
contrib/traceroute/traceroute.c
563

Given the if block above will exit, no need for a else block here

589

casper cannot be null at this point given there checks above that exits

1013

This will always be true there is a check above for that.

1848–1853

maybe if would be more readable to have a bunch of defined earlier:

#if HAVE_LIBCAPSER
#define tcrt_gethostbyname(domain) cpa_gethostbyname(capdns, domain)
#else 
#define tcrt_gethostbyname(domain) cpa_gethostbyname(capdns, domain)
#endif

with all functions like that

emaste added inline comments.Feb 17 2017, 8:41 PM
contrib/traceroute/traceroute.c
230

we compile with -DHAVE_LIBCASPER; does this #if work?

370–374

maybe lose the extra space here, like you do with HAVE_LIBCASPER stack variables in fns?

561–562

errx instead of perror/exit?

1024

given that we already have cansandbox variable I'd prefer if we set if false if capsicum is not available, and drop the ENOSYS case

usr.sbin/traceroute/Makefile
3–4

it seems odd to have both of these; afaik we need only src.opts.mk?

oshogbo updated this revision to Diff 25368.Feb 18 2017, 10:39 PM
oshogbo marked 2 inline comments as done.

Fixes proposed by emaste and bapt.

oshogbo marked 5 inline comments as done.Feb 18 2017, 10:43 PM
oshogbo added inline comments.
contrib/traceroute/traceroute.c
230

Not sure if I get this.

1848–1853

When we will have libcaspermock this will not be needed any more.

oshogbo updated this revision to Diff 25369.Feb 18 2017, 10:45 PM

One more fix.

oshogbo marked an inline comment as done.Feb 18 2017, 10:45 PM
oshogbo added inline comments.
contrib/traceroute/traceroute.c
230

After one more look I get this ;)

bapt accepted this revision.Feb 19 2017, 12:15 PM
bapt edited edge metadata.
This revision is now accepted and ready to land.Feb 19 2017, 12:15 PM
emaste accepted this revision.Feb 19 2017, 8:31 PM
emaste edited edge metadata.
emaste added inline comments.
contrib/traceroute/traceroute.c
565–566

what about initializing these with const char *types[] = { "NAME", "ADDR" };?

allanjude accepted this revision.Feb 19 2017, 8:33 PM
allanjude edited edge metadata.

tested fine here

Closed by commit rS314000: Capsicumize traceroute. (authored by oshogbo). · Explain WhyFeb 20 2017, 11:48 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
contrib/
traceroute/
103 lines
usr.sbin/
traceroute/
8 lines

Diff 25369

View Options

contrib/traceroute/traceroute.c

.\" Copyright (c) 1990, 1991, 1993 .\" Copyright (c) 1990, 1991, 1993 The Regents of the University of California.
.\" The Regents of the University of California. All rights reserved. .\" Copyright (c) 2015 Mariusz Zaborski <oshogbo@FreeBSD.org>
.\" All rights reserved.
.\" .\"
.\" This code is derived from software contributed to Berkeley by .\" This code is derived from software contributed to Berkeley by
.\" Chris Torek and the American National Standards Committee X3, .\" Chris Torek and the American National Standards Committee X3,
Context not available.
.\" @(#)fclose.3 8.1 (Berkeley) 6/4/93 .\" @(#)fclose.3 8.1 (Berkeley) 6/4/93
.\" $FreeBSD$ .\" $FreeBSD$
.\" .\"
.Dd April 22, 2006 .Dd Jun 1, 2015
.Dt FCLOSE 3 .Dt FCLOSE 3
.Os .Os
wblockUnsubmitted
Done
Inline Actions

No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up diffs. I'll try to include some context to make this easier.)

38 the file descriptor will be written to it.

wblock: No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up…
.Sh NAME .Sh NAME
.Nm fclose , .Nm fclose ,
.Nm fdclose ,
wblockUnsubmitted
Done
Inline Actions

Typo: s/arguemnt/argument/

wblock: Typo: s/arguemnt/argument/
wblockUnsubmitted
Done
Inline Actions

Oops, I missed the other typo:
s/diffrent/different/

wblock: Oops, I missed the other typo: s/diffrent/different/
.Nm fcloseall .Nm fcloseall
wblockUnsubmitted
Done
Inline Actions

Needs a comma after this "descriptor", where the pause is located:

42 If the stream does not have an associated file descriptor,

wblock: Needs a comma after this "descriptor", where the pause is located: 42 If the stream does not…
.Nd close a stream .Nd close a stream
.Sh LIBRARY .Sh LIBRARY
Context not available.
.In stdio.h .In stdio.h
.Ft int .Ft int
.Fn fclose "FILE *stream" .Fn fclose "FILE *stream"
.Ft int
.Fn fdclose "FILE *stream" "int *fdp"
.Ft void .Ft void
.Fn fcloseall void .Fn fcloseall void
.Sh DESCRIPTION .Sh DESCRIPTION
Context not available.
.Xr fflush 3 . .Xr fflush 3 .
.Pp .Pp
The The
.Fn fdclose
function is equivalent to
.Fn fclose
except that it does not close the underlying file descriptor.
If
.Fa fdp
is not
jillesUnsubmitted
Done
Inline Actions

Calling the argument fdp instead of fd may make this clearer.

Try:

If
.Fa fd
is not
.Dv NULL ,
the file descriptor will be written to it.

jilles: Calling the argument fdp instead of fd may make this clearer. Try: If .Fa fd is not .Dv NULL…
.Dv NULL ,
jillesUnsubmitted
Done
Inline Actions

The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have an associated file descriptor", possibly with examples like fmemopen(), funopen() and open_memstream().

These words also occur under ERRORS below.

If the stream is not open, the behaviour is not defined and need not be described in the man page.

jilles: The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have…
the file descriptor, will be written to it.
If the
.Fa fdp
arguemnt will be diffrent then NULL the file descriptor will be returned in it,
If the stream does not have an associated file descriptor
wblockUnsubmitted
Done
Inline Actions

Needs a comma after "descriptor".

wblock: Needs a comma after "descriptor".
.Fa fdp
will be set to -1.
This type of stream is created with functions such as
wblockUnsubmitted
Done
Inline Actions

"Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of stream", or maybe it does not even need to be said. "functions like" is confusing, probably should be "such as". Finally, switch to active rather than passive voice by changing "could" to "can" or "is":

Streams are created with functions such as
or
This type of stream is created with functions such as
or even
This type of stream is created with

wblock: "Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of…
.Xr fmemopen 3 ,
.Xr funopen 3 ,
or
.Xr open_memstream 3 .
.Pp
The
.Fn fcloseall .Fn fcloseall
function calls function calls
.Fn fclose .Fn fclose
on all open streams. on all open streams.
.Sh RETURN VALUES .Sh RETURN VALUES
Upon successful completion 0 is returned. .Fn fcloseall
returns no value.
.Pp
wblockUnsubmitted
Done
Inline Actions

Several things:

  1. Not necessary to use "the blah function". After the first time, just say the name of the function: "blah returns..." (no leading "The").
  2. "returns no value" is somewhat ambiguous, could be misread to mean it returns null. "does not return a value" might be more clear.

So:

.Fn fcloseall
does not return a value.

wblock: Several things: 1. Not necessary to use "the blah function". After the first time, just say…
Upon successful completion the
.Fn fclose
and
.Fn fdclose
wblockUnsubmitted
Done
Inline Actions

Start the new sentence on a new line:

99  This is intentional.

100 It makes it easier to make sure programs written under

wblock: Start the new sentence on a new line: 99 This is intentional. 100 It makes it easier to…
functions return 0.
Otherwise, Otherwise,
.Dv EOF .Dv EOF
is returned and the global variable is returned and the global variable
.Va errno .Va errno
is set to indicate the error. is set to indicate the error.
In either case no further access to the stream is possible.
.Sh ERRORS .Sh ERRORS
jillesUnsubmitted
Done
Inline Actions

This change seems unnecessary. It is still the case that there are two situations: success and error.

jilles: This change seems unnecessary. It is still the case that there are two situations: success and…
The The
.Fn fdclose
function shall fail if:
wblockUnsubmitted
Done
Inline Actions

"Shall" implies a recommendation. Probably better as "will".

wblock: "Shall" implies a recommendation. Probably better as "will".
.Bl -tag -width Er
.It Bq Er EOPNOTSUPP
The stream does not have an associated file descriptor.
.Pp
The
.Fn fclose .Fn fclose
function and
may also fail and set .Fn fdclose
functions may also fail and set
.Va errno .Va errno
for any of the errors specified for the routines for any of the errors specified for
.Xr close 2
or
.Xr fflush 3 . .Xr fflush 3 .
.Pp
jillesUnsubmitted
Done
Inline Actions

errno should keep its .Va markup

jilles: errno should keep its .Va markup
The
.Fn fclose
function may also fail and set errno for any of the errors specified for
.Xr close 2.
.Sh NOTES .Sh NOTES
The The
.Fn fclose .Fn fclose
function and
does not handle NULL arguments; they will result in a segmentation .Fn fdclose
violation. functions do not handle NULL arguments in the
This is intentional - it makes it easier to make sure programs written .Fa stream
under variable; this will result in a segmentation violation.
This is intentional. It makes it easier to make sure programs written under
wblockUnsubmitted
Done
Inline Actions

(Missing an "it" after "makes").
Use a full stop rather than an emdash:

This is intentional.
It makes it easier to make sure programs written under

wblock: (Missing an "it" after "makes"). Use a full stop rather than an emdash: This is intentional.
.Fx .Fx
are bug free. are bug free.
This behaviour is an implementation detail, and programs should not This behaviour is an implementation detail, and programs should not
Context not available.
function function
conforms to conforms to
.St -isoC . .St -isoC .
.Pp .Sh HISTORY
jillesUnsubmitted
Done
Inline Actions

This should be .Sh HISTORY.

jilles: This should be `.Sh HISTORY`.
The The
.Fn fcloseall .Fn fcloseall
function first appeared in function first appeared in
.Fx 7.0 . .Fx 7.0 .
.Pp
The
.Fn fdclose
function first appeared in
.Fx 11.0 .
Context not available.
View Options

usr.sbin/traceroute/Makefile

.\" Copyright (c) 1990, 1991, 1993 .\" Copyright (c) 1990, 1991, 1993 The Regents of the University of California.
.\" The Regents of the University of California. All rights reserved. .\" Copyright (c) 2015 Mariusz Zaborski <oshogbo@FreeBSD.org>
.\" All rights reserved.
.\" .\"
.\" This code is derived from software contributed to Berkeley by .\" This code is derived from software contributed to Berkeley by
.\" Chris Torek and the American National Standards Committee X3, .\" Chris Torek and the American National Standards Committee X3,
Context not available.
.\" @(#)fclose.3 8.1 (Berkeley) 6/4/93 .\" @(#)fclose.3 8.1 (Berkeley) 6/4/93
.\" $FreeBSD$ .\" $FreeBSD$
.\" .\"
.Dd April 22, 2006 .Dd Jun 1, 2015
.Dt FCLOSE 3 .Dt FCLOSE 3
.Os .Os
wblockUnsubmitted
Done
Inline Actions

No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up diffs. I'll try to include some context to make this easier.)

38 the file descriptor will be written to it.

wblock: No comma after "descriptor" here. (This might be due to phabricator "helping" by mixing up…
.Sh NAME .Sh NAME
.Nm fclose , .Nm fclose ,
.Nm fdclose ,
wblockUnsubmitted
Done
Inline Actions

Typo: s/arguemnt/argument/

wblock: Typo: s/arguemnt/argument/
wblockUnsubmitted
Done
Inline Actions

Oops, I missed the other typo:
s/diffrent/different/

wblock: Oops, I missed the other typo: s/diffrent/different/
.Nm fcloseall .Nm fcloseall
wblockUnsubmitted
Done
Inline Actions

Needs a comma after this "descriptor", where the pause is located:

42 If the stream does not have an associated file descriptor,

wblock: Needs a comma after this "descriptor", where the pause is located: 42 If the stream does not…
.Nd close a stream .Nd close a stream
.Sh LIBRARY .Sh LIBRARY
Context not available.
.In stdio.h .In stdio.h
.Ft int .Ft int
.Fn fclose "FILE *stream" .Fn fclose "FILE *stream"
.Ft int
.Fn fdclose "FILE *stream" "int *fdp"
.Ft void .Ft void
.Fn fcloseall void .Fn fcloseall void
.Sh DESCRIPTION .Sh DESCRIPTION
Context not available.
.Xr fflush 3 . .Xr fflush 3 .
.Pp .Pp
The The
.Fn fdclose
function is equivalent to
.Fn fclose
except that it does not close the underlying file descriptor.
If
.Fa fdp
is not
jillesUnsubmitted
Done
Inline Actions

Calling the argument fdp instead of fd may make this clearer.

Try:

If
.Fa fd
is not
.Dv NULL ,
the file descriptor will be written to it.

jilles: Calling the argument fdp instead of fd may make this clearer. Try: If .Fa fd is not .Dv NULL…
.Dv NULL ,
jillesUnsubmitted
Done
Inline Actions

The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have an associated file descriptor", possibly with examples like fmemopen(), funopen() and open_memstream().

These words also occur under ERRORS below.

If the stream is not open, the behaviour is not defined and need not be described in the man page.

jilles: The words "non-standard close method" may be unclear. Perhaps say "If the stream does not have…
the file descriptor, will be written to it.
If the
.Fa fdp
arguemnt will be diffrent then NULL the file descriptor will be returned in it,
If the stream does not have an associated file descriptor
wblockUnsubmitted
Done
Inline Actions

Needs a comma after "descriptor".

wblock: Needs a comma after "descriptor".
.Fa fdp
will be set to -1.
This type of stream is created with functions such as
wblockUnsubmitted
Done
Inline Actions

"Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of stream", or maybe it does not even need to be said. "functions like" is confusing, probably should be "such as". Finally, switch to active rather than passive voice by changing "could" to "can" or "is":

Streams are created with functions such as
or
This type of stream is created with functions such as
or even
This type of stream is created with

wblock: "Such stream" should be "Such a stream", but it's a little confusing, maybe "This type of…
.Xr fmemopen 3 ,
.Xr funopen 3 ,
or
.Xr open_memstream 3 .
.Pp
The
.Fn fcloseall .Fn fcloseall
function calls function calls
.Fn fclose .Fn fclose
on all open streams. on all open streams.
.Sh RETURN VALUES .Sh RETURN VALUES
Upon successful completion 0 is returned. .Fn fcloseall
returns no value.
.Pp
wblockUnsubmitted
Done
Inline Actions

Several things:

  1. Not necessary to use "the blah function". After the first time, just say the name of the function: "blah returns..." (no leading "The").
  2. "returns no value" is somewhat ambiguous, could be misread to mean it returns null. "does not return a value" might be more clear.

So:

.Fn fcloseall
does not return a value.

wblock: Several things: 1. Not necessary to use "the blah function". After the first time, just say…
Upon successful completion the
.Fn fclose
and
.Fn fdclose
wblockUnsubmitted
Done
Inline Actions

Start the new sentence on a new line:

99  This is intentional.

100 It makes it easier to make sure programs written under

wblock: Start the new sentence on a new line: 99 This is intentional. 100 It makes it easier to…
functions return 0.
Otherwise, Otherwise,
.Dv EOF .Dv EOF
is returned and the global variable is returned and the global variable
.Va errno .Va errno
is set to indicate the error. is set to indicate the error.
In either case no further access to the stream is possible.
.Sh ERRORS .Sh ERRORS
jillesUnsubmitted
Done
Inline Actions

This change seems unnecessary. It is still the case that there are two situations: success and error.

jilles: This change seems unnecessary. It is still the case that there are two situations: success and…
The The
.Fn fdclose
function shall fail if:
wblockUnsubmitted
Done
Inline Actions

"Shall" implies a recommendation. Probably better as "will".

wblock: "Shall" implies a recommendation. Probably better as "will".
.Bl -tag -width Er
.It Bq Er EOPNOTSUPP
The stream does not have an associated file descriptor.
.Pp
The
.Fn fclose .Fn fclose
function and
may also fail and set .Fn fdclose
functions may also fail and set
.Va errno .Va errno
for any of the errors specified for the routines for any of the errors specified for
.Xr close 2
or
.Xr fflush 3 . .Xr fflush 3 .
.Pp
jillesUnsubmitted
Done
Inline Actions

errno should keep its .Va markup

jilles: errno should keep its .Va markup
The
.Fn fclose
function may also fail and set errno for any of the errors specified for
.Xr close 2.
.Sh NOTES .Sh NOTES
The The
.Fn fclose .Fn fclose
function and
does not handle NULL arguments; they will result in a segmentation .Fn fdclose
violation. functions do not handle NULL arguments in the
This is intentional - it makes it easier to make sure programs written .Fa stream
under variable; this will result in a segmentation violation.
This is intentional. It makes it easier to make sure programs written under
wblockUnsubmitted
Done
Inline Actions

(Missing an "it" after "makes").
Use a full stop rather than an emdash:

This is intentional.
It makes it easier to make sure programs written under

wblock: (Missing an "it" after "makes"). Use a full stop rather than an emdash: This is intentional.
.Fx .Fx
are bug free. are bug free.
This behaviour is an implementation detail, and programs should not This behaviour is an implementation detail, and programs should not
Context not available.
function function
conforms to conforms to
.St -isoC . .St -isoC .
.Pp .Sh HISTORY
jillesUnsubmitted
Done
Inline Actions

This should be .Sh HISTORY.

jilles: This should be `.Sh HISTORY`.
The The
.Fn fcloseall .Fn fcloseall
function first appeared in function first appeared in
.Fx 7.0 . .Fx 7.0 .
.Pp
The
.Fn fdclose
function first appeared in
.Fx 11.0 .
Context not available.