Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed crash when data_length equals 0 #1210

Closed
wants to merge 1 commit into from
Closed

Fixed crash when data_length equals 0 #1210

wants to merge 1 commit into from

Conversation

gizmocuz
Copy link
Contributor

I experienced a crash on both Windows and Linux systems when the data_length was 0.
This causes 'OnValueRefreshed' to be called with an empty string, resulting in a crash.

Tested the 'Dev' branch with the 'config' folder from the master.
In the setup i disabled downloading/checking for the manufacturer file update

I suspect this has something to do with 'GreenWave PowerNode 6' node, or right after this node is queried

With this patch the library was able to query all nodes and continue

@Fishwaldo
Copy link
Member

There is something seriously wrong here. Can you send logs of the messages that cause the crash....

@gizmocuz
Copy link
Contributor Author

gizmocuz commented Apr 28, 2017

Yes, but only in a few weeks
But you can easily simulate this by setting the size to zero in the code
Bug or no bug, we should make sure this can never happen and check against this
The length is taken out of an received buffer and in my case with one of my hardware types the value is zero

@gizmocuz
Copy link
Contributor Author

Or we should make sure it can accept a zero length but then it should not crash in the following code

@Fishwaldo
Copy link
Member

The problem here is that the message its crashing on is the deviceID and Product ID. If we don't get that we can never load config files or whatnot. I'd like to see the actual message to see if we can work around a 0 length message and still extract that info.

@Fishwaldo
Copy link
Member

ping.... any Logs you can share that show this issue?

@gizmocuz
Copy link
Contributor Author

I'm just back... will do so very soon

@gizmocuz
Copy link
Contributor Author

gizmocuz commented May 21, 2017

  • Checked out a fresh copy of the dev branch
  • I needed to correct the visual studio solution (include a few files, fixed compile errors (plz see the comment i made in the dev merge)
    e2e25ab

Experiencing the same crash, same location, and it seems related to the greenwave powernode 6, but i am attaching the log

Some visual studio screenshots during the crash:

image

image

image

As you see, an empty deviceID is parsed to default_value->OnValueRefreshed
This pull request will check this first.

OZW_Log_21052017.txt

The timeout on Node-1 (the Aeotec controller (old one)) worries me a bit as well

@gizmocuz
Copy link
Contributor Author

Here another log file (with the config from the dev branch) same crash, but this one seems to have an additional log line
OZW_Log.txt

@Fishwaldo
Copy link
Member

The error wasn't anything todo with the lenght - hence why I was skeptical...
it was because we had a duplicate Index for the ValueID's. we were trying to set a string on a INT valueID... :)

That will teach me to not use ENUM's for indexes.... thanks for the report..

@Fishwaldo Fishwaldo closed this May 24, 2017
@gizmocuz
Copy link
Contributor Author

Glad you got it solved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants