Re: refcount btrace
On Mon, Apr 11, 2022 at 07:19:00PM +0200, Martin Pieuchot wrote:
> On 08/04/22(Fri) 12:16, Alexander Bluhm wrote:
> > On Fri, Apr 08, 2022 at 02:39:34AM +, Visa Hankala wrote:
> > > On Thu, Apr 07, 2022 at 07:55:11PM +0200, Alexander Bluhm wrote:
> > > > On Wed, Mar 23, 2022 at 06:13:27PM +0100, Alexander Bluhm wrote:
> > > > > In my opinion tracepoints give insight at minimal cost. It is worth
> > > > > it to have it in GENERIC to make it easy to use.
> > > >
> > > > After release I want to revive the btrace of refcounts discussion.
> > > >
> > > > As mpi@ mentioned the idea of dt(4) is to have these trace points
> > > > in GENERIC kernel. If you want to hunt a bug, just turn it on.
> > > > Refounting is a common place for bugs, leaks can be detected easily.
> > > >
> > > > The alternative are some defines that you can compile in and access
> > > > from ddb. This is more work and you would have to implement it for
> > > > every recount.
> > > > https://marc.info/?l=openbsd-tech=163786435916039=2
> > > >
> > > > There is no measuarable performance difference. dt(4) is written
> > > > in a way that is is only one additional branch. At least my goal
> > > > is to add trace points to useful places when we identify them.
> > >
> > > DT_INDEX_ENTER() still checks the index first, so it has two branches
> > > in practice.
> > >
> > > I think dt_tracing should be checked first so that it serves as
> > > a gateway to the trace code. Under normal operation, the check's
> > > outcome is always the same, which is easy even for simple branch
> > > predictors.
> >
> > Reordering the check is easy. Now dt_tracing is first.
> >
> > > I have a slight suspicion that dt(4) is now becoming a way to add code
> > > that would be otherwise unacceptable. Also, how "durable" are trace
> > > points perceived? Is an added trace point an achieved advantage that
> > > is difficult to remove even when its utility has diminished? There is
> > > a similarity to (ad hoc) debug printfs too.
> >
> > As I understand dt(4) it is a replacement for debug printfs. But
> > it has advantages. I can be turnd on selectively from userland.
> > It does not spam the console, but can be processed in userland. It
> > is always there, you don't have to recompile.
> >
> > Of course you always have the printf or tracepoint at the worng
> > place. I think people debugging the code should move them to
> > the useful places. Then we may end with generally useful tool.
> > A least that is my hope.
> >
> > There are obvious places to debug. We have syscall entry and return.
> > And I think reference counting is also generally interesting.
>
> I'm happy if this can help debugging real reference counting issues. Do
> you have a script that could be committed to /usr/share/btrace to show
> how to track reference counting using these probes?
Script looks like this:
#!/usr/sbin/btrace
tracepoint:refcnt:inpcb{
printf("%s %x %u %+d\n", probe, arg0, arg1, arg2)
}
Note that output should be -1 instead of +4294967295, but that is
a different problem.
tracepoint:refcnt:inpcb fd80793885c0 2 +1
tracepoint:refcnt:inpcb fd80793885c0 3 +4294967295
tracepoint:refcnt:inpcb fd80793885c0 2 +1
tracepoint:refcnt:inpcb fd80793885c0 3 +4294967295
tracepoint:refcnt:inpcb fd80793885c0 2 +1
tracepoint:refcnt:inpcb fd80793885c0 3 +4294967295
Or with kernel stack:
#!/usr/sbin/btrace
tracepoint:refcnt:inpcb{
printf("%s %x %u %+d%s\n", probe, arg0, arg1, arg2, kstack)
}
tracepoint:refcnt:inpcb fd80793885c0 3 +4294967295
refcnt_rele+0x88
in_pcbunref+0x24
pf_find_state+0x2a6
pf_test_state+0x172
pf_test+0xd17
ip6_output+0xd14
tcp_output+0x164f
tcp_usrreq+0x386
sosend+0x37c
dofilewritev+0x14d
sys_write+0x51
syscall+0x314
Xsyscall+0x128
kernel
> > Index: dev/dt/dt_prov_static.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dt_prov_static.c,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 dt_prov_static.c
> > --- dev/dt/dt_prov_static.c 17 Mar 2022 14:53:59 - 1.13
> > +++ dev/dt/dt_prov_static.c 8 Apr 2022 09:40:29 -
> > @@ -87,6 +87,12 @@ DT_STATIC_PROBE1(smr, barrier_exit, "int
> > DT_STATIC_PROBE0(smr, wakeup);
> > DT_STATIC_PROBE2(smr, thread, "uint64_t", "uint64_t");
> >
> > +/*
> > + * reference counting
> > + */
> > +DT_STATIC_PROBE0(refcnt, none);
> > +DT_STATIC_PROBE3(refcnt, inpcb, "void *", "int", "int");
> > +DT_STATIC_PROBE3(refcnt, tdb, "void *", "int", "int");
> >
> > /*
> > * List of all static probes
> > @@ -127,15 +133,24 @@ struct dt_probe *const dtps_static[] = {
> > &_DT_STATIC_P(smr, barrier_exit),
> > &_DT_STATIC_P(smr, wakeup),
> > &_DT_STATIC_P(smr, thread),
> > + /* refcnt */
> > + &_DT_STATIC_P(refcnt, none),
> > + &_DT_STATIC_P(refcnt, inpcb),
> > + &_DT_STATIC_P(refcnt, tdb),
> > };
> >
> > +struct dt_probe *const *dtps_index_refcnt;
> > +
> > int
> >
Re: Kill selrecord()
I don't think this is the right time to rush and commit big diffs which noone can actively test, not even build on all architectures, because there are no snapshots being built, because the tree is semi-frozen to make a release. visa is right. Visa Hankala wrote: > On Mon, Apr 11, 2022 at 07:14:46PM +0200, Martin Pieuchot wrote: > > Now that poll(2) & select(2) use the kqueue backend under the hood we > > can start retiring the old machinery. > > > > The diff below does not touch driver definitions, however it : > > > > - kills selrecord() & doselwakeup() > > > > - make it obvious that `kern.nselcoll' is now always 0 > > > > - Change all poll/select hooks to return 0 > > > > - Kill a seltid check in wsdisplaystart() which is now always true > > > > In a later step we could remove the *_poll() requirement from device > > drivers and kill seltrue() & selfalse(). > > > > ok? > > I was planning to wait for two weeks after the general availability > of the 7.1 release, and then begin the dismantling in logical order > from fo_poll. > > The final tier of the new code's testing will begin when release users > upgrade their production machines. Because of this, I would refrain > from burning bridges just yet. >
Re: vfs: document (and correct) the protection required for manipulating v_numoutput
On Sun, Mar 27, 2022 at 03:36:20PM +0200, Sebastien Marie wrote:
> v_numoutput is a struct member of vnode which is used to keep track the
> number
> of writes in progress.
>
> in several function comments, it is marked as "Manipulates v_numoutput. Must
> be
> called at splbio()".
>
> So I added a "[B]" mark in the comment to properly document the need of
> IPL_BIO
> protection.
>
> Next, I audited the tree for usage. I found 2 occurrences of v_numoutput
> (modification) without the required protection, inside dev/softraid.c. I
> added
> them.
>
> Comments or OK ?
Please move the declarations of `s' next to the other variable
declarations at the starts of the functions. With that, OK visa@
> Index: dev/softraid.c
> ===
> RCS file: /cvs/src/sys/dev/softraid.c,v
> retrieving revision 1.422
> diff -u -p -r1.422 softraid.c
> --- dev/softraid.c20 Mar 2022 13:14:02 - 1.422
> +++ dev/softraid.c27 Mar 2022 13:28:55 -
> @@ -437,8 +437,12 @@ sr_rw(struct sr_softc *sc, dev_t dev, ch
> b.b_resid = bufsize;
> b.b_vp = vp;
>
> - if ((b.b_flags & B_READ) == 0)
> + if ((b.b_flags & B_READ) == 0) {
> + int s;
> + s = splbio();
> vp->v_numoutput++;
> + splx(s);
> + }
>
> LIST_INIT(_dep);
> VOP_STRATEGY(vp, );
> @@ -2006,8 +2010,12 @@ sr_ccb_rw(struct sr_discipline *sd, int
> ccb->ccb_buf.b_vp = sc->src_vn;
> ccb->ccb_buf.b_bq = NULL;
>
> - if (!ISSET(ccb->ccb_buf.b_flags, B_READ))
> + if (!ISSET(ccb->ccb_buf.b_flags, B_READ)) {
> + int s;
> + s = splbio();
> ccb->ccb_buf.b_vp->v_numoutput++;
> + splx(s);
> + }
>
> LIST_INIT(>ccb_buf.b_dep);
>
> Index: sys/vnode.h
> ===
> RCS file: /cvs/src/sys/sys/vnode.h,v
> retrieving revision 1.163
> diff -u -p -r1.163 vnode.h
> --- sys/vnode.h 12 Dec 2021 09:14:59 - 1.163
> +++ sys/vnode.h 27 Mar 2022 13:28:56 -
> @@ -89,6 +89,7 @@ RBT_HEAD(namecache_rb_cache, namecache);
> * Locks used to protect struct members in struct vnode:
> * a atomic
> * V vnode_mtx
> + * B IPL_BIO
> */
> struct uvm_vnode;
> struct vnode {
> @@ -113,7 +114,7 @@ struct vnode {
> struct buf_rb_bufs v_bufs_tree;/* lookup of all bufs */
> struct buflists v_cleanblkhd; /* clean blocklist head */
> struct buflists v_dirtyblkhd; /* dirty blocklist head */
> - u_int v_numoutput;/* num of writes in progress */
> + u_int v_numoutput;/* [B] num of writes in progress */
> LIST_ENTRY(vnode) v_synclist; /* vnode with dirty buffers */
> union {
> struct mount*vu_mountedhere;/* ptr to mounted vfs (VDIR) */
>
Re: Kill selrecord()
On Mon, Apr 11, 2022 at 07:14:46PM +0200, Martin Pieuchot wrote: > Now that poll(2) & select(2) use the kqueue backend under the hood we > can start retiring the old machinery. > > The diff below does not touch driver definitions, however it : > > - kills selrecord() & doselwakeup() > > - make it obvious that `kern.nselcoll' is now always 0 > > - Change all poll/select hooks to return 0 > > - Kill a seltid check in wsdisplaystart() which is now always true > > In a later step we could remove the *_poll() requirement from device > drivers and kill seltrue() & selfalse(). > > ok? I was planning to wait for two weeks after the general availability of the 7.1 release, and then begin the dismantling in logical order from fo_poll. The final tier of the new code's testing will begin when release users upgrade their production machines. Because of this, I would refrain from burning bridges just yet.
fix openrsync on big endian archs
Hit this on sparc64. io_read_ulong() calls io_read_int() which already
does the le32toh() call. So skip the 2nd le32toh() call here.
With this openrsync works a lot better.
--
:wq Claudio
Index: io.c
===
RCS file: /cvs/src/usr.bin/rsync/io.c,v
retrieving revision 1.21
diff -u -p -r1.21 io.c
--- io.c28 Dec 2021 11:59:48 - 1.21
+++ io.c12 Apr 2022 12:29:23 -
@@ -585,8 +585,9 @@ io_read_ulong(struct sess *sess, int fd,
if (!io_read_int(sess, fd, )) {
ERRX1("io_read_int");
return 0;
- } else if (sval != -1) {
- *val = (uint64_t)le32toh(sval);
+ }
+ if (sval != -1) {
+ *val = sval;
return 1;
}
Re: vfs: document (and correct) the protection required for manipulating v_numoutput
On Sun, Mar 27, 2022 at 03:36:20PM +0200, Sebastien Marie wrote:
> Hi,
>
> v_numoutput is a struct member of vnode which is used to keep track the
> number
> of writes in progress.
>
> in several function comments, it is marked as "Manipulates v_numoutput. Must
> be
> called at splbio()".
>
> So I added a "[B]" mark in the comment to properly document the need of
> IPL_BIO
> protection.
>
> Next, I audited the tree for usage. I found 2 occurrences of v_numoutput
> (modification) without the required protection, inside dev/softraid.c. I
> added
> them.
>
> Comments or OK ?
anyone ?
the purpose of splbio() is to protect v_numoutput manipulation from interrupts.
for example, bread_cluster_callback() is called from interrupt context (per
code
documentation), and could modify v_numoutput (code path: bread_cluster_callback
-> biodone -> vwakeup).
to ensure that it effectively required (code not already called at IPL_BIO
level), I added some splassert() (the machine is using softraid with encrypting
discipline):
- for sr_rw:
splassert_check(6,81fde589,30859b2dea1d26e8,400,8000,8000) at
splassert_check+0x4d
sr_rw(8041f000,400,80472000,8000,10,0) at sr_rw+0x1fe
sr_meta_save(80422000,1,628df3ebb476eac1,80422000,82712a68,0)
at sr_meta_save+0x23d
sr_ioctl_createraid(8041f000,82712a68,0,822fe290,cae617dbeff84e1f,82712d50)
at sr_ioctl_createraid+0xe03
sr_boot_assembly(8041f000,8041f000,e2eb5961e3789080,8041f328,8041f000,8229fd40)
at sr_boot_assembly+0x936
sr_attach(0,8041f000,0,0,ab529b23aa62cd2b,0) at sr_attach+0x157
config_attach(0,822bff60,0,0,33d5fbe4fc2cb990,c00) at
config_attach+0x1f4
main(0,0,c00,1001000,100,8000fa40) at main+0x4fb
splassert: sr_rw: want 6 have 0
Starting stack trace...
splassert_check(6,81fde589) at splassert_check+0x4d
sr_rw(8041f000,413,81a52000,8000,10,0) at sr_rw+0x1fe
sr_meta_save(81a0b000,1) at sr_meta_save+0x23d
sr_ioctl_createraid(8041f000,8199b800,1,0) at
sr_ioctl_createraid+0xe03
sr_bio_handler(8041f000,0,c2e84226,8199b800) at
sr_bio_handler+0x223
VOP_IOCTL(8000a7ae9730,c2e84226,8199b800,3,fd879e7e4f00,8000a76acd28)
at VOP_IOCTL+0x5c
vn_ioctl(fd8746f39080,c2e84226,8199b800,8000a76acd28) at
vn_ioctl+0x75
sys_ioctl(8000a76acd28,8000a7d277e0,8000a7d27830) at sys_ioctl+0x2c4
syscall(8000a7d278a0) at syscall+0x374
Xsyscall() at Xsyscall+0x128
end of kernel
- for sr_ccb_rw, I had some problem to get proper/readable stack trace as the
console is just spammed too much and stack trace are interleaved.
splassert: sr_ccb_rw: want 6 have 0
Starting stack trace...
splassert_check(6,81f0b74d) at splassert_check+0x4d
sr_ccb_rw(80422000,0,836b80,3800,800021bff000,1001,f0cbd67d55f39eb0)
at sr_ccb_rw+0x194
sr_crypto_dev_rw(80423300,80423300) at sr_crypto_dev_rw+0x4a
sr_crypto_rw(80423300) at sr_crypto_rw+0xb6
sr_scsi_cmd(fd86af9f11c0) at sr_scsi_cmd+0x30c
scsi_xs_exec(fd86af9f11c0) at scsi_xs_exec+0x3f
sdstart(fd86af9f11c0) at sdstart+0x2d1
scsi_iopool_run(80422aa8) at scsi_iopool_run+0x159
scsi_xsh_runqueued86af9f11c0) at scsi_xs_exec+0x3f
sdstart(fd86af9f11c0) 0419d80) at scsi_xsh_add+0x91
sdstrategy(fd86b037fec8) at sdstrategy+0x112
spec_strategy(8000a7689c88) at spec_strategy+0x56
VOP_STRATEGY(8000fffda770,fd86b037fec8) at VOP_STRATEGY+0x4c
ufs_strategy(8000a7689d18) at ufs_strategy+0xe5
VOP_STRATEGY(8000a78bab98,fd86b037fec8) at VOP_STRATEGY+0x4c
bwrite(fd86b037fec8) at bwrite+0x138
ufs_dirremove(8000a78bab98,fd874cda31e0,800c,0) at ufs_dirremove+0x19f
ufs_remove(8000a7689e50) at ufs_remove+0xbe
VOP_REMOVE(80s_truncate+0x7d0
ufs_inactive(8000a7689d98) at ufs_inactive+
dounlinkat(8000a766e540,ff9c,146b649b778,0) at dounlinkat+0xbd
syscall(8000a768a070) at syscall+0x374
Xsyscall() at Xsyscall+0x128
end of kernel
end trace frame: 0x7f7bffa0, count: 235
End of stack trace.
> --
> Sebastien Marie
>
> Index: dev/softraid.c
> ===
> RCS file: /cvs/src/sys/dev/softraid.c,v
> retrieving revision 1.422
> diff -u -p -r1.422 softraid.c
> --- dev/softraid.c20 Mar 2022 13:14:02 - 1.422
> +++ dev/softraid.c27 Mar 2022 13:28:55 -
> @@ -437,8 +437,12 @@ sr_rw(struct sr_softc *sc, dev_t dev, ch
> b.b_resid = bufsize;
> b.b_vp = vp;
>
> - if ((b.b_flags & B_READ) == 0)
> + if ((b.b_flags & B_READ) == 0) {
> + int s;
> + s = splbio();
> vp->v_numoutput++;
> + splx(s);
> + }
>
> LIST_INIT(_dep);
>
rpki-client: move sbgp_sia() to a better place
This only moves sbgp_sia() a bit down between the code handling the
NID_sbgp_ipAddrBlock and the NID_certificate_policies extensions. Surely
this is better than to stick it between some random helper functions for
sbgp_ipaddrblk() and sbgp_assysnum().
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.67
diff -u -p -r1.67 cert.c
--- cert.c 12 Apr 2022 08:45:34 - 1.67
+++ cert.c 12 Apr 2022 09:02:24 -
@@ -125,75 +125,6 @@ sbgp_addr(struct parse *p,
}
/*
- * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_sia(struct parse *p, X509_EXTENSION *ext)
-{
- AUTHORITY_INFO_ACCESS *sia = NULL;
- ACCESS_DESCRIPTION *ad;
- ASN1_OBJECT *oid;
- int i, rc = 0;
-
- if (X509_EXTENSION_get_critical(ext)) {
- warnx("%s: RFC 6487 section 4.8.8: SIA: "
- "extension not non-critical", p->fn);
- goto out;
- }
-
- if ((sia = X509V3_EXT_d2i(ext)) == NULL) {
- cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
- "failed extension parse", p->fn);
- goto out;
- }
-
- for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
- ad = sk_ACCESS_DESCRIPTION_value(sia, i);
-
- oid = ad->method;
-
- if (OBJ_cmp(oid, carepo_oid) == 0) {
- if (!x509_location(p->fn, "SIA: caRepository",
- "rsync://", ad->location, >res->repo))
- goto out;
- } else if (OBJ_cmp(oid, manifest_oid) == 0) {
- if (!x509_location(p->fn, "SIA: rpkiManifest",
- "rsync://", ad->location, >res->mft))
- goto out;
- } else if (OBJ_cmp(oid, notify_oid) == 0) {
- if (!x509_location(p->fn, "SIA: rpkiNotify",
- "https://;, ad->location, >res->notify))
- goto out;
- }
- }
-
- if (p->res->mft == NULL || p->res->repo == NULL) {
- warnx("%s: RFC 6487 section 4.8.8: SIA missing caRepository "
- "or rpkiManifest", p->fn);
- goto out;
- }
-
- if (strstr(p->res->mft, p->res->repo) != p->res->mft) {
- warnx("%s: RFC 6487 section 4.8.8: SIA: "
- "conflicting URIs for caRepository and rpkiManifest",
- p->fn);
- goto out;
- }
-
- if (rtype_from_file_extension(p->res->mft) != RTYPE_MFT) {
- warnx("%s: RFC 6487 section 4.8.8: SIA: "
- "not an MFT file", p->fn);
- goto out;
- }
-
- rc = 1;
- out:
- AUTHORITY_INFO_ACCESS_free(sia);
- return rc;
-}
-
-/*
* Parse a range of addresses as in 3.2.3.8.
* Returns zero on failure, non-zero on success.
*/
@@ -773,6 +704,75 @@ out:
sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
sk_ASN1_TYPE_pop_free(sseq, ASN1_TYPE_free);
free(sv);
+ return rc;
+}
+
+/*
+ * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
+ * Returns zero on failure, non-zero on success.
+ */
+static int
+sbgp_sia(struct parse *p, X509_EXTENSION *ext)
+{
+ AUTHORITY_INFO_ACCESS *sia = NULL;
+ ACCESS_DESCRIPTION *ad;
+ ASN1_OBJECT *oid;
+ int i, rc = 0;
+
+ if (X509_EXTENSION_get_critical(ext)) {
+ warnx("%s: RFC 6487 section 4.8.8: SIA: "
+ "extension not non-critical", p->fn);
+ goto out;
+ }
+
+ if ((sia = X509V3_EXT_d2i(ext)) == NULL) {
+ cryptowarnx("%s: RFC 6487 section 4.8.8: SIA: "
+ "failed extension parse", p->fn);
+ goto out;
+ }
+
+ for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
+ ad = sk_ACCESS_DESCRIPTION_value(sia, i);
+
+ oid = ad->method;
+
+ if (OBJ_cmp(oid, carepo_oid) == 0) {
+ if (!x509_location(p->fn, "SIA: caRepository",
+ "rsync://", ad->location, >res->repo))
+ goto out;
+ } else if (OBJ_cmp(oid, manifest_oid) == 0) {
+ if (!x509_location(p->fn, "SIA: rpkiManifest",
+ "rsync://", ad->location, >res->mft))
+ goto out;
+ } else if (OBJ_cmp(oid, notify_oid) == 0) {
+ if (!x509_location(p->fn, "SIA: rpkiNotify",
+ "https://;, ad->location, >res->notify))
+ goto out;
+ }
+ }
+
+ if (p->res->mft
Re: rpki-client: reuse URI location code for AIAs and CRLs
On Tue, Apr 12, 2022 at 09:58:21AM +0200, Theo Buehler wrote:
> We can generalize sbgp_sia_location() and reuse it for AIAs and CRLs.
> This makes the checks a bit more stringent, which seems to be fine in
> practice. It also ensures that there are no embedded NULs which came
> up recently. One thing I'm not sure about is that valid_uri() refuses
> URIs with "/." which is an additional check.
>
> I'll also fix the regress Makefile.inc if this goes in.
We do not allow /. in URI to avoid both hidden dirs and path traversals
from happening. Since AIA and CRL are both RPKI URI they need to respect
the same rules as caRepository and manifest URI. So doing this check there
as well is fine.
OK claudio@
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 cert.c
> --- cert.c11 Apr 2022 10:39:45 - 1.66
> +++ cert.c12 Apr 2022 07:28:38 -
> @@ -125,40 +125,6 @@ sbgp_addr(struct parse *p,
> }
>
> /*
> - * Extract and validate a SIA accessLocation, RFC 6487, 4.8.8 and RFC 8192,
> 3.2.
> - * Returns 0 on failure and 1 on success.
> - */
> -static int
> -sbgp_sia_location(const char *fn, const char *descr, const char *proto,
> -GENERAL_NAME *location, char **out)
> -{
> - ASN1_IA5STRING *uri;
> -
> - if (*out != NULL) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: %s already specified",
> - fn, descr);
> - return 0;
> - }
> -
> - if (location->type != GEN_URI) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: %s not URI", fn, descr);
> - return 0;
> - }
> -
> - uri = location->d.uniformResourceIdentifier;
> -
> - if (!valid_uri(uri->data, uri->length, proto)) {
> - warnx("%s: RFC 6487 section 4.8.8: bad %s location", fn, descr);
> - return 0;
> - }
> -
> - if ((*out = strndup(uri->data, uri->length)) == NULL)
> - err(1, NULL);
> -
> - return 1;
> -}
> -
> -/*
> * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> * Returns zero on failure, non-zero on success.
> */
> @@ -188,15 +154,15 @@ sbgp_sia(struct parse *p, X509_EXTENSION
> oid = ad->method;
>
> if (OBJ_cmp(oid, carepo_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "caRepository",
> + if (!x509_location(p->fn, "SIA: caRepository",
> "rsync://", ad->location, >res->repo))
> goto out;
> } else if (OBJ_cmp(oid, manifest_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "rpkiManifest",
> + if (!x509_location(p->fn, "SIA: rpkiManifest",
> "rsync://", ad->location, >res->mft))
> goto out;
> } else if (OBJ_cmp(oid, notify_oid) == 0) {
> - if (!sbgp_sia_location(p->fn, "rpkiNotify",
> + if (!x509_location(p->fn, "SIA: rpkiNotify",
> "https://;, ad->location, >res->notify))
> goto out;
> }
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.125
> diff -u -p -r1.125 extern.h
> --- extern.h 4 Apr 2022 16:02:54 - 1.125
> +++ extern.h 12 Apr 2022 07:33:20 -
> @@ -22,6 +22,7 @@
> #include
>
> #include
> +#include
>
> enum cert_as_type {
> CERT_AS_ID, /* single identifier */
> @@ -589,6 +590,8 @@ char *x509_get_pubkey(X509 *, const cha
> enum cert_purpose x509_get_purpose(X509 *, const char *);
> int x509_get_time(const ASN1_TIME *, time_t *);
> char *x509_convert_seqnum(const char *, const ASN1_INTEGER *);
> +int x509_location(const char *, const char *, const char *,
> + GENERAL_NAME *, char **);
>
> /* printers */
> char *time2str(time_t);
> Index: x509.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 x509.c
> --- x509.c8 Apr 2022 15:29:59 - 1.39
> +++ x509.c12 Apr 2022 07:33:02 -
> @@ -306,24 +306,10 @@ x509_get_aia(X509 *x, const char *fn, ch
> "expected caIssuers, have %d", fn, OBJ_obj2nid(ad->method));
> goto out;
> }
> - if (ad->location->type != GEN_URI) {
> - warnx("%s: RFC 6487 section 4.8.7: AIA: "
> - "want GEN_URI type, have %d", fn, ad->location->type);
> - goto out;
> - }
>
> - if (ASN1_STRING_length(ad->location->d.uniformResourceIdentifier)
> - > MAX_URI_LENGTH) {
> - warnx("%s: RFC 6487 section 4.8.7: AIA: "
> -
rpki-client: reuse URI location code for AIAs and CRLs
We can generalize sbgp_sia_location() and reuse it for AIAs and CRLs.
This makes the checks a bit more stringent, which seems to be fine in
practice. It also ensures that there are no embedded NULs which came
up recently. One thing I'm not sure about is that valid_uri() refuses
URIs with "/." which is an additional check.
I'll also fix the regress Makefile.inc if this goes in.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.66
diff -u -p -r1.66 cert.c
--- cert.c 11 Apr 2022 10:39:45 - 1.66
+++ cert.c 12 Apr 2022 07:28:38 -
@@ -125,40 +125,6 @@ sbgp_addr(struct parse *p,
}
/*
- * Extract and validate a SIA accessLocation, RFC 6487, 4.8.8 and RFC 8192,
3.2.
- * Returns 0 on failure and 1 on success.
- */
-static int
-sbgp_sia_location(const char *fn, const char *descr, const char *proto,
-GENERAL_NAME *location, char **out)
-{
- ASN1_IA5STRING *uri;
-
- if (*out != NULL) {
- warnx("%s: RFC 6487 section 4.8.8: SIA: %s already specified",
- fn, descr);
- return 0;
- }
-
- if (location->type != GEN_URI) {
- warnx("%s: RFC 6487 section 4.8.8: SIA: %s not URI", fn, descr);
- return 0;
- }
-
- uri = location->d.uniformResourceIdentifier;
-
- if (!valid_uri(uri->data, uri->length, proto)) {
- warnx("%s: RFC 6487 section 4.8.8: bad %s location", fn, descr);
- return 0;
- }
-
- if ((*out = strndup(uri->data, uri->length)) == NULL)
- err(1, NULL);
-
- return 1;
-}
-
-/*
* Parse "Subject Information Access" extension, RFC 6487 4.8.8.
* Returns zero on failure, non-zero on success.
*/
@@ -188,15 +154,15 @@ sbgp_sia(struct parse *p, X509_EXTENSION
oid = ad->method;
if (OBJ_cmp(oid, carepo_oid) == 0) {
- if (!sbgp_sia_location(p->fn, "caRepository",
+ if (!x509_location(p->fn, "SIA: caRepository",
"rsync://", ad->location, >res->repo))
goto out;
} else if (OBJ_cmp(oid, manifest_oid) == 0) {
- if (!sbgp_sia_location(p->fn, "rpkiManifest",
+ if (!x509_location(p->fn, "SIA: rpkiManifest",
"rsync://", ad->location, >res->mft))
goto out;
} else if (OBJ_cmp(oid, notify_oid) == 0) {
- if (!sbgp_sia_location(p->fn, "rpkiNotify",
+ if (!x509_location(p->fn, "SIA: rpkiNotify",
"https://;, ad->location, >res->notify))
goto out;
}
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.125
diff -u -p -r1.125 extern.h
--- extern.h4 Apr 2022 16:02:54 - 1.125
+++ extern.h12 Apr 2022 07:33:20 -
@@ -22,6 +22,7 @@
#include
#include
+#include
enum cert_as_type {
CERT_AS_ID, /* single identifier */
@@ -589,6 +590,8 @@ char*x509_get_pubkey(X509 *, const cha
enum cert_purpose x509_get_purpose(X509 *, const char *);
int x509_get_time(const ASN1_TIME *, time_t *);
char *x509_convert_seqnum(const char *, const ASN1_INTEGER *);
+int x509_location(const char *, const char *, const char *,
+ GENERAL_NAME *, char **);
/* printers */
char *time2str(time_t);
Index: x509.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.39
diff -u -p -r1.39 x509.c
--- x509.c 8 Apr 2022 15:29:59 - 1.39
+++ x509.c 12 Apr 2022 07:33:02 -
@@ -306,24 +306,10 @@ x509_get_aia(X509 *x, const char *fn, ch
"expected caIssuers, have %d", fn, OBJ_obj2nid(ad->method));
goto out;
}
- if (ad->location->type != GEN_URI) {
- warnx("%s: RFC 6487 section 4.8.7: AIA: "
- "want GEN_URI type, have %d", fn, ad->location->type);
- goto out;
- }
- if (ASN1_STRING_length(ad->location->d.uniformResourceIdentifier)
- > MAX_URI_LENGTH) {
- warnx("%s: RFC 6487 section 4.8.7: AIA: "
- "URI exceeds max length of %d", fn, MAX_URI_LENGTH);
+ if (!x509_location(fn, "AIA: caIssuers", NULL, ad->location, aia))
goto out;
- }
- *aia = strndup(
- ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
- ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
- if (*aia == NULL)
- err(1, NULL);
rc = 1;
out:
@@
