Re: rpki-client: refactor sbgp_assysnum() and sbgp_ipaddrblk()
On Mon, Sep 25, 2023 at 05:02:06PM +0200, Theo Buehler wrote:
> On Mon, Sep 25, 2023 at 04:43:31PM +0200, Claudio Jeker wrote:
> > On Mon, Sep 25, 2023 at 04:38:48PM +0200, Theo Buehler wrote:
> > > On Mon, Sep 25, 2023 at 02:47:37PM +0200, Claudio Jeker wrote:
> > > > On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> > > > > This is a second chunk split out of the diff mentioned in my previous
> > > > > mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> > > > > sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> > > > > the info from the X509_EXTENSION. This should not change anything, but
> > > > > the logic is a bit tricky.
> > > > >
> > > > > We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> > > > > at the top of the two new sbgp_parse_*.
> > > >
> > > > It looks inded like nthing is changed. The thing I dislike a bit is how
> > > > **as and *asz are updated inside the sbgp_parse_* functions. There is
> > > > return 0 before and after the calloc / recallocarray calls and so it
> > > > depends a lot on the caller to be careful here. The code right now is
> > > > ok.
> > >
> > > Thanks for that clue. I didn't particularly like my diff either. The
> > > below is better, has less churn and should be easier to review. This way
> > > the caller doesn't have to be careful.
> > >
> > > I left the currently existing variables asz and ipsz untouched since it
> > > becomes too confusing. I want to rename asz -> sz and new_asz -> asz in
> > > a follow-up, similarly for ipsz.
> >
> > Indeed much better. OK claudio@
>
> And here's the rename. It is mechanical apart from two lines where I
> fixed the order of the variable declarations and from a line I
> unwrapped.
OK claudio@
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 cert.c
> --- cert.c25 Sep 2023 14:56:20 - 1.116
> +++ cert.c25 Sep 2023 14:58:57 -
> @@ -159,7 +159,7 @@ sbgp_parse_assysnum(const char *fn, cons
> {
> const ASIdOrRanges *aors = NULL;
> struct cert_as *as = NULL;
> - size_t asz, new_asz = 0;
> + size_t asz = 0, sz;
> int i;
>
> assert(*out_as == NULL && *out_asz == 0);
> @@ -178,11 +178,11 @@ sbgp_parse_assysnum(const char *fn, cons
>
> switch (asidentifiers->asnum->type) {
> case ASIdentifierChoice_inherit:
> - asz = 1;
> + sz = 1;
> break;
> case ASIdentifierChoice_asIdsOrRanges:
> aors = asidentifiers->asnum->u.asIdsOrRanges;
> - asz = sk_ASIdOrRange_num(aors);
> + sz = sk_ASIdOrRange_num(aors);
> break;
> default:
> warnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
> @@ -190,21 +190,21 @@ sbgp_parse_assysnum(const char *fn, cons
> goto out;
> }
>
> - if (asz == 0) {
> + if (sz == 0) {
> warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges", fn);
> goto out;
> }
> - if (asz >= MAX_AS_SIZE) {
> + if (sz >= MAX_AS_SIZE) {
> warnx("%s: too many AS number entries: limit %d",
> fn, MAX_AS_SIZE);
> goto out;
> }
> - as = calloc(asz, sizeof(struct cert_as));
> + as = calloc(sz, sizeof(struct cert_as));
> if (as == NULL)
> err(1, NULL);
>
> if (aors == NULL) {
> - if (!sbgp_as_inherit(fn, as, _asz))
> + if (!sbgp_as_inherit(fn, as, ))
> goto out;
> }
>
> @@ -214,11 +214,11 @@ sbgp_parse_assysnum(const char *fn, cons
> aor = sk_ASIdOrRange_value(aors, i);
> switch (aor->type) {
> case ASIdOrRange_id:
> - if (!sbgp_as_id(fn, as, _asz, aor->u.id))
> + if (!sbgp_as_id(fn, as, , aor->u.id))
> goto out;
> break;
> case ASIdOrRange_range:
> - if (!sbgp_as_range(fn, as, _asz, aor->u.range))
> + if (!sbgp_as_range(fn, as, , aor->u.range))
> goto out;
> break;
> default:
> @@ -229,7 +229,7 @@ sbgp_parse_assysnum(const char *fn, cons
> }
>
> *out_as = as;
> - *out_asz = new_asz;
> + *out_asz = asz;
>
> return 1;
>
> @@ -361,7 +361,7 @@ sbgp_parse_ipaddrblk(const char *fn, con
> const IPAddressOrRange *aor;
> enum afi afi;
> struct cert_ip *ips = NULL;
> - size_t ipsz, new_ipsz = 0;
> + size_t ipsz = 0, sz;
> int i, j;
>
> assert(*out_ips == NULL && *out_ipsz ==
Re: rpki-client: refactor sbgp_assysnum() and sbgp_ipaddrblk()
On Mon, Sep 25, 2023 at 04:43:31PM +0200, Claudio Jeker wrote:
> On Mon, Sep 25, 2023 at 04:38:48PM +0200, Theo Buehler wrote:
> > On Mon, Sep 25, 2023 at 02:47:37PM +0200, Claudio Jeker wrote:
> > > On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> > > > This is a second chunk split out of the diff mentioned in my previous
> > > > mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> > > > sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> > > > the info from the X509_EXTENSION. This should not change anything, but
> > > > the logic is a bit tricky.
> > > >
> > > > We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> > > > at the top of the two new sbgp_parse_*.
> > >
> > > It looks inded like nthing is changed. The thing I dislike a bit is how
> > > **as and *asz are updated inside the sbgp_parse_* functions. There is
> > > return 0 before and after the calloc / recallocarray calls and so it
> > > depends a lot on the caller to be careful here. The code right now is ok.
> >
> > Thanks for that clue. I didn't particularly like my diff either. The
> > below is better, has less churn and should be easier to review. This way
> > the caller doesn't have to be careful.
> >
> > I left the currently existing variables asz and ipsz untouched since it
> > becomes too confusing. I want to rename asz -> sz and new_asz -> asz in
> > a follow-up, similarly for ipsz.
>
> Indeed much better. OK claudio@
And here's the rename. It is mechanical apart from two lines where I
fixed the order of the variable declarations and from a line I
unwrapped.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.116
diff -u -p -r1.116 cert.c
--- cert.c 25 Sep 2023 14:56:20 - 1.116
+++ cert.c 25 Sep 2023 14:58:57 -
@@ -159,7 +159,7 @@ sbgp_parse_assysnum(const char *fn, cons
{
const ASIdOrRanges *aors = NULL;
struct cert_as *as = NULL;
- size_t asz, new_asz = 0;
+ size_t asz = 0, sz;
int i;
assert(*out_as == NULL && *out_asz == 0);
@@ -178,11 +178,11 @@ sbgp_parse_assysnum(const char *fn, cons
switch (asidentifiers->asnum->type) {
case ASIdentifierChoice_inherit:
- asz = 1;
+ sz = 1;
break;
case ASIdentifierChoice_asIdsOrRanges:
aors = asidentifiers->asnum->u.asIdsOrRanges;
- asz = sk_ASIdOrRange_num(aors);
+ sz = sk_ASIdOrRange_num(aors);
break;
default:
warnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
@@ -190,21 +190,21 @@ sbgp_parse_assysnum(const char *fn, cons
goto out;
}
- if (asz == 0) {
+ if (sz == 0) {
warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges", fn);
goto out;
}
- if (asz >= MAX_AS_SIZE) {
+ if (sz >= MAX_AS_SIZE) {
warnx("%s: too many AS number entries: limit %d",
fn, MAX_AS_SIZE);
goto out;
}
- as = calloc(asz, sizeof(struct cert_as));
+ as = calloc(sz, sizeof(struct cert_as));
if (as == NULL)
err(1, NULL);
if (aors == NULL) {
- if (!sbgp_as_inherit(fn, as, _asz))
+ if (!sbgp_as_inherit(fn, as, ))
goto out;
}
@@ -214,11 +214,11 @@ sbgp_parse_assysnum(const char *fn, cons
aor = sk_ASIdOrRange_value(aors, i);
switch (aor->type) {
case ASIdOrRange_id:
- if (!sbgp_as_id(fn, as, _asz, aor->u.id))
+ if (!sbgp_as_id(fn, as, , aor->u.id))
goto out;
break;
case ASIdOrRange_range:
- if (!sbgp_as_range(fn, as, _asz, aor->u.range))
+ if (!sbgp_as_range(fn, as, , aor->u.range))
goto out;
break;
default:
@@ -229,7 +229,7 @@ sbgp_parse_assysnum(const char *fn, cons
}
*out_as = as;
- *out_asz = new_asz;
+ *out_asz = asz;
return 1;
@@ -361,7 +361,7 @@ sbgp_parse_ipaddrblk(const char *fn, con
const IPAddressOrRange *aor;
enum afi afi;
struct cert_ip *ips = NULL;
- size_t ipsz, new_ipsz = 0;
+ size_t ipsz = 0, sz;
int i, j;
assert(*out_ips == NULL && *out_ipsz == 0);
@@ -372,27 +372,26 @@ sbgp_parse_ipaddrblk(const char *fn, con
switch (af->ipAddressChoice->type) {
case IPAddressChoice_inherit:
Re: rpki-client: refactor sbgp_assysnum() and sbgp_ipaddrblk()
On Mon, Sep 25, 2023 at 04:38:48PM +0200, Theo Buehler wrote:
> On Mon, Sep 25, 2023 at 02:47:37PM +0200, Claudio Jeker wrote:
> > On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> > > This is a second chunk split out of the diff mentioned in my previous
> > > mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> > > sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> > > the info from the X509_EXTENSION. This should not change anything, but
> > > the logic is a bit tricky.
> > >
> > > We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> > > at the top of the two new sbgp_parse_*.
> >
> > It looks inded like nthing is changed. The thing I dislike a bit is how
> > **as and *asz are updated inside the sbgp_parse_* functions. There is
> > return 0 before and after the calloc / recallocarray calls and so it
> > depends a lot on the caller to be careful here. The code right now is ok.
>
> Thanks for that clue. I didn't particularly like my diff either. The
> below is better, has less churn and should be easier to review. This way
> the caller doesn't have to be careful.
>
> I left the currently existing variables asz and ipsz untouched since it
> becomes too confusing. I want to rename asz -> sz and new_asz -> asz in
> a follow-up, similarly for ipsz.
Indeed much better. OK claudio@
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 cert.c
> --- cert.c12 Sep 2023 09:33:30 - 1.115
> +++ cert.c25 Sep 2023 14:29:56 -
> @@ -153,40 +153,26 @@ sbgp_as_inherit(const char *fn, struct c
> return append_as(fn, ases, asz, );
> }
>
> -/*
> - * Parse RFC 6487 4.8.11 X509v3 extension, with syntax documented in RFC
> - * 3779 starting in section 3.2.
> - * Returns zero on failure, non-zero on success.
> - */
> -static int
> -sbgp_assysnum(struct parse *p, X509_EXTENSION *ext)
> +int
> +sbgp_parse_assysnum(const char *fn, const ASIdentifiers *asidentifiers,
> +struct cert_as **out_as, size_t *out_asz)
> {
> - ASIdentifiers *asidentifiers = NULL;
> const ASIdOrRanges *aors = NULL;
> - size_t asz;
> - int i, rc = 0;
> + struct cert_as *as = NULL;
> + size_t asz, new_asz = 0;
> + int i;
>
> - if (!X509_EXTENSION_get_critical(ext)) {
> - warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> - "extension not critical", p->fn);
> - goto out;
> - }
> -
> - if ((asidentifiers = X509V3_EXT_d2i(ext)) == NULL) {
> - warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> - "failed extension parse", p->fn);
> - goto out;
> - }
> + assert(*out_as == NULL && *out_asz == 0);
>
> if (asidentifiers->rdi != NULL) {
> warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> - "should not have RDI values", p->fn);
> + "should not have RDI values", fn);
> goto out;
> }
>
> if (asidentifiers->asnum == NULL) {
> warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> - "no AS number resource set", p->fn);
> + "no AS number resource set", fn);
> goto out;
> }
>
> @@ -200,26 +186,25 @@ sbgp_assysnum(struct parse *p, X509_EXTE
> break;
> default:
> warnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
> - "unknown type %d", p->fn, asidentifiers->asnum->type);
> + "unknown type %d", fn, asidentifiers->asnum->type);
> goto out;
> }
>
> if (asz == 0) {
> - warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges",
> - p->fn);
> + warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges", fn);
> goto out;
> }
> if (asz >= MAX_AS_SIZE) {
> warnx("%s: too many AS number entries: limit %d",
> - p->fn, MAX_AS_SIZE);
> + fn, MAX_AS_SIZE);
> goto out;
> }
> - p->res->as = calloc(asz, sizeof(struct cert_as));
> - if (p->res->as == NULL)
> + as = calloc(asz, sizeof(struct cert_as));
> + if (as == NULL)
> err(1, NULL);
>
> if (aors == NULL) {
> - if (!sbgp_as_inherit(p->fn, p->res->as, >res->asz))
> + if (!sbgp_as_inherit(fn, as, _asz))
> goto out;
> }
>
> @@ -229,22 +214,58 @@ sbgp_assysnum(struct parse *p, X509_EXTE
> aor = sk_ASIdOrRange_value(aors, i);
> switch (aor->type) {
> case ASIdOrRange_id:
> - if (!sbgp_as_id(p->fn, p->res->as, >res->asz,
> -
Re: rpki-client: refactor sbgp_assysnum() and sbgp_ipaddrblk()
On Mon, Sep 25, 2023 at 02:47:37PM +0200, Claudio Jeker wrote:
> On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> > This is a second chunk split out of the diff mentioned in my previous
> > mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> > sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> > the info from the X509_EXTENSION. This should not change anything, but
> > the logic is a bit tricky.
> >
> > We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> > at the top of the two new sbgp_parse_*.
>
> It looks inded like nthing is changed. The thing I dislike a bit is how
> **as and *asz are updated inside the sbgp_parse_* functions. There is
> return 0 before and after the calloc / recallocarray calls and so it
> depends a lot on the caller to be careful here. The code right now is ok.
Thanks for that clue. I didn't particularly like my diff either. The
below is better, has less churn and should be easier to review. This way
the caller doesn't have to be careful.
I left the currently existing variables asz and ipsz untouched since it
becomes too confusing. I want to rename asz -> sz and new_asz -> asz in
a follow-up, similarly for ipsz.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.115
diff -u -p -r1.115 cert.c
--- cert.c 12 Sep 2023 09:33:30 - 1.115
+++ cert.c 25 Sep 2023 14:29:56 -
@@ -153,40 +153,26 @@ sbgp_as_inherit(const char *fn, struct c
return append_as(fn, ases, asz, );
}
-/*
- * Parse RFC 6487 4.8.11 X509v3 extension, with syntax documented in RFC
- * 3779 starting in section 3.2.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_assysnum(struct parse *p, X509_EXTENSION *ext)
+int
+sbgp_parse_assysnum(const char *fn, const ASIdentifiers *asidentifiers,
+struct cert_as **out_as, size_t *out_asz)
{
- ASIdentifiers *asidentifiers = NULL;
const ASIdOrRanges *aors = NULL;
- size_t asz;
- int i, rc = 0;
+ struct cert_as *as = NULL;
+ size_t asz, new_asz = 0;
+ int i;
- if (!X509_EXTENSION_get_critical(ext)) {
- warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "extension not critical", p->fn);
- goto out;
- }
-
- if ((asidentifiers = X509V3_EXT_d2i(ext)) == NULL) {
- warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "failed extension parse", p->fn);
- goto out;
- }
+ assert(*out_as == NULL && *out_asz == 0);
if (asidentifiers->rdi != NULL) {
warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "should not have RDI values", p->fn);
+ "should not have RDI values", fn);
goto out;
}
if (asidentifiers->asnum == NULL) {
warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
- "no AS number resource set", p->fn);
+ "no AS number resource set", fn);
goto out;
}
@@ -200,26 +186,25 @@ sbgp_assysnum(struct parse *p, X509_EXTE
break;
default:
warnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
- "unknown type %d", p->fn, asidentifiers->asnum->type);
+ "unknown type %d", fn, asidentifiers->asnum->type);
goto out;
}
if (asz == 0) {
- warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges",
- p->fn);
+ warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges", fn);
goto out;
}
if (asz >= MAX_AS_SIZE) {
warnx("%s: too many AS number entries: limit %d",
- p->fn, MAX_AS_SIZE);
+ fn, MAX_AS_SIZE);
goto out;
}
- p->res->as = calloc(asz, sizeof(struct cert_as));
- if (p->res->as == NULL)
+ as = calloc(asz, sizeof(struct cert_as));
+ if (as == NULL)
err(1, NULL);
if (aors == NULL) {
- if (!sbgp_as_inherit(p->fn, p->res->as, >res->asz))
+ if (!sbgp_as_inherit(fn, as, _asz))
goto out;
}
@@ -229,22 +214,58 @@ sbgp_assysnum(struct parse *p, X509_EXTE
aor = sk_ASIdOrRange_value(aors, i);
switch (aor->type) {
case ASIdOrRange_id:
- if (!sbgp_as_id(p->fn, p->res->as, >res->asz,
- aor->u.id))
+ if (!sbgp_as_id(fn, as, _asz, aor->u.id))
goto out;
break;
case
Re: rpki-client: refactor sbgp_assysnum() and sbgp_ipaddrblk()
On Sat, Sep 23, 2023 at 01:23:34PM +0200, Theo Buehler wrote:
> This is a second chunk split out of the diff mentioned in my previous
> mail. It factors the parsing of ASIdentifiers and IPAddrBlocks out of
> sbgp_assysnum() and sbgp_ipaddrblk() and makes the latter only extract
> the info from the X509_EXTENSION. This should not change anything, but
> the logic is a bit tricky.
>
> We could initialize *as and *asz, as well as *ips and *ipsz to NULL/0,
> at the top of the two new sbgp_parse_*.
It looks inded like nthing is changed. The thing I dislike a bit is how
**as and *asz are updated inside the sbgp_parse_* functions. There is
return 0 before and after the calloc / recallocarray calls and so it
depends a lot on the caller to be careful here. The code right now is ok.
One minor nit though:
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 cert.c
> --- cert.c12 Sep 2023 09:33:30 - 1.115
> +++ cert.c23 Sep 2023 11:03:48 -
> +/*
> + * Parse RFC 6487 4.8.11 X509v3 extension, with syntax documented in RFC
> + * 3779 starting in section 3.2.
> + * Returns zero on failure, non-zero on success.
> + */
> +static int
> +sbgp_assysnum(struct parse *p, X509_EXTENSION *ext)
> +{
> + ASIdentifiers *asidentifiers = NULL;
> + int rc = 0;
> +
> + if (!X509_EXTENSION_get_critical(ext)) {
> + warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> + "extension not critical", p->fn);
> + goto out;
> + }
> +
> + if ((asidentifiers = X509V3_EXT_d2i(ext)) == NULL) {
> + warnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> + "failed extension parse", p->fn);
> + goto out;
> + }
> +
> + if (!sbgp_parse_assysnum(p->fn, asidentifiers, >res->as,
> >res->asz))
This line is over 80 chars.
Apart from that OK.
--
:wq Claudio
