python / cpython

The Python programming language
https://www.python.org
Other
63.4k stars 30.36k forks source link

Question on doc of __getattribute__ #122494

Open Kevvvvvvin opened 3 months ago

Kevvvvvvin commented 3 months ago

Documentation

In object.__getattribute__:

For certain sensitive attribute accesses, raises an auditing event object.__getattr__ with arguments obj and name.

Cause getattr is called when the default attribute access fails, I think it should be obj.__getattribute__ for this auditing event.

Linked PRs

picnixz commented 3 months ago

I'm not sure this is the case. I can't find in the C source code the corresponding raised audit. All I can find is about object.__getattr__. Why do you believe it should be object.__getattribute__ instead?

Note that the audit table does not mention that kind of audit: https://docs.python.org/3/library/audit_events.html#audit-events and that object.__getattr__ is used as an audit event whenever you query a sensitive attribute, independently of whether it's via __getattr__ or __getattribute__.

Kevvvvvvin commented 3 months ago

Hi @picnixz , As the doc said, __getattr__ is

Called when the default attribute access fails...

and __getattribute__ is

Called unconditionally to implement attribute accesses for instances of the class.

From my point of view, if you would like to implement such audition function of sensitive attribute accesses, it should be put at the entrance(__getattribute__) rather than successor(__getattr__) only when failure happened, isn't it?

picnixz commented 3 months ago

The event name was probably chosen to be similar to __setattr__ and __delattr__. Those events are mainly useful for hooks and they don't really care whether you have retrieved the object in __getattribute__ or in __getattr__. What matters is that you wanted to access it.

Also, this is a longstanding behaviour and changing the name of the event would be a breaking change.

Damien-Chen commented 3 months ago

Hi @picnixz , Based on the discussion above, there is no need to do such a change on document ?

Kevvvvvvin commented 3 months ago

what I mean is not

changing the name of the event

just the content in doc confuses me. If it should be __getattr__ then

For certain sensitive attribute accesses, raises an auditing event object.getattr with arguments obj and name.

should be put under the __getattr__ section and the the sentence could be like "For certain failed attribute accesses..."

picnixz commented 3 months ago

I don't think it should be put under that section because __getattr__ might not be called at all:

Note that if the attribute is found through the normal mechanism, __getattr__() is not called.

So if you put it in the other section, you might think that the event is not fired if if __getattr__ is not called, which is wrong. For the sensitive attributes such as frame.f_code the event is fired at the level of __getattribute__ but is called object.__getattr__ (maybe I was not clear enough, but this event is 1) fired in built-in objects (hence in C), 2) fired in what you would call __gettattribute__).

@zooba As the author of PEP 578, the rationale of having it called __getattr__ instead of __getattribute__ was just for consistencies of __setattr__/__delattr__ or was there something else?

Kevvvvvvin commented 3 months ago

Thanks for your explanation, I know what you meant well now :). Wait for zooba's reply to see the rationale of calling it __getattr__

zooba commented 3 months ago

Probably just the documentation is in the wrong section. No audit events are raised for attribute access in general, but only in specific cases (like function or generator members, which 99.9% of code should never need to access). These cases are undocumented, so the event can be generic and we can easily add/remove places where it gets raised (as long as they're still attribute accesses) because listeners are ready for any object and any attribute name.

The event is so named because __getattr__ is the operation that's being audited, and the arguments match. The special method __getattribute__ was added later and obviously had to have a different name, but fundamentally, it's getattr, setattr and delattr that are the operations.

Kevvvvvvin commented 2 months ago

Hi @picnixz What do you think of zooba's comment above?

picnixz commented 2 months ago

Mmh. I'm not convinced because specifying the event inside the __getattr__ section and at the same time saying that __getattr__ might not be called at all would make me think that the event might not be called in some cases but the event should be called when you try to access something, not if you manage to access it I think.

On the other hand, I also agree that users could be more confused if it's being mentioned in __getattribute__. Let's ask some docs experts: @AA-Turner @ncoghlan where would you suggest we should mention the object.__getattr__ event?

zooba commented 2 months ago

It's a bit of a weird event, because it's only raised when you try to access certain things. And those things are unspecified.

Honestly, it's all a bit weird because this section of documentation is a bit weird. If there's a better reference doc for attribute access in general (including setattr), then the audit event documentation could move there. It isn't even object.__getattr__ that ever raises the event - it's always overrides of it.