[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 51f8196d05f0e271358eee0f90fe044b20449fb5 by Victor Stinner in 
branch 'main':
bpo-30459: Use (void) in macros setting variables (GH-28982)
https://github.com/python/cpython/commit/51f8196d05f0e271358eee0f90fe044b20449fb5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +27269
pull_request: https://github.com/python/cpython/pull/28982

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Christian Tismer]

Christian Tismer  added the comment:

Congrats to that change!

--
nosy: +Christian.Tismer

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:

> That's not true; at least ALSA's python bindings use PyTuple_SET_ITEM as a 
> lvalue as well.

alsa-python used PyTuple_SET_ITEM(..., obj) to decide if it should call 
Py_INCREF(obj). This code looks suspicious. PyTuple_SET_ITEM() should not be 
used to set an item to NULL.

It's already fixed:
https://github.com/alsa-project/alsa-python/commit/5ea2f8709b4d091700750661231f8a3ddce0fc7c

IMO it's a good thing that such suspicious code is discovered. The surprising 
part is that it worked previously :-)

Downstream Fedora issue: https://bugzilla.redhat.com/show_bug.cgi?id=1906380 
(CLOSED)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Petr Viktorin]

Petr Viktorin  added the comment:

> The change respects the documentation which always documented the result type 
> as "void".

Then, IMO, the documentation should be fixed.

> My expectation is that apart py-qt4, no project abuse these 3 macros.

That's not true; at least ALSA's python bindings use PyTuple_SET_ITEM as a 
lvalue as well.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:

I checked: commit 0ef96c2b2a291c9d2d9c0ba42bbc1900a21e65f3 is part of Python 
3.10.0a3 (released 3 days ago).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:

> This change goes directly against PEP 387.

The change respects the documentation which always documented the result type 
as "void".

3.10: https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SET_ITEM
3.5: https://docs.python.org/3.5/c-api/tuple.html#c.PyTuple_SET_ITEM
2.7: https://docs.python.org/2.7/c-api/tuple.html#c.PyTuple_SET_ITEM

This change is backward incompatible on purpose: it's to implement the 
documented behavior, and the macro was misused leading to a bug: 
"PyList_SET_ITEM (l, i, obj) < 0" in py-qt4.

I would also prefer a deprecation warning, but I don't see how do detect when a 
macro is abused to write "x = SET();" or "SET() = x;" Maybe a C linter could 
detect that, but I don't know any tool doing that.

The best we can do is to announce the change. That's why I documented in What's 
New in Python 3.10, and not only "hidden" in the Changelog:
https://docs.python.org/dev/whatsnew/3.10.html#id2

My expectation is that apart py-qt4, no project abuse these 3 macros.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Petr Viktorin]

Petr Viktorin  added the comment:

This change goes directly against PEP 387.

--
nosy: +petr.viktorin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

Change by STINNER Victor :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.10 -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:

Thanks Espie Marc for the bug report, it's now fixed in the master branch. IMO 
not only clang users will benefit of a better defined API. For example, it 
should help other Python implementation to implement such API, without the 
weird side effects of a macro.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 0ef96c2b2a291c9d2d9c0ba42bbc1900a21e65f3 by Victor Stinner in 
branch 'master':
bpo-30459: Cast the result of PyCell_SET to void (GH-23654)
https://github.com/python/cpython/commit/0ef96c2b2a291c9d2d9c0ba42bbc1900a21e65f3


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +22523
pull_request: https://github.com/python/cpython/pull/23654

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 556d97f473fa538cef780f84bd29239ecf57d9c5 by Zackery Spytz in 
branch 'master':
bpo-30459: Cast the result of PyList_SET_ITEM() to void (GH-19975)
https://github.com/python/cpython/commit/556d97f473fa538cef780f84bd29239ecf57d9c5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I prefer PR 19975 to PR 23645. It solves the initial issue and is much simpler.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

Espie Marc  added the comment:

On Sat, Dec 05, 2020 at 01:28:33AM +, STINNER Victor wrote:
> 
> STINNER Victor  added the comment:
> 
> I propose to merge my PR 23645 change right now. If it breaks too many C 
> extensions, we still have time before Python 3.10.0 final scheduled at 
> Monday, 2021-10-04, to revert the change.
> 
> 
> "if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)"
> 
> Well, this is obviously a bug in py-qt4 and it's a good thing to get a 
> compiler error on such code. I guess that the intent was to use 
> PyList_SetItem().
> 
> It's also an issue for Python implementation other than CPython which does 
> not behave exactly like CPython C API. I prefer to fix the C API to make it 
> respect its own documentation (return "void").
> 
> 
> Espie Marc: "Well, there is not going to be a lot of breakage. This problem 
> is the only instance I've encountered in the full OpenBSD ports tree."
> 
> Oh, thanks for providing this very useful feedback! That's very promising.
> 
> Did you report the issue to py-qt4 upstream? If yes, can you please copy here 
> the link to your report?

I'm sorry, it's been so long ago, I can't remember.

I've been dealing with 10s of other bugs since.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

STINNER Victor  added the comment:

I propose to merge my PR 23645 change right now. If it breaks too many C 
extensions, we still have time before Python 3.10.0 final scheduled at Monday, 
2021-10-04, to revert the change.


"if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)"

Well, this is obviously a bug in py-qt4 and it's a good thing to get a compiler 
error on such code. I guess that the intent was to use PyList_SetItem().

It's also an issue for Python implementation other than CPython which does not 
behave exactly like CPython C API. I prefer to fix the C API to make it respect 
its own documentation (return "void").


Espie Marc: "Well, there is not going to be a lot of breakage. This problem is 
the only instance I've encountered in the full OpenBSD ports tree."

Oh, thanks for providing this very useful feedback! That's very promising.

Did you report the issue to py-qt4 upstream? If yes, can you please copy here 
the link to your report?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +22513
pull_request: https://github.com/python/cpython/pull/23645

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Zackery Spytz]

Change by Zackery Spytz :


--
versions: +Python 3.9 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Zackery Spytz]

Change by Zackery Spytz :


--
keywords: +patch
nosy: +ZackerySpytz
nosy_count: 5.0 -> 6.0
pull_requests: +19291
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/19975

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

Espie Marc added the comment:

it's still 100% safe as a macro since each parameter is not used more than 
once. only the return type is an issue.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Raymond Hettinger]

Raymond Hettinger added the comment:

I prefer to leave this as macro rather than assuming the compiler will heed the 
inline hint.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Stefan Krah]

Stefan Krah added the comment:

I guess since it's documented, it would be ok to change for 3.7.  BTW, we also 
allow "static inline" now in headers, so perhaps we could make it a function.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

Espie Marc added the comment:

Note that the API is fully documented for returning void... not anything else.

"No basis" right. We're taling 1 pieces of software. a lot of what is 
actually used in the world.

I'm very surprised, considering python has routinely done "spring cleanup" by 
breaking fairly old apis.
 
If this breaks, people will fix their code, seriously.

In most places, we would rather have undocumented, unportable code, break 
*cleanly*, rather than rely on a fuzzy behavior that could possibly change at 
any moment, and that was never documented as doing anything.


Or is there some kind of mystique that, because this is low-level C 
implementation, somehow, python programmers are not going to be able to cope 
with the internals ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Raymond Hettinger]

Raymond Hettinger added the comment:

> Well, there is not going to be a lot of breakage. This 
> problem is the only instance I've encountered in the 
> full OpenBSD ports tree.

That is no basis for knowing what the entire rest of the world has done with 
done with this API (perhaps valid working code using chained assignments).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

Espie Marc added the comment:

yep, casting to (void) would be safer indeed. didn't think of that one ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I think it would be safer to just cast the result of the expression to void. 
This decreases the chance of breaking valid code, that for example uses 
PyList_SET_ITEM() in a comma expression.

--
nosy: +haypo, serhiy.storchaka
versions: +Python 3.7 -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

Espie Marc added the comment:

Well, there is not going to be a lot of breakage. This problem is the only 
instance I've encountered in the full OpenBSD ports tree.

I thought python was supposed to be a clean language, and didn't shy away from 
removing stuff/tweaking stuff to achieve that goal ?...

The py-kde4 error was deadly. I'm lucky clang finally caught it, but I'd rather 
this kind of stuff just not compile.

I think we're in a world where *correctness* is finally beginning to matter a 
bit more than *compatibility forever whatever the cost*.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Raymond Hettinger]

Raymond Hettinger added the comment:

The docs do make the claim of returning void: 
https://docs.python.org/3/c-api/list.html#c.PyList_SET_ITEM

However, I think we should change the docs rather than changing the macro.  
This macro is very old and very widely used.  Changing it is likely to break 
existing code which may rely on the current behavior.

Also, I don't want to encourage code like what you saw in py-qt4.  It is very 
indirect about what it is trying to express.

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue30459] PyList_SET_ITEM could be safer

[Espie Marc]

New submission from Espie Marc:

Documentation says PyList_SET_ITEM is void, but it lies. The macro is such that 
it yields the actual element being set.

wrapping the macro content in a do {} while (0)  makes sure PyList_SET_ITEM is 
really void, e.g.:
#define PyList_SET_ITEM(op, i, v) do { (((PyListObject *)(op))->ob_item[i] = 
(v)); } while (0)


I just ran into the problem while compiling py-qt4 with clang.
There was some confusion between PyList_SET_ITEM and PyList_SetItem:

if (obj == NULL || PyList_SET_ITEM (l, i, obj) < 0)

g++ didn't catch it (because it doesn't see negative pointers as a problem), 
but clang++ instantly broke.

With PyList_SET_ITEM truly void the problem disappears.

--
components: Interpreter Core
messages: 294362
nosy: espie
priority: normal
severity: normal
status: open
title: PyList_SET_ITEM  could be safer
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com