Segfault and what's cylinder "use" used for in the planner?

[Rick Walsh]

On 29 April 2016 at 07:26, Rick Walsh <rickmwalsh at gmail.com> wrote:

>
> On 29 Apr 2016 00:38, "Lubomir I. Ivanov" <neolit123 at gmail.com> wrote:
> >
> > On 28 April 2016 at 11:25, Rick Walsh <rickmwalsh at gmail.com> wrote:
> > > Hi,
> > >
> > > I was just looking at the layout of the planner, and realized that I
> > > couldn't find a use for the use field in the available gasses table.
> > >
> > > We determine whether or not a segment is closed circuit by whether a
> > > setpoint has been set in the dive planner points table.  It doesn't
> look
> > > like we actually use the cylinder use value.
> > >
> > > Can it be removed?
> > >
> > > Furthermore, the only way I could find to set the value is to type in
> the
> > > number (0-7) corresponding to the enum.  There's no drop-down of
> options,
> > > and typing the name of the value doesn't work.  There's also a lovely
> > > segfault when I enter the value 8 (or any value greater than 7).  If I
> enter
> > > a negative number, the text becomes one of the default dive tags.  Yay
> > > memory.  Segfault with value 8.
> > >
> > > (gdb) bt
> > > #0  0x00007ffff0e5db3a in strlen () from /lib64/libc.so.6
> > > #1  0x00007ffff1a097c0 in QByteArray::QByteArray(char const*, int) ()
> from
> > > /lib64/libQt5Core.so.5
> > > #2  0x00000000006d0105 in gettextFromC::trGettext (this=0xcf5d50,
> text=0xd1
> > > <error: Cannot access memory at address 0xd1>)
> > >     at /home/rick/src/subsurface/core/gettextfromc.cpp:7
> > > #3  0x000000000065470a in CylindersModel::data (this=0xec1020,
> index=...,
> > > role=2) at /home/rick/src/subsurface/qt-models/cylindermodel.cpp:138
> >
> > ^ that would be a:
> >
> > cylindermodel.cpp:137:
> >
> > case USE:
> >    ret =
> gettextFromC::instance()->trGettext(cylinderuse_text[cyl->cylinder_use]);
> >
> > where "cyl" is of type "cylinder_t" and "cylinder_use" is of type
> > "enum cylinderuse".
> > hmm, but "enum cylinderuse" doesn't have 7 elements, it has 3 (or
> > NUM_GAS_USE)...
> >
> > as defined in dive.h:54:
> > enum cylinderuse {OC_GAS, DILUENT, OXYGEN, NUM_GAS_USE};
> >
> > are you sure there are 7 elements and if so where are they defined?
>
> Thanks Lubomir.  Yes, there should only be 3 (+1) elements, with values 0
> to 2. But after the cylinderuse elements (0-2), it displayed the
> dive_comp_type elements if values 4-7 were entered. 3 was blank.
>
> >
> > a patch that can fix the potential buffer overflow of N elements is
> attached.
>
> I won't have a chance to test for a few days, but it should be relatively
> easy for someone else to verify the bug, and confirm your fix works.
>

Ok, I found 15 minutes and have tested your patch.  It fixes the problem
for values >2, but doesn't help with negative values, which lead to funny
values on Linux, and I think segfault on Windows (I tried a reasonably
recent 4.5 release I have installed at work today).  I could send a patch
on top of yours, but as it isn't in master yet, it's probably easier if you
modify one line of yours.

diff --git a/qt-models/cylindermodel.cpp b/qt-models/cylindermodel.cpp
index 6e0abf0..350d15a 100644
--- a/qt-models/cylindermodel.cpp
+++ b/qt-models/cylindermodel.cpp
@@ -274,7 +274,7 @@ bool CylindersModel::setData(const QModelIndex &index,
const QVariant &value, in
        case USE:
                if (CHANGED()) {
                        int use = vString.toInt();
-                       if (use > NUM_GAS_USE - 1)
+                       if (use > NUM_GAS_USE - 1 || use < 0)
                                use = 0;
                        cyl->cylinder_use = (enum cylinderuse)use;
                        changed = true;

> but it's probably a good thing to also limit the user on the UI side -
> > i.e. once a bad value is entered that field should be auto-adjusted to
> > a good value.
> >
> Agreed. I think this should be a drop-down list. Or remove the field from
> the table entirely. I couldn't work out if/how it's used at all.
>
> Rick
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20160429/8c53ee06/attachment.html>