lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 14 Oct 2017 09:42:02 +0100 From: Christos Gkekas <chris.gekas@...il.com> To: Gregory Farnum <gfarnum@...hat.com> Cc: "Yan, Zheng" <zyan@...hat.com>, Sage Weil <sage@...hat.com>, Ilya Dryomov <idryomov@...il.com>, ceph-devel <ceph-devel@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] ceph: Delete unused variables in mds_client On 12/10/17 15:45:42 -0700, Gregory Farnum wrote: > On Thu, Oct 12, 2017 at 2:55 PM, Christos Gkekas <chris.gekas@...il.com> wrote: > > Remove variables in mds_client that are set but never used. > > Hmm, it looks like all the values removed here (except err) *are* used > and this patch would break the world. Specifically... > > > > > Signed-off-by: Christos Gkekas <chris.gekas@...il.com> > > --- > > fs/ceph/mds_client.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index f23c820..cadf9b6 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -3797,11 +3797,8 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) > > void *p = msg->front.iov_base; > > void *end = p + msg->front.iov_len; > > u32 epoch; > > - u32 map_len; > > u32 num_fs; > > u32 mount_fscid = (u32)-1; > > - u8 struct_v, struct_cv; > > - int err = -EINVAL; > > > > ceph_decode_need(&p, end, sizeof(u32), bad); > > epoch = ceph_decode_32(&p); > > @@ -3809,10 +3806,6 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) > > dout("handle_fsmap epoch %u\n", epoch); > > > > ceph_decode_need(&p, end, 2 + sizeof(u32), bad); > > - struct_v = ceph_decode_8(&p); > > - struct_cv = ceph_decode_8(&p); > > - map_len = ceph_decode_32(&p); > > - > > p here is a pointer into a buffer which was sent to us over the > network (msg->front.iov_base, shown at top of patch). ceph_decode is > reading data out of that buffer into these variables and advancing p > for the subsequent readers. If you drop these statements, we start > trying to read values at the wrong offsets, and obviously nothing > makes sense. > > > ceph_decode_need(&p, end, sizeof(u32) * 3, bad); > > p += sizeof(u32) * 2; /* skip epoch and legacy_client_fscid */ > > > > @@ -3820,12 +3813,9 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) > > while (num_fs-- > 0) { > > void *info_p, *info_end; > > u32 info_len; > > - u8 info_v, info_cv; > > u32 fscid, namelen; > > > > ceph_decode_need(&p, end, 2 + sizeof(u32), bad); > > - info_v = ceph_decode_8(&p); > > - info_cv = ceph_decode_8(&p); > > info_len = ceph_decode_32(&p); > > ceph_decode_need(&p, end, info_len, bad); > > info_p = p; > > @@ -3852,7 +3842,6 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) > > 0, true); > > ceph_monc_renew_subs(&fsc->client->monc); > > } else { > > - err = -ENOENT; > > goto err_out; > > Amusingly, this bit does seem to be correct, as the err_out block just > unconditionally sets mdsc->mdsmap_err to -ENOENT (and is only > reachable from this goto). > > > } > > return; > > I assume this patch was generated by some code cleanliness tool, so it > appears to need some work... :) > -Greg Oops, you are absolutely right. GCC is warning about those variables being unused, but it is clearly wrong. They are used and are indeed very important for decoding the input data as you correctly highlighted. Thanks for your time and sorry for sending this across. Will resend another patch just removing 'err'. Christos Gkekas
Powered by blists - more mailing lists