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

PPPinterface: remove faulty address getter check for invalid pointer #11975

Merged

Conversation

michalpasztamobica
Copy link
Contributor

@michalpasztamobica michalpasztamobica commented Nov 28, 2019

Description

Summary of change

(edited after Seppo's remark)
PPPInterface address getter had reversed logic of the check - this is fixed now, by removing the check entirely.

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@kivaisan

@ciarmcom
Copy link
Member

@michalpasztamobica, thank you for your changes.
@kivaisan @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

API does NOT permit a NULL, therefore you should NOT check for null.

This causes broken code to continue running, when it should have crashed.

@michalpasztamobica michalpasztamobica changed the title PPPinterface: address getters check for invalid pointer PPPinterface: remove faulty address getter check for invalid pointer Nov 28, 2019
@michalpasztamobica
Copy link
Contributor Author

@SeppoTakalo , I see your point and I now simply removed the faulty check. Please, review again.

@kivaisan
Copy link
Contributor

kivaisan commented Nov 29, 2019

API does NOT permit a NULL, therefore you should NOT check for null.

Why API was then defined with a pointer parameter? A reference would have been more safe in that case.

@SeppoTakalo
Copy link
Contributor

Yes, reference would be safer, but does not leave any glue that this parameter is going to be modified, also feels more natural for C developers. Therefore using pointers is a dominant in Mbed OS.

@kjbracey
Copy link
Contributor

kjbracey commented Nov 29, 2019

In my experience, references aren't really much "safer", but they do serve as documentation that null isn't acceptable. It's still quite easy to generate null references that crash in exactly the same way.

They do at least block you from literally doing get_ip_address(nullptr), but no-one's really going to do that. The danger is more from allocation failures or similar. Being a reference parameter isn't going to stop you making a mistake like this:

 SockAddr *addrs = (SockAddr *)malloc(16 * sizeof SockAddr);
 for (int i = 0; i < 16; i++) {
      get_ip_address(i, addr[i]);
  }

(And if something like that did happen, I would rather get_ip_address exploded to help me spot and find the flawed calling code).

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 29, 2019

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 29, 2019

Aborted CI, 5.15 jobs need to get first in

@mbed-ci
Copy link

mbed-ci commented Nov 29, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2019

CI restarted

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Dec 3, 2019
@mbed-ci
Copy link

mbed-ci commented Dec 3, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 removed the needs: CI label Dec 4, 2019
@0xc0170 0xc0170 merged commit 34c9cf9 into ARMmbed:master Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants