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

properly initialized and checked Ethernet init callback (fix #890) #897

Closed
wants to merge 1 commit into from

Conversation

maidnl
Copy link
Contributor

@maidnl maidnl commented Jun 17, 2024

fix for potential null pointer access / _initializerCallback not initialized #890
This PR properly initializes _initializerCallback function pointer and checks for its validity before to call it

@@ -5,7 +5,10 @@
int arduino::EthernetClass::begin(uint8_t *mac, unsigned long timeout, unsigned long responseTimeout) {
if (eth_if == nullptr) {
//Q: What is the callback for?
_initializerCallback();
//A: To call a specific function on your system before any initialization is performed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@manchoz
Copy link
Contributor

manchoz commented Jun 17, 2024

@pennam
Copy link
Contributor

pennam commented Jun 17, 2024

@maidnl @pennam see also main...manchoz:ArduinoCore-mbed:rm_initCallback

@manchoz are you aware of this callback being used somewhere?

@maidnl
Copy link
Contributor Author

maidnl commented Jun 17, 2024

Thanks @manchoz, however your PR removes completely the callback and this would brake backward compatibility (in case someone used the callback), so I would prefer to keep the callback and just make it safe.
What do you think @pennam ?

@manchoz
Copy link
Contributor

manchoz commented Jun 17, 2024

@pennam @maidnl, there is no code referring to this callback. AFAIK, it is a leftover of the early stages of the ArduinoCore-mbed. @facchinm WDYT?

@pennam
Copy link
Contributor

pennam commented Jun 17, 2024

isn't already impossible to use the _initializerCallback() ? eth_if would never be a nullptr because is always initialized here:

EthernetInterface *eth_if = &net;

@maidnl
Copy link
Contributor Author

maidnl commented Jun 17, 2024

@pennam, I could be wrong, but here is my understanding:
you have 3 constructors of EthernetClass (1) EthernetClass(EthernetInterface *_if), (2) EthernetClass();, (3) EthernetClass(voidPrtFuncPtr _cb). _initializerCallback() (which is pointer to function) is initialized using constructor (3) and is not related to eth_if. I agree however on the fact that eth_if could not be nullptr ever, but just because you cannot call the constructor EthernetClass(nullptr) because of the conflict between (1) and (3).
This PR is needed in all cases since _initializeCallback is not properly initialized nor there is a check for its validity when is called.

@pennam
Copy link
Contributor

pennam commented Jun 17, 2024

@maidnl i was thinking if it makes any sense to keep the _initializerCallback() in the code:

  • using ctor 3 EthernetClass(voidPrtFuncPtr _cb); the _initializerCallback() is never called because eth_if is always different from nullptr
  • using ctor 2 EthernetClass(); same story
  • using ctor 1 EthernetClass(EthernetInterface *_if) you can teoretically create an Ethernet object with a null eth_if, but you can't configure the _initializerCallback() 😞

@manchoz
Copy link
Contributor

manchoz commented Jun 17, 2024

Let's go ahead and remove it. I don't see any use case for this feature.

@pennam
Copy link
Contributor

pennam commented Jun 18, 2024

👍 let's go for #900

@pennam
Copy link
Contributor

pennam commented Jun 20, 2024

superseded by #901

@pennam pennam closed this Jun 20, 2024
@maidnl maidnl deleted the fix_initializer_callback branch July 3, 2024 08:36
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.

None yet

3 participants