From: Jonathan Nieder Date: Wed, 13 Sep 2017 18:24:31 GMT Subject: Re: [PATCH v2] config: avoid "write_in_full(fd, buf, len) < len" pattern Message-ID: <20170913182431.GE27425@aiede.mtv.corp.google.com> In-Reply-To: <20170913181515.p7u2ouine3ysblqc@sigill.intra.peff.net> Jeff King wrote: > What I missed is that copy_begin and copy_end here are actually size_t > variables, not the pointers. Sorry for the confusion, and here's an > updated version of the patch with this paragraph amended (the patch > itself is identical): Subtle. The world makes more sense now. Thanks for figuring it out. > -- >8 -- > Subject: [PATCH] config: avoid "write_in_full(fd, buf, len) < len" pattern > > The return type of write_in_full() is a signed ssize_t, > because we may return "-1" on failure (even if we succeeded > in writing some bytes). But "len" itself is may be an > unsigned type (the function takes a size_t, but of course we > may have something else in the calling function). So while > it seems like: > > if (write_in_full(fd, buf, len) < len) > die_errno("write error"); > > would trigger on error, it won't if "len" is unsigned. The > compiler sees a signed/unsigned comparison and promotes the > signed value, resulting in (size_t)-1, the highest possible > size_t (or again, whatever type the caller has). This cannot > possibly be smaller than "len", and so the conditional can > never trigger. > > I scoured the code base for cases of this, but it turns out > that these two in git_config_set_multivar_in_file_gently() > are the only ones. Here our "len" is the difference between > two size_t variables, making the result an unsigned size_t. > We can fix this by just checking for a negative return value > directly, as write_in_full() will never return any value > except -1 or the full count. [...] > Reported-by: demerphq > Signed-off-by: Jeff King > --- For what it's worth, Reviewed-by: Jonathan Nieder Thank you. Compilers' signed/unsigned comparison warning can be noisy, but I'm starting to feel it's worth the suppression noise to turn it on when DEVELOPER=1 anyway. What do you think? Is there a way to turn it on selectively for certain functions on the LHS (like read() and write() style functions)? Sincerely, Jonathan