Re: [AGG] [PATCH] Fix off-by-one error in mol_view example

[Alex H. <ale...@gm...>]

On Wed, Feb 10, 2021 at 7:57 AM Jim Barry via Vector-agg-general
<vec...@li...> wrote:
>
> I'm afraid this is incorrect, and is a good example of why the results of code analysis tools should be examined carefully.
>
> The function trim_cr_lf returns the length of the trimmed string, so "m_name[len] = 0" is the correct way to ensure that m_name is zero terminated.
>
> If changed to [len-1], the last character of the name is truncated. So with the "1.sdf" sample data file, the name is displayed as MFCD0013393 when it should be MFCD00133935. Also, if "len" happened to be zero then "m_name[len-1]" is obviously out of bounds.
>
> However, there are a couple of other problems. The m_name buffer is 128 characters in size, but space is not allowed for the terminating zero when enforcing the maximum string length, resulting in a buffer overflow.

The buffer overflow is the problem that cppcheck identified and I was
trying to fix. I incorrectly assumed that len included the terminating
null character. I apologize for not looking at the code more closely.

> There is also a potential problem in trim_cr_lf, which is that strcpy has undefined behaviour if the source and destination strings overlap.
>
> I have undone the previous change, fixed the length check, and replaced the strcpy call with memmove, which does permit the buffers to overlap. (The trim_cr_lf function always zero-terminates the buffer so that is already taken care of.)

You really didn't have to commit and then revert the incorrect fix
that I originally suggested. But anyway, thank you for resolving the
buffer overflow, and for finding and fixing the bug with strcpy as
well.

-Alex