1748 replies to this topic

[captnoord]

  * when not really really necessary we prefer to not touch other's code

 

You could always choose to ignore my patches. You could instead google my nickname

and checkup on my opensource experience.

[mirakels]

I think that part should be...   ...so in fact it was wrong as it is now...

 

 

bool eListboxServiceContent::setCurrent(const eServiceReference &ref)
{
    int index=0;
    for (list::iterator i(m_list.begin()); i != m_list.end(); ++i, ++index)
        if ( *i == ref )
        {
            m_cursor = i;
            m_cursor_number = index;
            if (m_listbox)
            {
                m_listbox->moveSelectionTo(cursorResolve(index));
                return true;
            }
            break;
        }
    return false;
}

eh, that is exactly what the patch from captnoord is doing. So what is the problem? Technically it is not a functional change, at least not a change of the most probable intentional functionality. I.e. it is a bug fix!

[littlesat]

The original suggestions had extra { around the for... Which is not really required, but also a taste what the coder wants or nog....

But still I fully agree with this change....

The other two suggested patches look also fine for me, but possibly one without the changes to the debug and I do not fully understand why 16 plus one character is required.

Edited by littlesat, 31 July 2014 - 23:02.

[pieterg]

Looking at the code, the 'if m_listbox' is just a sanitycheck, so the original code is as intended, only the indentation is wrong.

[littlesat]

If so, than it does not require the break... As the return 1 already breaks it...

[captnoord]

I did the +1 because the char array size suggests it is receiving array amount of characters. When it comes down to strings it iz usefull to have it null terminated. The '%16s' will scan for a 16 character string, which also gives it a null termination. So you need a char buffer[17] for it. Otherwise we will endup with a out of bound write. Which could result in stack corruption.

[Persian Prince]

@PLi team

 

did you add lcn support to your codes : https://github.com/o...fbf30d35384f1a5 ?

 

do we need it ?

[Erik Slagter]

did you add lcn support to your codes : https://github.com/o...fbf30d35384f1a5 ?

Probably, BUT as always, this is probably DVB-T2 functionality, and that's not used here, so we cannot test it. So if you have the opportunity to test, please apply locally and test. If it's okay, we'll probably follow.

[Erik Slagter]

FIxed mem leak because of mismatching allocation and deallocation in 'lib\gdi\picload.cpp'

 

see attached path file

Agree.

 

Committed.

 

Interesting fact: the delete statements were introduced by commit 2fb50a7e7447022319ad0d80349d5fc93b8b20dd "Memory leak and file handle leak fix" by Alex Revetchi <alex.revetchi@gmail.com> which was signed off by Littlesat...

[Erik Slagter]

FIxed possible buffer overflow in 'lib/service/servicefs.cpp'

Committed.

 

Very well spotted, thx!

Edited by Erik Slagter, 1 August 2014 - 10:21.

[Erik Slagter]

The last patch I am going to break up into smaller chunks with appropriate descriptions.

 

diff --git a/lib/service/listboxservice.cpp b/lib/service/listboxservice.cpp
index edd03d7..b2793a2 100644
--- a/lib/service/listboxservice.cpp
+++ b/lib/service/listboxservice.cpp
@@ -88,15 +88,19 @@ bool eListboxServiceContent::setCurrent(const eServiceReference &ref)
{
  int index=0;
  for (list::iterator i(m_list.begin()); i != m_list.end(); ++i, ++index)
+ {
   if ( *i == ref )
   {
    m_cursor = i;
    m_cursor_number = index;
    if (m_listbox)
+   {
     m_listbox->moveSelectionTo(cursorResolve(index));
     return true;
+   }
    break;
   }
+ }
  return false;
}

 

The outer block only has one if statement inside, so it's not necessary to add braces. I think it's better though because it removes cause for confusion. This will be moved to a seperate commit with cosmetics.

 

The inner block, at second glance, I disagree about with pieterg, if you remove the indent, you end with two lines "return" and "break" after each other. This suggests it's not just the indent that's wrong. So applied.

 

@@ -112,7 +116,7 @@ int eListboxServiceContent::getNextBeginningWithChar(char c)
{
// printf("Char: %c\n", c);
  int index=0;
- for (list::iterator i(m_list.begin()); i != m_list.end(); ++i, ++index)
+ for (list::iterator i = m_list.begin(); i != m_list.end(); ++i, ++index)
  {
   std::string text;
   ePtr<iStaticServiceInformation> service_info;

As far as I know this is purely a matter of style preferral. Also AFAIK used style is the only allowed style for "older" C++ standards.

 

while ( idx <= len )
   {
    char cc = text[idx++];
-   if ( cc >= 33 && cc < 127)
+   if ( isprint(cc) )
    {
     if (cc == c)
      return index;

Applied.

 

Well-spotted, again. Missed the include though

 

@@ -465,8 +469,10 @@ int eListboxServiceContent::currentCursorSelectable()
  if (cursorValid())
  {
   /* don't allow markers to be selected, unless we're in edit mode (because we want to provide some method to the user to remove a marker) */
-  if (m_cursor->flags & eServiceReference::isMarker && m_marked.empty()) return 0;
-  return 1;
+  if (m_cursor->flags & eServiceReference::isMarker && m_marked.empty())
+   return 0;
+  else
+   return 1;
  }
  return 0;
}

Applied (cosmetic).

 

@@ -934,12 +940,14 @@ void eListboxServiceContent::paint(gPainter &painter, eWindowStyle &style, const
    // the progress data...
    eRect tmp = eRect(pb_xpos + m_progressbar_border_width, pb_ypos + m_progressbar_border_width, evt_done, m_progressbar_height);
    ePtr<gPixmap> &pixmap = m_pixmaps[picServiceEventProgressbar];
-   if (pixmap) {
+   if (pixmap)
+   {
     painter.clip(tmp);
     painter.blit(pixmap, ePoint(pb_xpos + m_progressbar_border_width, pb_ypos + m_progressbar_border_width), tmp, gPainter::BT_ALPHATEST);
     painter.clippop();
    }
-   else {
+   else
+   {
     if (!selected && m_color_set[serviceEventProgressbarColor])
      painter.setForegroundColor(m_color[serviceEventProgressbarColor]);
     else if (selected && m_color_set[serviceEventProgressbarColorSelected])
@@ -948,13 +956,15 @@ void eListboxServiceContent::paint(gPainter &painter, eWindowStyle &style, const
    }

    // the progressbar border
-   if (!selected)  {
+   if (!selected)
+   {
     if (m_color_set[serviceEventProgressbarBorderColor])
      ProgressbarBorderColor = m_color[serviceEventProgressbarBorderColor];
     else if (m_color_set[eventborderForeground])
      ProgressbarBorderColor = m_color[eventborderForeground];
    }
-   else { /* !selected */
+   else /* !selected */
+   {
     if (m_color_set[serviceEventProgressbarBorderColorSelected])
      ProgressbarBorderColor = m_color[serviceEventProgressbarBorderColorSelected];
     else if (m_color_set[eventborderForegroundSelected])

Not applied.

 

I don't think this is enough a coding style violation to warrant changing. This is what I meant when I mentioned we prefer not to touch other's code if it's not really necessary. Cosmetic changes make it harder to dig into the history of (functional) changes. Also many people prefer this style, so I guess it will show up at much more places in enigma's source.

[Erik Slagter]

AFAIK all patches are processed now. Thanks captnoord for your contributions!

[littlesat]

Erik,

That last one is approx my code... I prefer to change it by always put the { on the next line and not at the end of the line.... Can you change this for me?

[SpaceRat]

else {

looks totally confusing to me.

It's either

if {
} else {
}

(my preference)

or

if
{
}
else
{
}

but not a mixture of } in its own line and { on same line ... never seen that anywhere before.

[Erik Slagter]

Both agree with, BUT I repeat, don't change someone else's code if it's not really necessary. If only for git history's sake.

[captnoord]

Thanks Eric for applying my patches.

[Erik Slagter]

Thanks Eric for applying my patches.

That's not a good way to make friends

[theparasol]

Thanks "Erik" & "Kapdnoort"

[captnoord]

fixed possible null pointer dereference in 'lib/gui/ewidget.cpp'

Attached Files

•  fixed-possible-null-pointer-dereference.patch   581bytes   15 downloads

[SpaceRat]

I really, really appreciate to see someone cleaning up.