On Fri, Jun 03, 2022 at 09:45:21AM +0900, Namjae Jeon wrote:
> 2022-06-03 9:10 GMT+09:00, Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> > On Fri, Jun 03, 2022 at 08:18:19AM +0900, Namjae Jeon wrote:
> >> 2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> >> > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file
> >> > *fp,
> >> > char *buf, loff_t *pos,
> >> > fp->stream.name,
> >> > fp->stream.size,
> >> > &stream_buf);
> >> > - if ((int)v_len < 0) {
> >> > + if (v_len < 0) {
> >> > pr_err("not found stream in xattr : %zd\n", v_len);
> >> > - err = (int)v_len;
> >> > + err = v_len;
> >> Data type of ssize_t is long. Wouldn't some static checker warn us
> >> that this is a problem?
> >
> > None that I know of.
> >
> > To a human reader, the cast isn't needed because when a function returns
> > ssize_t the negatives can only be error codes in the (-4095)-(-1) range.
> > No other negative sizes make sense.
> >
> > On the other hand, the "if ((int)v_len < 0) {" line really should
> > trigger a static checker because there are times when sizes could be
> > over 2GB. I will write down that I need to create that checker.
> Okay, And there is a similar type casting in ksmbd_vfs_stream_read().
> Is it not necessary to change together?
The motivation for this change was that I was looking for places which
save negative error codes in a positive value. Since the ksmbd_vfs_stream_read()
is using ssize_t then it didn't trigger that warning. But
ksmbd_vfs_stream_read() has some minor issues.
if ((int)v_len <= 0)
This cast is "buggy". Harmless though.
return (int)v_len;
This is fine, but unnecessary.
if (v_len <= *pos) {
count = -EINVAL;
I feel like this should return 0 bytes instead of -EINVAL; It probably
doesn't matter.
I am going to try write a static checker that complains specifically
about casting size_t (not long) to int. I will make it complain about
ssize_t as well, but that's kind of unusual because ssize_t can already
store negatives.
regards,
dan carpenter
