s52plib::RenderLSLegacy buffer overflow & Application crash

[Guenael]

Hi,

I posted a bug report on CruisersForum following the recommendation of O-Charts.org support, and I re-post here, because it's related to o-charts_pi.

Ref. https://www.cruisersforum.com/forums/f134/opencpn-version-5-8-4-released-276788-12.html#post3875618

---

Hello,

I experience stability issues with OpenCPN and I found many reports in this thread very close to my case.

I will describe here a reproducible case, a bit of context and some notes, but my main question is about the logging. I wanted to enable extra logging to try to investigate this issue by myself.

I tried to set DebugOpenGL=1 but I didn't get any interesting logs before the crash.

I conducted my test on both Linux and Windows. On Linux I also recompiled the binaries directly from the sources but it don't change anything. The interesting point is that I got the same issue on both systems (Linux/Windows), and I have a reproducible case at a specific GPS location, using o-chart plugin with Canada Atlantic Coast (oeuSENC-CAac). Coordinate:
51° 05.3621' N, 058° 44.1917' W

When you zoom in at this specific spot, the application crashes (again, on both Linux and Windows, so I don't think it's related to any OS, library or hardware driver).

I also tried to avoid OpenGL hardware acceleration, by using software GL, but I got the same issue.

The only message I got during the crash is:
Job 1, 'opencpn' terminated by signal SIGSEGV (Address boundary error)

It looks like a buffer allocation error, typical with C/C++.

I started to investigate the core-dump, and a quick analysis pointed to a call on o-charts plugin. The last entry (#0) below shows a call on the RenderLSLegacy function.

Core was generated by `/usr/bin/opencpn'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000727d407d242a in s52plib::RenderLSLegacy(_ObjRazRules*, _Rules*) ()
   from /usr/lib/opencpn/libo-charts_pi.so

Code ref:
https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L4064
(note: s52plib is not a remote repo link, it's a local copy)

There are some issues related to a similar issue here:
OpenCPN/OpenCPN#3437
But I can reproduce this case with or without OpenGL.

The upper function related is: eSENCChart::DCRenderLPB(wxMemoryDC&, PlugIn_ViewPort const&, wxRect*)

Code ref:
https://github.com/bdbcat/o-charts_pi/blob/master/src/eSENCChart.cpp#L6245

Below, a part of the stack-trace.

#0  0x0000727d407d242a in s52plib::RenderLSLegacy(_ObjRazRules*, _Rules*) ()
    at /usr/lib/opencpn/libo-charts_pi.so
#1  0x0000727d407dbbcd in s52plib::DoRenderObject(wxDC*, _ObjRazRules*) ()
    at /usr/lib/opencpn/libo-charts_pi.so
#2  0x0000727d406b8e47 in eSENCChart::DCRenderLPB(wxMemoryDC&, PlugIn_ViewPort const&, wxRect*) () at /usr/lib/opencpn/libo-charts_pi.so
#3  0x0000727d406b93a2 in eSENCChart::DCRenderRect(wxMemoryDC&, PlugIn_ViewPort const&, wxRect*) () at /usr/lib/opencpn/libo-charts_pi.so
#4  0x0000727d406d500f in eSENCChart::DoRenderViewOnDC(wxMemoryDC&, PlugIn_ViewPort const&, bool) () at /usr/lib/opencpn/libo-charts_pi.so
#5  0x0000727d406d5db1 in eSENCChart::RenderRegionView(PlugIn_ViewPort const&, wxRegion const&) () at /usr/lib/opencpn/libo-charts_pi.so
#6  0x0000727d406b4bc9 in eSENCChart::RenderRegionViewOnDCNoText(PlugIn_ViewPort const&, wxRegion const&) () at /usr/lib/opencpn/libo-charts_pi.so
#7  0x000063355427f963 in ChartPlugInWrapper::RenderRegionViewOnDCNoText(wxMemoryDC&, ViewPort const&, OCPNRegion const&) ()
#8  0x00006335542e11b1 in Quilt::DoRenderQuiltRegionViewOnDC(wxMemoryDC&, ViewPort&, OCPNRegion&) ()

Some additional information on my setup:

Hardware (standard PC computer, with dual-boot Linux/Windows)

• CPU Intel i7-5820K, x86_64, 32GB
• GFX card NVidia GeForce RTX 2080
• Dongle used for o-charts (SG-Lock USB Key)

OS (the Linux part)

• ArchLinux (up to date, weekly)
• Linux 6.7.6-arch1-1 #1 SMP PREEMPT_DYNAMIC Fri, 23 Feb 2024 16:31:48 +0000 x86_64 GNU/Linux

OpenCPN version: v5.8.4

• https://github.com/OpenCPN/OpenCPN/releases/tag/Release_5.8.4

o-charts plugin version: v2.0.3.1

• https://github.com/bdbcat/o-charts_pi/releases/tag/2.0.3.1

Note: s52plib is obviously imported from OpenCPN, and I will try to check the diff between this local version and the latest version to ensure this bug was not already fixed.

Good day,
Guenael

[Guenael]

Hi again,

I did some additional tests.

To make sure the issue was related to the legacy render functions, I bypassed these 2 functions below with a simple return 1; (again, only for debugging purposes...) and the application no longer crash on the previous case (same GPS coordinates used).

diff --git a/libs/s52plib/src/s52plib.cpp b/libs/s52plib/src/s52plib.cpp
index 544475e..a8f7a3a 100755
--- a/libs/s52plib/src/s52plib.cpp
+++ b/libs/s52plib/src/s52plib.cpp
@@ -4062,6 +4062,8 @@ int s52plib::RenderLS(ObjRazRules *rzRules, Rules *rules) {
 
 // Line Simple Style
 int s52plib::RenderLSLegacy(ObjRazRules *rzRules, Rules *rules) {
+  return 1;
+
   if (!rzRules->obj->m_chart_context->chart)
     return RenderLSPlugIn(rzRules, rules);
 
@@ -4941,6 +4943,8 @@ int s52plib::reduceLOD(double LOD_meters, int nPoints, double *source,
 
 // Line Complex
 int s52plib::RenderLCLegacy(ObjRazRules *rzRules, Rules *rules) {
+  return 1;
+
   if (!rzRules->obj->m_chart_context->chart)
     return RenderLCPlugIn(rzRules, rules);

References:

• int s52plib::RenderLSLegacy(ObjRazRules *rzRules, Rules *rules) {
  https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L4064

• int s52plib::RenderLCLegacy(ObjRazRules *rzRules, Rules *rules) {
  https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L4943

A simple way to pin-point the exact issue is to isolate the code block by block, or use GDB to get some additional information about data structure involved.

Anyway, I would encourage the o-charts team to take a look at this one, because a lot of people are complaining about the stability & crash while zooming on the chart. Ref. https://www.cruisersforum.com/forums/f134/opencpn-version-5-8-4-released-276788-12.html

I hope this will help to solve the issue.
Guenael

[Guenael]

Some additional hints, for this function s52plib::RenderLCPlugIn, the issue come from the allocation bloc:

    //  Allocate some storage for converted points
    wxPoint *ptp = (wxPoint *)malloc(
        (max_points + 2) * sizeof(wxPoint));  // + 2 allows for end nodes
    double *pdp = (double *)malloc(2 * (max_points + 2) * sizeof(double));

Ref: https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L5166

And the related bloc that calculate the required number of elements:

  //  Calculate max malloc size required
  int max_points = 0;
  if (rzRules->obj->m_ls_list_legacy) {
    VE_Element *pedge;
    PI_line_segment_element *ls = rzRules->obj->m_ls_list_legacy;

    while (ls) {
      int nPoints;
      // fetch the first point
      if (ls->type == TYPE_EE) {
        pedge = (VE_Element *)ls->private0;
        nPoints = pedge->nCount;
      } else {
        nPoints = 2;
      }

      max_points += nPoints;

      ls = ls->next;
    }
  }

Ref: https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L5137

By increasing the number of elements, the application doesn't crash. I didn't investigate/understand the logic and all the details, but it's clearly on this side.

I understand that this part of the code doesn't come from the o-chart plugin, but from OpenCPN library directly, and I have a note on this. Instead of using a local copy of the code, a best practice is to use a sub-module referencing this external directory (and you can choose your preferred version, but it remains external and it's more clear).
https://github.com/OpenCPN/OpenCPN/tree/master/libs/s52plib/src

[bdbcat]

Thanks for the detailed analysis. I will dig into this very soon.

[Guenael]

For this function s52plib::RenderLCLegacy, the code block in question is this one below (in the loop):

    else {
      //  Calculate max malloc size required
      nls_max = 0;
      int *index_run_x = rzRules->obj->m_lsindex_array;
      for (int imseg = 0; imseg < rzRules->obj->m_n_lsindex; imseg++) {
        index_run_x++;  // Skip cNode
        //  Get the edge
        unsigned int enode = *index_run_x;
        if (enode) {
          VE_Element *pedge = (*ve_hash)[enode];
          if (pedge) {
            if (pedge->nCount > nls_max) nls_max = pedge->nCount;
          }
        }
        index_run_x += 2;
      }
      rzRules->obj->m_n_edge_max_points =
          nls_max;  // Got it, cache for next time
    }

Ref: https://github.com/bdbcat/o-charts_pi/blob/master/libs/s52plib/src/s52plib.cpp#L4975C1-L4994C1

I can post an issue on the OpenCPN repo if it not your code.

Thanks and have a good day,
Guenael

[bdbcat]

Issue corrected, o-charts_pi v2.0.6.0 now available in master catalog.

Root problem:
The particular cell causing trouble has very high resolution on coastline features, much more than seems reasonable for the scale of 1:22000.
This results in a very large .oesu file (~35 Mbyte). We had an upper limit on .oesu file element sizes, just as good code practice. And this cell exceeded our in-built limits.
Simple solution: boost the limit to accommodate this worst case cell.

Thanks for your help in isolating the trouble.
Please close if no further problem.
Dave

[Guenael]

Great news!
I confirm, problem solved with this new value.
(Version tested : tag: 2.0.6.0, origin/sdt-1, and using git submodule update)

Quick note, the master branch remains on the previous value.

Thanks for this fix and your time!
Guenael