Use memmove for overlapping buffer

[rwmjones]

Coverity found the following test where memcpy is used to copy an overlapping buffer. memmove should be used instead.

Error: BUFFER_SIZE (CWE-474):
libmspack-0.10.1alpha/test/md5.c:281: overlapping_buffer: The source buffer "&ctx->buffer[64]" potentially overlaps with the destination buffer "ctx->buffer", which results in undefined behavior for "memcpy". libmspack-0.10.1alpha/test/md5.c:281: remediation: Use memmove instead of "memcpy".
  279|             md5_process_block (ctx->buffer, 64, ctx);
  280|             left_over -= 64;
  281|->           memcpy (ctx->buffer, &ctx->buffer[64], left_over);
  282|           }
  283|         ctx->buflen = left_over;

[roverflow]

Hey @kyz, just wanted to follow up to see if there are any plans to move this forward? We'd really like to see this fix upstream. Anything blocking this PR merge?

[kyz]

I wanted to check the logic for myself as this looks like a code linter's suggestion. The linter could be right, or not.

This is my reading of the logic:

• ctx->buffer is a 128-byte scratchpad
• md5_process_bytes()'s job is to put any number of bytes, of any alignment, through the MD5 block algorithm. It processes any complete blocks it can, but if there are incomplete remnants after processing full blocks, it needs to make a copy of them for itself and its next invocation (either md5_process_bytes() or md5_finish_ctx()) as it can't get them later
• Code after the "Process available complete blocks" comment processes buffer/len in 64-byte blocks. It stops when len <= 64. If data is "unaligned", it first copies 64 unaligned buffer bytes into &ctx->buffer[0]
• If, after processing as many complete blocks as possible in buffer, len is between 1 and 64, the code after comment "Move remaining bytes in internal buffer" copies those bytes into ctx->buffer for next time. This codepath also supports the case of multiple md5_process_bytes() calls not reaching the 64 byte threshold to process one more block, so it maintains ctx->buflen (which can get up to 63, because if it reaches 64, the top of the function processes the now-completed block)
• There are up to 63 bytes already in ctx->buffer (otherwise it'd be complete block and would'be been processed). There are up to 63 bytes in buffer. It concatenates the two with a memcpy()
• If that now makes more than 64 bytes, we have a block to process. It is processed, and the remaining bytes (if any) from &ctx->buffer[64] onwards are shifted to the start of the buffer, &ctx->buffer[0]
• Based on the definition that ctx->buflen can't be >= 64, and len can't be > 64, it is never the case that the memcpy on line 281 overlaps. To do that, left_over would have to be >= 64. left_over also does not wrap around / underflow, because it is checked to be >= 64 before subtracting 64 from it. left_over will be between 0 and 63 and the memcpy() source and dest will not overlap.

So I don't think the memmove is needed.

Do you agree, or do you think I've missed something?

To add to that, this source file was copied verbatim from glibc and has been in use since 1995. I would've expected to see this issue raised sooner, against glibc itself, if this memcpy() overlapped.

glibc 2.39 removed libcrypt and recommended using libxcrypt, whose md5 function has a different implementation but similar memcpy logic

Neither the libxcrypt MD5 implementation, nor the old glibc one currently in use, are as fast as they can be, and checksumming takes a large portion of the CPU time for cabextract -t, so I am in the market for an alternative md5.c, provided it has a suitable licence, is readable and works as portable C90 code (it can have architecture/compiler specific speedups but the baseline should be it works on any C90 compiler with >=32-bit ints)

[rwmjones]

This seems a reasonable explanation, so we can close this.

Unfortunately Coverity is going to keep moaning about this. You'd be within your rights to ignore that because it's not your problem (it's closed source software after all). But a couple of things that might be done to avoid that would be either: just use memmove anyway, or annotate the line by adding this above the line containing the memcpy:

/* coverity[overlapping_buffer] */