Movatterモバイル変換

bedevere-appbot added the awaiting core review label

bedevere-appbot mentioned this pull request

UBSan: Calling a function through pointer to incorrect function type is undefined behavior#111178

Closed

Copy link

MemberAuthor

vstinner commentedMar 24, 2025

Fix the following error:

$ ./python -m test -v test_socket == CPython 3.14.0a6+ (heads/main:4596666a9f9, Mar 24 2025, 14:50:29) [Clang 19.1.7 (Fedora 19.1.7-2.fc41)]== Linux-6.13.4-200.fc41.x86_64-x86_64-with-glibc2.40 little-endian== Python build: release UBSAN(...)0:00:00 load avg: 3.93 [1/1] test_sockettestBindBrEdrL2capSocket (test.test_socket.BasicBluetoothTest.testBindBrEdrL2capSocket) ...Python/getargs.c:705:13: runtime error: store to misaligned address 0x7ffc1837d0f2 for type 'int', which requires 4 byte alignment0x7ffc1837d0f2: note: pointer points here 00 00  1f 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  d0 d0 37 18 fc 7f 00 00  90 a6 90 01 00 00              ^ SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Python/getargs.c:705:13

Copy link

MemberAuthor

vstinner commentedMar 24, 2025

@picnixz: This change looks like a bugfix, do you think that it should be backported to 3.12 and 3.13 branches?

picnixz reviewed

Copy link

Member

picnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

AreBTPROTO_RFCOMM andBTPROTO_HCI for non-NetBSD below also suffering from the same issue? And yes, it is a bugfix IMO even if the function is internal.

Modules/socketmodule.c OutdatedShow resolvedHide resolved

vstinner added2 commits

March 24, 2025 15:28

Simplify code

1aebf4c

Initialize cid to 0

9eb932c

Copy link

MemberAuthor

vstinner commentedMar 24, 2025

Are BTPROTO_RFCOMM and BTPROTO_HCI for non-NetBSD below also suffering from the same issue?

I didn't get UBsan warning/error on this code, so I prefer to leave it unchanged.

And yes, it is a bugfix IMO even if the function is internal.

This function is called by sock.bind(), sock.connect() and sock.sendto() for example.

Copy link

Member

picnixz commentedMar 24, 2025•
edited
Loading

I didn't get UBsan warning/error on this code, so I prefer to leave it unchanged.

At compile time I don't think there will be but at runtime it might be because the tests are not covering the calls

This function is called by sock.bind(), sock.connect() and sock.sendto() for example.

By internal I meant it's not documented (but yes it's called by many other functions)

vstinner added needs backport to 3.12

only security fixes

needs backport to 3.13bugs and security fixes labels

https://man.archlinux.org/man/l2cap.7.en
https://man.freebsd.org/cgi/man.cgi?query=ng_btsocket
https://man.netbsd.org/bluetooth.4

Copy link

MemberAuthor

vstinner commentedMar 24, 2025

At compile time I don't think there will be but at runtime it might be because the tests are not covering the calls

I added abort() in BTPROTO_RFCOMM and BTPROTO_HCI code paths and ran test_socket. Sadly, test_socket pass with my change, which means that these two code paths are not tested by test_socket.

If it's not tested, I don't feel comfortable to modify these code paths.

Copy link

MemberAuthor

vstinner commentedMar 24, 2025

On Linux, thesockaddr_l2 structure is defined as:

structsockaddr_l2 {sa_family_tl2_family;unsigned shortl2_psm;bdaddr_tl2_bdaddr;unsigned shortl2_cid;uint8_tl2_bdaddr_type;};

l2_psm andl2_cid members are unsigned. So I'm not sure if my change is correct.

cc@serhiy-storchaka

Copy link

Member

serhiy-storchaka commentedMar 24, 2025

On common platformsint covers ranges ofunsigned short, so the correct code should work. But will not there be a compiler warning whenint is implicitly converted tounsigned short? So maybe useunsigned short forpsm? In any case, this code is Linux-only, because on FreeBSD and NetBSD there are nocid andbdaddr_type fields.

I afraid that there is a similar error for BTPROTO_RFCOMM: on Linux, FreeBSD and NetBSD thechannel field has typeuint8_t, notint.

vstinner added2 commits

March 28, 2025 11:20

Use 'unsigned short' type

533a478

Fix also BTPROTO_RFCOMM and BTPROTO_HCI code path.

Use 'unsigned char'

9968c1e

Copy link

MemberAuthor

vstinner commentedMar 28, 2025

I updated my PR to useunsigned short andunsigned char, and I also updated BTPROTO_RFCOMM and BTPROTO_HCI.

@serhiy-storchaka @picnixz: Please review the updated PR.

Add NEWS entry

a95b22d

serhiy-storchaka reviewed

Mar 28, 2025

Modules/socketmodule.c OutdatedShow resolvedHide resolved

Use 'unsigned long' to parse channel on Windows

18ed09e

serhiy-storchaka reviewed

Mar 29, 2025

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131668%2Fmerge

Modules/socketmodule.c

		#else/* __NetBSD__ \|\| __DragonFly__ */
		_BT_HCI_MEMB(addr,family)=AF_BLUETOOTH;
		if (!PyArg_ParseTuple(args,"i",&_BT_HCI_MEMB(addr,dev))) {
		unsigned shortdev=_BT_HCI_MEMB(addr,dev);

Copy link

Member

serhiy-storchakaMar 29, 2025

Choose a reason for hiding this comment

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

Where is it defined?

Copy link

Member

picnixzApr 1, 2025•
edited
Loading

Choose a reason for hiding this comment

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

There's a bunch ofdefine at the top of the file:

#if (defined(HAVE_BLUETOOTH_H)|| defined(HAVE_BLUETOOTH_BLUETOOTH_H)) \&& !defined(__NetBSD__)&& !defined(__DragonFly__)#defineUSE_BLUETOOTH 1#if defined(__FreeBSD__)...#define_BT_HCI_MEMB(sa,memb) ((sa)->hci_##memb)...#elif defined(__NetBSD__)|| defined(__DragonFly__)// <- unreachable...#define_BT_HCI_MEMB(sa,memb) ((sa)->bt_##memb)#else...#define_BT_HCI_MEMB(sa,memb) ((sa)->hci_##memb)...#endif#endif

But AFAICT, theelif defined(__NetBSD__) || defined(__DragonFly__) is not possible at all since we're still in the big#if where we want!defined(__NetBSD__) && !defined(__DragonFly__). So indeed, it looks like the macro wouldn't be defined here.

I think we should remove!defined(__NetBSD__) && !defined(__DragonFly__)

Copy link

Member

serhiy-storchakaApr 1, 2025

Choose a reason for hiding this comment

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

I know about_BT_HCI_MEMB. I asked on what platform the*_dev member is defined, what its name and the structure name. Its turned out that it ishci_node on FreeBSD and NetBSD.hci_dev should exist on Linux.

serhiy-storchaka added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Mar 29, 2025

Copy link

bedevere-bot commentedMar 29, 2025

🤖 New build scheduled with the buildbot fleet by@serhiy-storchaka for commit18ed09e 🤖

Results will be shown at:

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Mar 29, 2025

Copy link

MemberAuthor

vstinner commentedMar 29, 2025

Oh no, build fails on FreeBSD:

./Modules/socketmodule.c:2116:28: error: incompatible pointer to integer conversion initializing 'unsigned short' with an expression of type 'char[32]' [-Wint-conversion] 2116 |             unsigned short dev = _BT_HCI_MEMB(addr, dev);      |                            ^     ~~~~~~~~~~~~~~~~~~~~~~~./Modules/socketmodule.c:2122:37: error: array type 'char[32]' is not assignable 2122 |             _BT_HCI_MEMB(addr, dev) = dev;      |             ~~~~~~~~~~~~~~~~~~~~~~~ ^

Unrelated failures.

ARM64 Windows PR and ARM64 Windows Non-Debug PR: pythoninfo steps crash with exit code-1073741819.

s390x Fedora Stable Clang PR, s390x RHEL8 PR, s390x RHEL9 PR: test_binary_header() of test_tools.test_msgfmt.CompilationTest failed. I don't know why, but it's unrelated to this change.

Copy link

Member

picnixz commentedMar 29, 2025•
edited
Loading

(There's an open PR fixingtest_binary_header)#131879

Copy link

Member

serhiy-storchaka commentedMar 29, 2025

This is weird, because on FreeBSD thesockaddr_hci structure does not have thehci_dev member, it has thehci_node. Buthci_dev is defined ashci_node in this file.

It is not an integer, it is a null-terminated string (size == 32 == NG_NODESIZ == HCI_DEVNAME_SIZE).

Fix BTPROTO_HCI on FreeBSD

99dcf45

Update BTPROTO_HCI code

c0d7d03

Copy link

MemberAuthor

vstinner commentedApr 1, 2025

You need also to make makesockaddr consisting with the parsing code.

Alright, done.

serhiy-storchaka reviewed

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, except that I am not sure about exception types.

Modules/socketmodule.c OutdatedShow resolvedHide resolved

vstinner added3 commits

April 1, 2025 15:13

Change exception type to ValueError

7178cfd

Remove outdated comments

94f7b6f

Remove useless assignment

7455bf5

The socket address channel is not initialized before callingPyArg_ParseTuple().

vstinnerenabled auto-merge (squash)

April 1, 2025 13:33

vstinner merged commit8cd29c2 intopython:main

42 checks passed

vstinner deleted the ubsan_test_socket2 branch

April 1, 2025 13:54

bedevere-appbot removed the awaiting core review label

Copy link

miss-islington-appbot commentedApr 1, 2025

Thanks@vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

Copy link

miss-islington-appbot commentedApr 1, 2025

Sorry,@vstinner, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 8cd29c2b533e5a1a262238695d05f2a7c44d6455 3.13

miss-islington-appbot assignedvstinner

Copy link

miss-islington-appbot commentedApr 1, 2025

Sorry,@vstinner, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 8cd29c2b533e5a1a262238695d05f2a7c44d6455 3.12

vstinner added a commit to vstinner/cpython that referenced this pull request

pythongh-111178: Fix getsockaddrarg() undefined behavior (python#131668)

ad606fc

Don't pass direct references to sockaddr members since their type maynot match PyArg_ParseTuple() types. Instead, use temporary 'int' and'unsigned char' variables, and update sockaddr members afterwards.On FreeBSD, treat BTPROTO_HCI node name as a bytes string,not as an integer.(cherry picked from commit8cd29c2)

Copy link

bedevere-appbot commentedApr 1, 2025

GH-131977 is a backport of this pull request to the3.13 branch.

bedevere-appbot removed the needs backport to 3.13bugs and security fixes label

vstinner removed the needs backport to 3.12only security fixes label

Copy link

MemberAuthor

vstinner commentedApr 1, 2025

Merged, thanks for reviews.

vstinner added a commit that referenced this pull request

[3.13]gh-111178: Fix getsockaddrarg() undefined behavior (#131668) (#…

c318a03

…131977)gh-111178: Fix getsockaddrarg() undefined behavior (#131668)Don't pass direct references to sockaddr members since their type maynot match PyArg_ParseTuple() types. Instead, use temporary 'int' and'unsigned char' variables, and update sockaddr members afterwards.On FreeBSD, treat BTPROTO_HCI node name as a bytes string,not as an integer.(cherry picked from commit8cd29c2)

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

[3.13]pythongh-111178: Fix getsockaddrarg() undefined behavior (pyth…

a0fefae

…onGH-131668) (pythonGH-131977)pythongh-111178: Fix getsockaddrarg() undefined behavior (pythonGH-131668)Don't pass direct references to sockaddr members since their type maynot match PyArg_ParseTuple() types. Instead, use temporary 'int' and'unsigned char' variables, and update sockaddr members afterwards.On FreeBSD, treat BTPROTO_HCI node name as a bytes string,not as an integer.(cherry picked from commitc318a03)Co-authored-by: Victor Stinner <vstinner@python.org>(cherry picked from commit8cd29c2)

miss-islington mentioned this pull request

[3.12] gh-111178: Fix getsockaddrarg() undefined behavior (GH-131668) (GH-131977)#131979

Merged

serhiy-storchaka mentioned this pull request

HCI Bluetooth socket bind error on an arm crosscompiled environment#67632

Closed

vstinner added a commit that referenced this pull request

[3.12]gh-111178: Fix getsockaddrarg() undefined behavior (GH-131668) (…

afb3f33

…GH-131977) (#131979)[3.13]gh-111178: Fix getsockaddrarg() undefined behavior (GH-131668) (GH-131977)gh-111178: Fix getsockaddrarg() undefined behavior (GH-131668)Don't pass direct references to sockaddr members since their type maynot match PyArg_ParseTuple() types. Instead, use temporary 'int' and'unsigned char' variables, and update sockaddr members afterwards.On FreeBSD, treat BTPROTO_HCI node name as a bytes string,not as an integer.(cherry picked from commitc318a03)Co-authored-by: Victor Stinner <vstinner@python.org>(cherry picked from commit8cd29c2)Co-authored-by: Victor Stinner <vstinner@python.org>

serhiy-storchaka mentioned this pull request