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

[John H. <jo...@gl...>]

Hi Jim,

Sorry for the hasty commit. I'm not used to McSeem's C-style memory
management.

I was looking at the examples and I found that many of them don't build or
run anymore. Some examples download additional files from antigrain.com,
which is no more. The Makefiles literally have make rules like:

1.sdf:
    @echo "Required file missing!!"
    @echo "Please download http://www.antigrain.com/1.sdf to /examples/sdl/"
    @echo "Attempting to retrieve file: (will fail if curl -O  is not
installed)"
    curl -O  http://www.antigrain.com/1.sdf


All but three of the required files are already present in the examples/art
directory. I'm preparing a commit that adds the remaining three files and
changes the Makefiles and Windows readme files to use the new location.

-- john

On Wed, Feb 10, 2021 at 6: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.
>
> 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
>>
> _______________________________________________
> Vector-agg-general mailing list
> Vec...@li...
> https://lists.sourceforge.net/lists/listinfo/vector-agg-general
>