Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 13, 2017, 17:53 UTC
Message-ID: <20170913175357.GC27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170913171127.ocnitq6wz57ivhrh@sigill.intra.peff.net>

Jeff King wrote:

Show 9 quoted lines

> We ask to write 41 bytes and make sure that the return value
> is at least 41. This is the same "dangerous" pattern that
> was fixed in the prior commit (wherein a negative return
> value is promoted to unsigned), though it is not dangerous
> here because our "41" is a constant, not an unsigned
> variable.
>
> But we should convert it anyway to avoid modeling a
> dangerous construct.

If the above logic is correct, then I suspect this series does not go far enough. write_in_full() would be one of those APIs that is easy to misuse and difficult to use correctly, and if so we should fix that at the source instead of trying to teach callers not to hold it wrong.

E.g. what would you think of

 1. Introduce a write_fully (sorry, I am bad at names) function
    that returns 0 on success and a coccinelle semantic patch in
    contrib/coccinelle to migrate callers in "make coccicheck":

@@ expression E; expression F; expression G; @@ -write_in_full(E, F, G) < G +write_fully(E, F, G)

 2. Run "make coccicheck" and apply the result.
 3. Remove the write_in_full function.

Does read_in_full need a similar treatment?

Thanks, Jonathan