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

[Jim B. <ji...@ch...>]

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.

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.)

- Jim

On Fri, 5 Feb 2021 at 19:28, Alex Henrie <ale...@gm...> wrote:

> This bug was identified by cppcheck.
> ---
>  agg-2.4/examples/mol_view.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/agg-2.4/examples/mol_view.cpp b/agg-2.4/examples/mol_view.cpp
> index 7813dc3..adba034 100644
> --- a/agg-2.4/examples/mol_view.cpp
> +++ b/agg-2.4/examples/mol_view.cpp
> @@ -205,7 +205,7 @@ bool molecule::read(FILE* fd)
>      if(len > 128) len = 128;
>
>      if(len) memcpy(m_name, buf, len);
> -    m_name[len] = 0;
> +    m_name[len-1] = 0;
>
>      if(!fgets(buf, 510, fd)) return false;
>      if(!fgets(buf, 510, fd)) return false;
> --
> 2.30.0
>