fr_pair_afrom_da() now copies unknown da's

which means that the caller has to free them. So we check all of
the callers of fr_dict_unknown_afrom_fields(), and up them to
free the unknown da's.

For RADIUS, this means allocating a temporary talloc_ctx. That
context is used for allocating all unknown da's.  Then, then
fr_radius_decode_pair() is done, it frees the tmp_ctx.

That pattern means we don't have to explicitly free the da's
in every function, and also that the unknown da's are freed
relatively quickly.

diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c
index 498d154967a51aab8817571dfd86c0778bd2514b..492108cc969fcb67d08e7058151069bab4b3cbd5 100644 (file)
--- a/src/lib/util/pair.c
+++ b/src/lib/util/pair.c
@@ -109,6 +109,17 @@ VALUE_PAIR *fr_pair_afrom_da(TALLOC_CTX *ctx, fr_dict_attr_t const *da)
                return NULL;
        }
 
+       /*
+        *      If we get passed an unknown da, we need to ensure that
+        *      it's parented by "vp".
+        */
+       if (da->flags.is_unknown) {
+               fr_dict_attr_t const *unknown;
+
+               unknown = fr_dict_unknown_acopy(vp, da);
+               da = unknown;
+       }
+
        /*
         *      Use the 'da' to initialize more fields.
         */
@@ -165,7 +176,7 @@ alloc:
                /*
                 *      Ensure that the DA is parented by the VP.
                 */
-               da = fr_dict_unknown_afrom_fields(ctx, fr_dict_root(fr_dict_internal), vendor, attr);
+               da = fr_dict_unknown_afrom_fields(vp, fr_dict_root(fr_dict_internal), vendor, attr);
                if (!da) {
                        talloc_free(vp);
                        return NULL;
@@ -201,6 +212,7 @@ alloc:
 VALUE_PAIR *fr_pair_afrom_child_num(TALLOC_CTX *ctx, fr_dict_attr_t const *parent, unsigned int attr)
 {
        fr_dict_attr_t const *da;
+       VALUE_PAIR *vp;
 
        da = fr_dict_attr_child_by_num(parent, attr);
        if (!da) {
@@ -220,7 +232,9 @@ VALUE_PAIR *fr_pair_afrom_child_num(TALLOC_CTX *ctx, fr_dict_attr_t const *paren
                if (!da) return NULL;
        }
 
-       return fr_pair_afrom_da(ctx, da);
+       vp = fr_pair_afrom_da(ctx, da);
+       fr_dict_unknown_free(&da);
+       return vp;
 }
 
 /** Deserialise a value pair from a string

diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c
index ecc27e7e4e6dd8e0e5f20568652e19b3e57cfbdb..10b37b1871307f42d9bd984803ff31e4906dfb4c 100644 (file)
--- a/src/lib/util/struct.c
+++ b/src/lib/util/struct.c
@@ -41,7 +41,8 @@ VALUE_PAIR *fr_unknown_from_network(TALLOC_CTX *ctx, fr_dict_attr_t const *paren
                                             fr_dict_vendor_num_by_da(parent), parent->attr);
        if (!child) return NULL;
 
-       vp = fr_pair_afrom_da(ctx, child);
+       vp = fr_pair_afrom_da(ctx, child); /* makes a copy of 'child' */
+       fr_dict_unknown_free(&child);
        if (!vp) return NULL;
 
        if (fr_value_box_from_network(vp, &vp->data, vp->da->type, vp->da, data, data_len, true) < 0) {

diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c
index 0d9b1a3db751672fb21eba5dea804e043e4b260f..ea78d655412ce34b3c1650ff146987f2300cca7f 100644 (file)
--- a/src/protocols/radius/base.c
+++ b/src/protocols/radius/base.c
@@ -1057,6 +1057,7 @@ ssize_t   fr_radius_decode(TALLOC_CTX *ctx, uint8_t const *packet, size_t packet_l
        uint8_t const           *attr, *end;
        fr_radius_ctx_t         packet_ctx;
 
+       packet_ctx.tmp_ctx = talloc_init("tmp");
        packet_ctx.secret = secret;
        packet_ctx.vector = original + 4;
 
@@ -1071,20 +1072,28 @@ ssize_t fr_radius_decode(TALLOC_CTX *ctx, uint8_t const *packet, size_t packet_l
         */
        while (attr < end) {
                slen = fr_radius_decode_pair(ctx, &cursor, dict_radius, attr, (end - attr), &packet_ctx);
-               if (slen < 0) return slen;
+               if (slen < 0) {
+                       talloc_free(packet_ctx.tmp_ctx);
+                       return slen;
+               }
 
                /*
                 *      If slen is larger than the room in the packet,
                 *      all kinds of bad things happen.
                 */
-                if (!fr_cond_assert(slen <= (end - attr))) return -1;
+                if (!fr_cond_assert(slen <= (end - attr))) {
+                        talloc_free(packet_ctx.tmp_ctx);
+                        return -1;
+                }
 
                attr += slen;
+               talloc_free_children(packet_ctx.tmp_ctx);
        }
 
        /*
         *      We've parsed the whole packet, return that.
         */
+       talloc_free(packet_ctx.tmp_ctx);
        return packet_len;
 }
 

diff --git a/src/protocols/radius/decode.c b/src/protocols/radius/decode.c
index b2c271722fef0a5c15ca45b0066948cc7f21bb43..9163a82b170c3d005bbfe9e1e0ce095cbba29a12 100644 (file)
--- a/src/protocols/radius/decode.c
+++ b/src/protocols/radius/decode.c
@@ -419,6 +419,7 @@ ssize_t fr_radius_decode_tlv(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t con
        fr_dict_attr_t const    *child;
        VALUE_PAIR              *head = NULL;
        fr_cursor_t             tlv_cursor;
+       fr_radius_ctx_t         *packet_ctx = decoder_ctx;
 
        if (data_len < 3) return -1; /* type, length, value */
 
@@ -435,21 +436,18 @@ ssize_t fr_radius_decode_tlv(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t con
 
                child = fr_dict_attr_child_by_num(parent, p[0]);
                if (!child) {
-                       fr_dict_attr_t const *unknown_child;
-
                        FR_PROTO_TRACE("Failed to find child %u of TLV %s", p[0], parent->name);
 
                        /*
                         *      Build an unknown attr
                         */
-                       unknown_child = fr_dict_unknown_afrom_fields(ctx, parent,
-                                                                    fr_dict_vendor_num_by_da(parent), p[0]);
-                       if (!unknown_child) {
+                       child = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent,
+                                                            fr_dict_vendor_num_by_da(parent), p[0]);
+                       if (!child) {
                        error:
                                fr_pair_list_free(&head);
                                return -1;
                        }
-                       child = unknown_child;
                }
                FR_PROTO_TRACE("decode context changed %s -> %s", parent->name, child->name);
 
@@ -478,6 +476,7 @@ static ssize_t decode_vsa_internal(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict
        unsigned int            attribute;
        ssize_t                 attrlen, my_len;
        fr_dict_attr_t const    *da;
+       fr_radius_ctx_t         *packet_ctx = decoder_ctx;
 
        /*
         *      Parent must be a vendor
@@ -541,7 +540,7 @@ static ssize_t decode_vsa_internal(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict
         *      See if the VSA is known.
         */
        da = fr_dict_attr_child_by_num(parent, attribute);
-       if (!da) da = fr_dict_unknown_afrom_fields(ctx, parent, dv->pen, attribute);
+       if (!da) da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, dv->pen, attribute);
        if (!da) return -1;
        FR_PROTO_TRACE("decode context changed %s -> %s", da->parent->name, da->name);
 
@@ -669,6 +668,7 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons
        uint8_t                 *head, *tail;
        uint8_t const           *attr, *end;
        fr_dict_attr_t const    *da;
+       fr_radius_ctx_t         *packet_ctx = decoder_ctx;
 
        /*
         *      data = VID VID VID VID WiMAX-Attr WiMAX-Len Continuation ...
@@ -686,7 +686,7 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons
        if (((size_t) (data[5] + 4)) != attr_len) return -1;
 
        da = fr_dict_attr_child_by_num(parent, data[4]);
-       if (!da) da = fr_dict_unknown_afrom_fields(ctx, parent, vendor, data[4]);
+       if (!da) da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, data[4]);
        if (!da) return -1;
        FR_PROTO_TRACE("decode context changed %s -> %s", da->parent->name, da->name);
 
@@ -696,7 +696,8 @@ static ssize_t decode_wimax(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t cons
        if ((data[6] & 0x80) == 0) {
                rcode = fr_radius_decode_pair_value(ctx, cursor, dict,
                                                    da, data + 7, data[5] - 3, data[5] - 3, decoder_ctx);
-               if (rcode < 0) return -1;
+               if (rcode < 0) return rcode;
+
                return attr_len;
        }
 
@@ -836,6 +837,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const
        fr_dict_vendor_t        my_dv;
        fr_dict_attr_t const    *vendor_da;
        fr_cursor_t             tlv_cursor;
+       fr_radius_ctx_t         *packet_ctx = decoder_ctx;
 
        /*
         *      Container must be a VSA
@@ -870,7 +872,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const
                        return -1;
                }
 
-               if (fr_dict_unknown_vendor_afrom_num(ctx, &n, parent, vendor) < 0) return -1;
+               if (fr_dict_unknown_vendor_afrom_num(packet_ctx->tmp_ctx, &n, parent, vendor) < 0) return -1;
                vendor_da = n;
 
                /*
@@ -885,14 +887,14 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t const
                dv = &my_dv;
 
                goto create_attrs;
-       } else {
-               /*
-                *      We found an attribute representing the vendor
-                *      so it *MUST* exist in the vendor tree.
-                */
-               dv = fr_dict_vendor_by_num(dict, vendor);
-               if (!fr_cond_assert(dv)) return -1;
        }
+
+       /*
+        *      We found an attribute representing the vendor
+        *      so it *MUST* exist in the vendor tree.
+        */
+       dv = fr_dict_vendor_by_num(dict, vendor);
+       if (!fr_cond_assert(dv)) return -1;
        FR_PROTO_TRACE("decode context %s -> %s", parent->name, vendor_da->name);
 
        /*
@@ -934,7 +936,6 @@ create_attrs:
                if (vsa_len < 0) {
                        fr_strerror_printf("%s: Internal sanity check %d", __FUNCTION__, __LINE__);
                        fr_pair_list_free(&head);
-                       fr_dict_unknown_free(&vendor_da);
                        return -1;
                }
 
@@ -953,7 +954,6 @@ create_attrs:
         *      attribute and first known attribute was cloned
         *      meaning we can now free the unknown vendor.
         */
-       fr_dict_unknown_free(&vendor_da);       /* Only frees unknown vendors */
 
        return total;
 }
@@ -1230,7 +1230,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
                 *      and succeed, instead of failing.  So we don't
                 *      need to handle that case here.
                 */
-               child = fr_dict_unknown_afrom_fields(ctx, parent, 0, p[0]);
+               child = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, 0, p[0]);
                if (!child) goto raw;
 
                /*
@@ -1283,7 +1283,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
                                 *      This can be used later by the encoder to rebuild
                                 *      the attribute header.
                                 */
-                               parent = fr_dict_unknown_afrom_fields(ctx, parent, vendor, p[4]);
+                               parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, p[4]);
                                p += 5;
                                data_len -= 5;
                                break;
@@ -1296,7 +1296,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
                                 *      fr_dict_unknown_afrom_fields will do the right thing
                                 *      and only create the unknown attr.
                                 */
-                               parent = fr_dict_unknown_afrom_fields(ctx, parent, vendor, p[4]);
+                               parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent, vendor, p[4]);
                                p += 5;
                                data_len -= 5;
                                break;
@@ -1362,7 +1362,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
                 *      therefore of type "octets", and will be
                 *      handled below.
                 */
-               parent = fr_dict_unknown_afrom_fields(ctx, parent->parent,
+               parent = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, parent->parent,
                                                      fr_dict_vendor_num_by_da(parent), parent->attr);
                if (!parent) {
                        fr_strerror_printf("%s: Internal sanity check %d", __FUNCTION__, __LINE__);
@@ -1386,10 +1386,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
         *      information, decode the actual p.
         */
        vp = fr_pair_afrom_da(ctx, parent);
-       if (!vp) {
-               if (parent->flags.is_unknown) fr_dict_unknown_free(&parent);
-               return -1;
-       }
+       if (!vp) return -1;
        vp->tag = tag;
 
        switch (parent->type) {
@@ -1521,6 +1518,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co
 {
        ssize_t                 rcode;
        fr_dict_attr_t const    *da;
+       fr_radius_ctx_t         *packet_ctx = decoder_ctx;
 
        if ((data_len < 2) || (data[1] < 2) || (data[1] > data_len)) {
                fr_strerror_printf("%s: Insufficient data", __FUNCTION__);
@@ -1530,7 +1528,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co
        da = fr_dict_attr_child_by_num(fr_dict_root(dict), data[0]);
        if (!da) {
                FR_PROTO_TRACE("Unknown attribute %u", data[0]);
-               da = fr_dict_unknown_afrom_fields(ctx, fr_dict_root(dict), 0, data[0]);
+               da = fr_dict_unknown_afrom_fields(packet_ctx->tmp_ctx, fr_dict_root(dict), 0, data[0]);
        }
        if (!da) return -1;
        FR_PROTO_TRACE("decode context changed %s -> %s",da->parent->name, da->name);
@@ -1541,9 +1539,13 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co
        if (data_len == 2) {
                VALUE_PAIR *vp;
 
-               if (!fr_dict_root(dict)->flags.is_root) return 2;
+               if (!fr_dict_root(dict)->flags.is_root) {
+                       return 2;
+               }
 
-               if (data[0] != FR_CHARGEABLE_USER_IDENTITY) return 2;
+               if (data[0] != FR_CHARGEABLE_USER_IDENTITY) {
+                       return 2;
+               }
 
                /*
                 *      Hacks for CUI.  The WiMAX spec says that it can be
@@ -1556,6 +1558,7 @@ ssize_t fr_radius_decode_pair(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dict_t co
                 */
                vp = fr_pair_afrom_da(ctx, da);
                if (!vp) return -1;
+
                fr_cursor_append(cursor, vp);
                vp->vp_tainted = true;          /* not REALLY necessary, but what the heck */
 
@@ -1601,6 +1604,7 @@ static int decode_test_ctx(void **out, TALLOC_CTX *ctx)
        if (fr_radius_init() < 0) return -1;
 
        test_ctx = talloc_zero(ctx, fr_radius_ctx_t);
+       test_ctx->tmp_ctx = talloc(ctx, uint8_t);
        test_ctx->secret = talloc_strdup(test_ctx, "testing123");
        test_ctx->vector = vector;
        talloc_set_destructor(test_ctx, _test_ctx_free);
@@ -1622,6 +1626,7 @@ static ssize_t fr_radius_decode_proto(void *proto_ctx, uint8_t const *data, size
        rcode = fr_radius_decode(test_ctx, data, packet_len, test_ctx->vector - 4, /* decode adds 4 to this */
                                 test_ctx->secret, talloc_array_length(test_ctx->secret) - 1, &vp);
        fr_pair_list_free(&vp);
+       talloc_free_children(test_ctx->tmp_ctx);
 
        return rcode;
 }

diff --git a/src/protocols/radius/packet.c b/src/protocols/radius/packet.c
index 63bdb7009bf7f16a30cde80c56ae759d31a92679..8d45f7215692d00c4b5a4cb91eeda72e7705dd7a 100644 (file)
--- a/src/protocols/radius/packet.c
+++ b/src/protocols/radius/packet.c
@@ -168,6 +168,8 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original,
                return -1;
        }
 
+       packet_ctx.tmp_ctx = talloc(packet, uint8_t);
+
        /*
         *      Extract attribute-value pairs
         */
@@ -189,6 +191,8 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original,
                 */
                my_len = fr_radius_decode_pair(packet, &cursor, dict_radius, ptr, packet_length, &packet_ctx);
                if (my_len < 0) {
+               fail:
+                       talloc_free(packet_ctx.tmp_ctx);
                        fr_pair_list_free(&head);
                        return -1;
                }
@@ -212,18 +216,18 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original,
                if ((max_attributes > 0) && (num_attributes > max_attributes)) {
                        char host_ipaddr[INET6_ADDRSTRLEN];
 
-                       fr_pair_list_free(&head);
                        fr_strerror_printf("Possible DoS attack from host %s: Too many attributes in request "
                                           "(received %d, max %d are allowed)",
                                           inet_ntop(packet->src_ipaddr.af,
                                                     &packet->src_ipaddr.addr,
                                                     host_ipaddr, sizeof(host_ipaddr)),
                                           num_attributes, max_attributes);
-                       return -1;
+                       goto fail;
                }
 
                ptr += my_len;
                packet_length -= my_len;
+               talloc_free_children(packet_ctx.tmp_ctx);
        }
 
        fr_cursor_init(&out, &packet->vps);
@@ -236,6 +240,7 @@ int fr_radius_packet_decode(RADIUS_PACKET *packet, RADIUS_PACKET *original,
         *      random pool.
         */
        fr_rand_seed(packet->data, RADIUS_HEADER_LENGTH);
+       talloc_free(packet_ctx.tmp_ctx);
 
        return 0;
 }

diff --git a/src/protocols/radius/radius.h b/src/protocols/radius/radius.h
index 27624cc44e40d7ba740f828b74279e3e55032b3d..00b482591a3ede2758a03b30dddbc829f759a28e 100644 (file)
--- a/src/protocols/radius/radius.h
+++ b/src/protocols/radius/radius.h
@@ -124,6 +124,7 @@ int         fr_radius_packet_send(RADIUS_PACKET *packet, RADIUS_PACKET const *original,
 void           _fr_radius_packet_log_hex(fr_log_t const *log, RADIUS_PACKET const *packet, char const *file, int line) CC_HINT(nonnull);
 
 typedef struct {
+       TALLOC_CTX              *tmp_ctx;               //!< for temporary things cleaned up during decoding
        uint8_t const           *vector;                //!< vector for encryption / decryption of data
        char const              *secret;                //!< shared secret.  MUST be talloc'd
        bool                    tunnel_password_zeros;