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

[mdns]: Fixed some minor bugs #730

Merged
merged 8 commits into from
Jan 15, 2025

Conversation

david-cermak
Copy link
Collaborator

@david-cermak david-cermak commented Jan 10, 2025

Github issues

  • 9270a1a fix(mdns): Move MDNS_NAME_BUF_LEN to public headers
  • 77532f9 fix(mdns): Cleanup includes in mdns.c
  • 5eddc9c fix(mdns): Allow advertizing service with port==0
  • 980f3d3 fix(mdns): Fixed complier warning if MDNS_MAX_SERVICES==0

Coverity issues

  • a69123d fix(mdns): Fixed potential out-of-bound interface error
  • 80a7b1f fix(mdns): Fixed incorrect error conversion
  • 411dee4 fix(mdns): Fixed potential overflow when allocating txt data

components/mdns/mdns.c Show resolved Hide resolved
components/mdns/include/mdns.h Show resolved Hide resolved
components/mdns/mdns.c Show resolved Hide resolved
components/mdns/mdns.c Show resolved Hide resolved
@zwx1995esp
Copy link
Contributor

zwx1995esp commented Jan 15, 2025

Hi, @david-cermak Another topic: I noticed that mDNS creates a non-recursive mutex and this kind of mutex can only be taken once by one task refer to FreeRTOS:
image

As far as I know, the APIs with the prefix _mdns_xxx are implemented without acquiring and releasing the lock, this kind of APIs is private and only used by mdns task internally. And the APIs mdns_xxx are implemented with the lock acquired and released, this kind of APIs is public and can be accessed by the users(but only the user? Could mdns task call these APIs?). So, currently we can not call any mdns_xxx in mdns task. Is this designed deliberately? If we use a non-recursive mutex, then in mdns task, we can call both mdns_xxx and _mdns_xxx APIs.

Since it's used by public API as maximum length of user buffer

Closes espressif#724
Closes coverity warning: 470092 Overflowed integer argument
Mixing esp_err_t (int) with err_t (uint8_t) from lwip.

Closes coverity isssue: 470139 Overflowed return value
invalid mdns_if was handled for enabling/announcing pcbs,
but not for the consequent browsing

Closes coverity isssue: 470162 Out-of-bounds access
@david-cermak
Copy link
Collaborator Author

Hi, @david-cermak Another topic: I noticed that mDNS creates a non-recursive mutex and this kind of mutex can only be taken once by one task refer to....

Valid point, but won't address this in this PR. I'm going to work more on mdns in the near future though, so I'll reconsider.
the general idea is use non-recursive mutex, unless necessary, which up to this point was not, but that may change in the future.

@david-cermak
Copy link
Collaborator Author

Thanks for the review @euripedesrocha and @zwx1995esp , I'll go ahead and merge this as is as I think I've addressed all the comments. I'll continue in #732 which depends on this one

@david-cermak david-cermak merged commit 9b74256 into espressif:master Jan 15, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants