>> if (byte_count < DEFAULT_BLK_SZ) { >> empty_rbuf: >>- while (ctx->rand_data_valid < DEFAULT_BLK_SZ) { >>- *ptr = ctx->rand_data[ctx->rand_data_valid]; >>- ptr++; >>- byte_count--; >>- ctx->rand_data_valid++; >>- if (byte_count == 0) >>+ while (ctx->rand_read_pos < DEFAULT_BLK_SZ) { >>+ *ptr++ = ctx->rand_data[ctx->rand_read_pos++]; >>+ if (--byte_count == 0) >> goto done; > I am against such collapsing of constructs into one-liners. It is not > obvious at first sight, which value gets incremented in what order. Such > collapsing was the cause for CVE-2013-4345 as it caused an off-by one. Given that this turns out to the the exact code where that happened, I can see the sensitivity of the matter! But I disagree in this case, and it took me a while to figure out how to phrase it. It's a code style issue, which means that it doesn't have a clear technical answer. It's more of a "voting on what people think is clearer" thing. In the case of "if (--byte_count)" issue, that's not something I feel very strongly about. But in the case of "*ptr++ = ctx->rand_data[ctx->rand_read_pos++]", it's the opposite. While I'm not going to pick a fight over it, my opinion is that this form is clearly better. There are two advantages to the code in this form: 1. "*dst++ = *src++" is a C idiom, like "for (i = 0; i < N; i++)". It's very easy to recognize and understand. A more broken-up form less obviously does all the necessary accounting. 2. The original bug was NOT caused by using side effects. Consider the bug fix: --- a/crypto/ansi_cprng.c +++ b/crypto/ansi_cprng.c @@ -230,11 +230,11 @@ remainder: */ if (byte_count < DEFAULT_BLK_SZ) { empty_rbuf: - for (; ctx->rand_data_valid < DEFAULT_BLK_SZ; - ctx->rand_data_valid++) { + while (ctx->rand_data_valid < DEFAULT_BLK_SZ) { *ptr = ctx->rand_data[ctx->rand_data_valid]; ptr++; byte_count--; + ctx->rand_data_valid++; if (byte_count == 0) goto done; } The problem was the *separation* of the copy and the associated increment. In some situations, one was done and not the other, leading to data duplication. The fix was to move the increment of ctx->rand_data_valid to before the "if (byte_count == 0)" loop exit test. However, putting the increment in the same line as the copy reduces the separation that caused the original bug, and makes it *more* clear that the parts always happen together. This, fundamentally, is the reason that I actually find it preferable: it's conceptually "one operation" that should always have all of its parts done together, and putting it on one line makes that easier for the reader to recognize. The problem with the original was putting it in the for(), not using side effect expressions. The above logic doesn't apply to "if (--byte_count)", which is why I don't care about it nearly as much. All that said, there are two significant remaining points: 3. this patch claims it's just a variable rename; perhaps it should stick to that, and 4. patch 10/15 totally deletes this code and replaces it with something even simpler, so if that is acceptable this entire discussion may be moot. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html