The other modes aren't as fancy -- they either do 32-bit if aligned, or per-byte if not, though no reason we can't be better here (and the code is different enough being a stream mode vs block mode hat I think we can justify being different here.

I'll try to think of something -- though the usage matches the use by all the other modes and by the crypto_{get,init}_ptrs functions as well (though I agree the naming for all of them leaves something to be desired).

It probably wouldn't hurt, though all of the modes have a variation of this (and probably should also have the relevant checks).  It might be better to have a separate ticket and address all of them instead of piecemeal.

It's hard to tell from all the refactoring, but it seems like this is new code. I don't believe we should punt on correctness of new code even if there are other existing pieces that have similar issues.

No. The new code has the same net effect.  The callers into this ensure if pData == NULL, that ulDataLen == 0, which means 0 bytes will be processed and *pulEncryptedDataLen is set to 0. I'll add something like IMPLY(pData == NULL, ulDataLen == 0) (this is true for all callers) to hopefully make it clearer.

I don't believe they're the same. You've changed the behavior that occurs when pData is NULL and uDataLen is non-zero. Previously this would hit the case that you've changed and return zero. However, in the current code if you pass pData as NULL, but ulDataLen is not NULL, if DEBUG is set, we'll abort (which doesn't seem good given the old behavior) or we'll segfault in ctr_xor.

C_Encrypt() guarantees (pData == NULL && ulDataLen == 0) || (pData != NULL) and C_EncryptUpdate() guarantees pdata != NULL.  The other internal calls always call with a non-NULL value of pData. I could rewrite the IMPLY into VERIFY((pData != NULL) || (pData == NULL && ulDataLen == 0) if that would be better. I think some sort of assertion (VERIFY/IMPLY/etc) should be ok here -- this is an internal function, so the external interfaces should be doing the checking the arguments and returning errors.

Fair enough. If you're confident that this should always be handled by callers and we shouldn't gracefully handle it and instead abort, I'll defer to you. I'll admit it still feels a bit weird, given that we can properly error out if it happens.

This is part of the 'tries to be a block cipher' part of the bug. The original code was buffering input data into AES_BLOCK_LEN sized chunks (but messing up the ctr_mode_final call), and so was using ac_remainder_len. The new CTR code does not buffer or segment data, so it never uses ac_remainder_len. It won't hurt to add assert (I'll go ahead and do that), but add a note that unlike the other cases (where the assert is used to ensure no unprocessed data remains), in this instance it is ensuring that no data was ever buffered between calls to aes_{en,de}crypt_contiguous_blocks