-
Notifications
You must be signed in to change notification settings - Fork 3k
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
use calloc for new_interface_server #14342
Conversation
malloc left garbage in the structure, dns IPs were filled with random content
Was this fixed upstream (newer lwip version) ? |
@0xc0170 you're right, this is in the lwIP core. As far as I can see, Mbed uses already the most recent lwIP version 2.1.2 from 2018. |
It might be worth creating a bug report to lwipstack or as you wrote fix it on our side (did we miss it and we should take care of it?). @ARMmbed/mbed-os-connectivity will know better |
the list of issues in lwIP has grown since 2018, looks like there is no real progress at the moment, or the intervalls for updates are really long. I'm not sure about the priority of this issue, the 2nd DNS is a random value and it should happen very very seldom that you hit a fake DNS for your address resolution. edit: |
This pull request has automatically been marked as stale because it has had no recent activity. @geky, @ARMmbed/mbed-os-maintainers, @ARMmbed/mbed-os-connectivity, please complete review of the changes to move the PR forward. Thank you for your contributions. |
Hello, in order to reduce the risk of regression, it would be better to keep It may be a lucky coincidence that 0 happens to be what we want. Future versions of LWIP may change the value of that flag and silently break again what is fixed by this PR. |
With mem_alloc, the whole structure gets random garbage. Filling the fields with valid values is ok also, but when the structure is extended you will have again garbage and you have to touch this code again. and again, this is not lwip code, it is a Mbed extension. |
Indeed, common good practices are also to provide an initializer, either as a macro, a constant object or a function that does that for us. I had a glance at the upstream LWIP and it seems like either this is some legacy code that has been removed or something specific to mbed's fork of lwip. Also, since this struct is never used outside of that very specific file, I would recommend move the struct's definition from the header to the c file it is used and provide a proper initialized to be later used in the related function. |
use ip_addr function to initialize dns_add_interface_server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note: I'm not part of @ARMmbed/mbed-os-connectivity so my approval only engages myself.
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
malloc left garbage in the structure, dns IPs were filled with random content
Summary of changes
PR for #14292
When creating a new dns server interfaces, the server list did contain garbage because of mem_malloc function. Replacing by mem_calloc() initialises with zeros which is IP_ADDR_ANY.
Impact of changes
bugfix
Migration actions required
Documentation
Pull request type
Test results
Test program is given in #14292
Reviewers
@geky