• 10 dec 2017: forum version update. In case of issues use this topic.
  • 30 nov 2017: pilight moved servers. In case of issues use this topic.
Hello There, Guest! Login Register


Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Long label text causes segfault
#1
In the old days it did not make sense to use very long label text, because the GUI could not display it. Today long label text is split into multiple lines so it will be displayed completely.

However when the daemon broadcasts a device update  and the the length of a text property of that device exceeds 291, a segfault occurs. This seems to be caused by a buffer defined in devices. c that is too small. However, the buffer is defined as char[255], where the segfault occurs at a length of 292.

This also is a problem with my generic_http protocol, when the size of the data returned by a website is bigger than 291, which can easily happen.

I checked what would happen when I made the buffer bigger (char[1024] to begin with) and indeed the label text can be longer then, without segfaulting.

So I think the buffer should be enlarged, but also a check should be added that truncates the text if even longer than the buffer size to prevent the segfault, including a notice in pilight.log.

I will do this first in my own environment to make sure it fixes it without side effects. Afterwards I can make a PR for it, if desired.
 
Reply
#2
Nice catch, it would be best if we could make the buffer a dynamic size depending on the need.
 
Reply
#3
Yes, I will allocate memory for the buffer dynamically.
I see the same buffer (vstring_)  is being (re)used in two places in devices.c.

I guess we can do it in one of two ways:

1. create two separate buffers and allocate memory for each as needed (takes some extra memory)
2. keep using one buffer, allocate some memory when it is initialized and then reallocate as needed

What would you prefer? Or would you do it in yet another way?

Btw. in devices.c there is also a fixed length (255)  buffer called sstring_. As far as I can see this buffer is intended only to hold the device state and 255 bytes seem to be quite a bit overdone for that. We  could make that dynamic too.
 
Reply
#4
I prefer the second one.

In general, static memory allocation is much faster than dynamic. That's why i keep them static when possible and when not wasting too much memory.
 
Reply
#5
I think of doing it this way:

Allocate 255 bytes of memory when the vstring_ buffer is initialized. Then before the buffer is being used, check if it is already big enough and only if that's not the case, reallocate. In most cases 255 will be enough, so no reallocation will be required. 

And I will leave the second (sstring_) buffer static as it is.
 
Reply
#6
Ok
 
Reply
#7
Picking up this thread again.

So the issue is that if you try to let an action update a device string (not only labels) with a long string, segfaults will occur.
We discussed before that the size of the string buffer in devices.c (vstring_) should be made dynamic.

You told that you are now working on porting the actions to lua. In view of that, is it useful to fix the buffersize in devices.c now? Or should I wait until you are ready porting the actions?
 
Reply
#8
The lua actions are done. I will push those before the end of the week. When that's done i need a unitest for the device error.

The new library and actions are pushed to rewrite. Backporting to staging is a bit more work. But you can already see what i'm up to.
 
Reply
#9
Thanks, I got a first impression of the way you have implemented the lua actions.
Do you intend to create an object for http that can be used in actions, similar to the one you created for sendmail?

In order to be able to keep working with the staging branch, I would like to start asap with adding the BLINK and BGCOLOR options to the label action and porting the actions I made myself to lua too.
Would it be safe to do that based on the versions now in the rewrite, or do you expect significant changes before you can backport them to staging?
 
Reply
#10
HTTP will follow soon. Code is already usable in staging.
 
Reply
  


Possibly Related Threads...
Thread Author Replies Views Last Post
  [SOLVED] Triggering generic_switch leads to segfault Ulrich.Arnold 19 906 10-23-2019, 09:03 AM
Last Post: Ulrich.Arnold
  ][solved]Segfault when retrieving big chunked http message Niek 21 5,964 11-29-2018, 03:17 PM
Last Post: curlymo
  http code 301 causes segfault Niek 3 3,355 08-14-2018, 06:57 PM
Last Post: curlymo
  Enhanced lua label action Niek 5 712 07-04-2018, 05:28 PM
Last Post: curlymo

Forum Jump:


Browsing: 1 Guest(s)