| Message ID | 1395944299-21970-9-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/27/2014 01:17 PM, Ilya Dryomov wrote: > Assert length of osd_state, osd_weight and osd_addr arrays. They > should all have exactly max_osd elements after the call to > osdmap_set_max_osd(). Since this function is allowed to fail, could these conditions lead to returning an error code rather than killing the machine? Your testing incoming data (which you can't necessarily trust), not a fundamental assumption of the code, so a BUG() seems harsh. Checking is absolutely the right thing to do. Switch it to return an error if you can. If you feel BUG() is right, so be it. Either way: Reviewed-by: Alex Elder <elder@linaro.org> > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > net/ceph/osdmap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c > index ec06010657b3..19aca4d3c5dd 100644 > --- a/net/ceph/osdmap.c > +++ b/net/ceph/osdmap.c > @@ -745,19 +745,19 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) > if (err) > goto bad; > > - /* osds */ > + /* osd_state, osd_weight, osd_addrs->client_addr */ > ceph_decode_need(p, end, 3*sizeof(u32) + > map->max_osd*(1 + sizeof(*map->osd_weight) + > sizeof(*map->osd_addr)), e_inval); > > - *p += 4; /* skip length field (should match max) */ > + BUG_ON(ceph_decode_32(p) != map->max_osd); > ceph_decode_copy(p, map->osd_state, map->max_osd); > > - *p += 4; /* skip length field (should match max) */ > + BUG_ON(ceph_decode_32(p) != map->max_osd); > for (i = 0; i < map->max_osd; i++) > map->osd_weight[i] = ceph_decode_32(p); > > - *p += 4; /* skip length field (should match max) */ > + BUG_ON(ceph_decode_32(p) != map->max_osd); > ceph_decode_copy(p, map->osd_addr, map->max_osd*sizeof(*map->osd_addr)); > for (i = 0; i < map->max_osd; i++) > ceph_decode_addr(&map->osd_addr[i]); > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 27, 2014 at 9:30 PM, Alex Elder <elder@ieee.org> wrote: > On 03/27/2014 01:17 PM, Ilya Dryomov wrote: >> Assert length of osd_state, osd_weight and osd_addr arrays. They >> should all have exactly max_osd elements after the call to >> osdmap_set_max_osd(). > > Since this function is allowed to fail, could these > conditions lead to returning an error code rather than > killing the machine? > > Your testing incoming data (which you can't necessarily > trust), not a fundamental assumption of the code, so > a BUG() seems harsh. > > Checking is absolutely the right thing to do. > > Switch it to return an error if you can. If you feel > BUG() is right, so be it. Either way: Changed to returning -EINVAL. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c index ec06010657b3..19aca4d3c5dd 100644 --- a/net/ceph/osdmap.c +++ b/net/ceph/osdmap.c @@ -745,19 +745,19 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) if (err) goto bad; - /* osds */ + /* osd_state, osd_weight, osd_addrs->client_addr */ ceph_decode_need(p, end, 3*sizeof(u32) + map->max_osd*(1 + sizeof(*map->osd_weight) + sizeof(*map->osd_addr)), e_inval); - *p += 4; /* skip length field (should match max) */ + BUG_ON(ceph_decode_32(p) != map->max_osd); ceph_decode_copy(p, map->osd_state, map->max_osd); - *p += 4; /* skip length field (should match max) */ + BUG_ON(ceph_decode_32(p) != map->max_osd); for (i = 0; i < map->max_osd; i++) map->osd_weight[i] = ceph_decode_32(p); - *p += 4; /* skip length field (should match max) */ + BUG_ON(ceph_decode_32(p) != map->max_osd); ceph_decode_copy(p, map->osd_addr, map->max_osd*sizeof(*map->osd_addr)); for (i = 0; i < map->max_osd; i++) ceph_decode_addr(&map->osd_addr[i]);
Assert length of osd_state, osd_weight and osd_addr arrays. They should all have exactly max_osd elements after the call to osdmap_set_max_osd(). Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- net/ceph/osdmap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)