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

dlt-gateway: Fix crash on invalid ip #381

Merged

Conversation

alexmohr
Copy link
Contributor

@alexmohr alexmohr commented May 24, 2022

  • dlt-gateway crashed when an invalid
    ip address was configured as a null pointer
    was used for strdup

  • Add timeout for dlt-client-connect.
    Timeout is hard coded to 1 seconds to prevent blocking
    the dlt-daemon overly long if a timeout is happening
    while creating a gateway connection.

The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0

Alexander Mohr, [email protected], Mercedes-Benz Tech Innovation GmbH, imprint

@alexmohr alexmohr force-pushed the fix-crash-on-invalid-gateway-ip branch from 3dac261 to 91c521e Compare May 31, 2022 11:29
@thanhbnq
Copy link
Collaborator

thanhbnq commented Sep 27, 2022

Hello @alexmohr ,
Thank you for your PR and sorry for late feedback.

Regarding the issue with dlt_client_connet, we also implemented the fix and I would like to get your feedback on #409

About the first issue "dlt-gateway crashed when an invalid ip address was configured as a null pointer was used for strdup",
I am not really sure your fix will help to solve it. I mean in the check dlt_gateway_check_ip(), we already prevent null pointer already.

Best regards,
Thanh

@alexmohr
Copy link
Contributor Author

Regarding the issue with dlt_client_connet, we also implemented the fix and I would like to get your feedback on #409
Looks like a valid alternative to what I'm suggesting here, but personally I find setting a timeout to the socket easier to maintain and read compared to setting / unsetting nonblock mode.

About the first issue "dlt-gateway crashed when an invalid ip address was configured as a null pointer was used for strdup", I am not really sure your fix will help to solve it. I mean in the check dlt_gateway_check_ip(), we already prevent null pointer already.

Yes and no the important change here is

 if (!tmp.ip_address) {
                invalid = 1;
            }

without setting the ip address to invalid the daemon will crash.

@thanhbnq
Copy link
Collaborator

Looks like a valid alternative to what I'm suggesting here, but personally I find setting a timeout to the socket easier to maintain and read compared to setting / unsetting nonblock mode.

In some cases, the connection is in progress of establishment and setting timeout could be not getting the exact result.
So if you are fine with our solution, then please kindly update this PR for dlt-gateway fix. I believe that we can apply your fix straightforwardly.

Thank you.
Thanh

@alexmohr alexmohr force-pushed the fix-crash-on-invalid-gateway-ip branch from 91c521e to 2410418 Compare September 28, 2022 06:29
* dlt-gateway crashed when an invalid
  ip address was configured as a null pointer
  was used for strdup

Signed-off-by: Alexander Mohr <[email protected]>

# !!!!!!!!!!!!!!!!!!!! ADJUST CHANGELOG MESSAGE !!!!!!!!!!!!!!!!
# Changelog: <added | fixed | changed | deprecated |removed |security | performance | other>
@alexmohr alexmohr force-pushed the fix-crash-on-invalid-gateway-ip branch from 2410418 to f93a299 Compare September 28, 2022 06:30
@alexmohr alexmohr changed the title dlt-gateway: Fix crash on invalid ip and add timeout dlt-gateway: Fix crash on invalid ip Sep 28, 2022
@alexmohr
Copy link
Contributor Author

This PR now only contains the fix for invalid gateway addresses and a spelling fix. as #409 supersedes the timeout implementation

Thanks
Alex

@michael-methner michael-methner merged commit a6c42a0 into COVESA:master Sep 28, 2022
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.

3 participants