common: fix segfault in public IPv6 addr picking #14124

Merged
merged 1 commit into from Apr 1, 2017

Conversation

Projects
None yet
3 participants
@Fabian-Gruenbichler
Contributor

Fabian-Gruenbichler commented Mar 24, 2017

sockaddr is only 16 bytes big, so declaring net as sockaddr
and then casting to sockaddr_in6 in case of IPv6 cannot
work.

using sockaddr_storage works for both IPv4 and IPv6, and is
used in other code parts as well.

note that the tests did not find this issue as they declared
the bigger structs and casted the references to (sockaddr *)

Signed-off-by: Fabian Grünbichler f.gruenbichler@proxmox.com

@Fabian-Gruenbichler

This comment has been minimized.

Show comment
Hide comment
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Mar 24, 2017

Contributor

@Fabian-Gruenbichler could you add Fixes: http://tracker.ceph.com/issues/19371 right before your Signed-off-by line in your commit message?

Contributor

tchaikov commented Mar 24, 2017

@Fabian-Gruenbichler could you add Fixes: http://tracker.ceph.com/issues/19371 right before your Signed-off-by line in your commit message?

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Mar 24, 2017

Contributor

@wido IIRC, you do deploy ceph in IPv6 environment, don't you? have you even run into an issue like this?

Contributor

tchaikov commented Mar 24, 2017

@wido IIRC, you do deploy ceph in IPv6 environment, don't you? have you even run into an issue like this?

@tchaikov tchaikov requested a review from wido Mar 24, 2017

@Fabian-Gruenbichler

This comment has been minimized.

Show comment
Hide comment
@Fabian-Gruenbichler

Fabian-Gruenbichler Mar 24, 2017

Contributor

@wido @tchaikov note that this only affects newly created monitors when the public network configuration option is set.

We also have some other (unrelated) issue regarding IPv6 and nonces which we have not yet been able to narrow down further..

Contributor

Fabian-Gruenbichler commented Mar 24, 2017

@wido @tchaikov note that this only affects newly created monitors when the public network configuration option is set.

We also have some other (unrelated) issue regarding IPv6 and nonces which we have not yet been able to narrow down further..

@wido

This comment has been minimized.

Show comment
Hide comment
@wido

wido Mar 27, 2017

Member

@tchaikov @Fabian-Gruenbichler, I haven't run in to this. I usually don't deploy a public nor cluster network and keep it to one IP per machine policy.

Member

wido commented Mar 27, 2017

@tchaikov @Fabian-Gruenbichler, I haven't run in to this. I usually don't deploy a public nor cluster network and keep it to one IP per machine policy.

@tchaikov tchaikov added the needs-qa label Mar 27, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Mar 27, 2017

Contributor

@wido thanks for confirming it!

Contributor

tchaikov commented Mar 27, 2017

@wido thanks for confirming it!

@tchaikov tchaikov removed the needs-qa label Mar 27, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Mar 27, 2017

Contributor
44/164 Test  #50: unittest_ipaddr .........................***Exception: Other  0.20 sec

make check failed..

Contributor

tchaikov commented Mar 27, 2017

44/164 Test  #50: unittest_ipaddr .........................***Exception: Other  0.20 sec

make check failed..

unittest_ipaddr does not pass

common: fix segfault in public IPv6 addr picking
sockaddr is only 16 bytes big, so declaring net as sockaddr
and then casting to sockaddr_in6 in case of IPv6 cannot
work.

using sockaddr_storage works for both IPv4 and IPv6, and is
used in other code parts as well.

note that the tests did not find this issue as they declared
the bigger structs and casted the references to (sockaddr *)

Fixes: http://tracker.ceph.com/issues/19371
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
@Fabian-Gruenbichler

This comment has been minimized.

Show comment
Hide comment
@Fabian-Gruenbichler

Fabian-Gruenbichler Mar 28, 2017

Contributor

ugh - sorry for missing that the unit tests don't run automatically on build (and thus not rechecking the test changes before submitting).

obviously casting (struct sockaddr_in *) to (struct sockaddr_storage *) can't work since parse_networks will memset the whole "sockaddr_storage" to 0. so we need to always call parse_networks with an actual sockaddr_storage, and later cast that to whatever we expect (test case) or it contains (actual usage).

TL;DR: unit tests pass now, actual code remains unchanged

Contributor

Fabian-Gruenbichler commented Mar 28, 2017

ugh - sorry for missing that the unit tests don't run automatically on build (and thus not rechecking the test changes before submitting).

obviously casting (struct sockaddr_in *) to (struct sockaddr_storage *) can't work since parse_networks will memset the whole "sockaddr_storage" to 0. so we need to always call parse_networks with an actual sockaddr_storage, and later cast that to whatever we expect (test case) or it contains (actual usage).

TL;DR: unit tests pass now, actual code remains unchanged

@tchaikov

This comment has been minimized.

Show comment
Hide comment

@tchaikov tchaikov merged commit 09c9778 into ceph:master Apr 1, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment