fix(context): bits/endian stop following arch after a second switch - #2764
fix(context): bits/endian stop following arch after a second switch#2764shariqueahmad108-ship-it wants to merge 2 commits into
Conversation
Repro from the issue: set arch to amd64 then back to i386, and bits stays at 64 - assembling anything after that blows up with "Invalid arch/bits combination: i386/64". Root cause is the arch setter's cascade check (`if k not in self._tls`) can't tell the difference between "user explicitly set bits" and "bits only has a value because a previous arch assignment put it there". Went with the fix peace-maker sketched in the issue thread: track which attributes were set directly by the user (a new _tls_explicit dict-stack, mirroring _tls) and only skip the cascade for those. Had to add _tls_explicit to __slots__ too since ContextType doesn't allow arbitrary attributes. Checked context.local()/clear()/Thread inheritance don't leak or lose this tracking across pushes/pops/thread boundaries - all three have manual reproductions plus a new doctest matching the issue's exact repro. Ran the full context.rst doctest suite before and after my change to confirm the same 8 failures exist on both (they're a local macOS environment thing - missing cross-arch binutils and /bin/bash not being an ELF - not anything I touched), and mypy-baseline shows 0 new issues. The os setter has the identical cascade bug for its own defaults (e.g. newline), just never got reported. Left it alone since the issue didn't ask for it and I didn't want to grow this PR - happy to open a separate one if that's wanted. Fixes Gallopsled#2498
|
Heads up on the red Lint check — it's not from this PR. The Looks like Happy to add |
|
The failing
The dependabot PR that only bumps Actions versions hit the identical failure on Aug 1: run 30699220265 ( Since Worth a separate issue for |
|
Can you explain why you are outputing LLM CoT into comments? |
|
Fair question, and yes — I use an AI assistant for a lot of this, including those two comments. That's on me for not saying so upfront, and I can see how the length and formatting read as noise rather than signal. The underlying work is mine to stand behind: I can walk through why Happy to trim the comments down, or to close this and let someone else take #2498 if AI-assisted PRs aren't welcome here. Just tell me which you'd prefer. |
|
Are you an agent or a person? Did you, or the person behind review your code? Can you, or the person behind take the responsiblity of what you write? |
what was the problem bruh |
|
Thank you, this looks cleaner than a separate attribute for every attribute set implicitly. Yes, please include os and newline. The original PR was against stable which does not have those changes yet. But on dev we want the same guards for changing I'll have a closer look at the code later. |
|
Thank you for contributing. What is your opinion on the approach I suggested here?
Have you... seen that PR? Instead of tracking what's set and what is not set, we could make the default an actual default, when nothing on the stack set it, impossible to unset etc. |
Fixes #2498.
Repro from the issue: set
archtoamd64then back toi386, andbitsstays at 64 — assembling anything after that blows up withInvalid arch/bits combination: i386/64. Thearchsetter's cascade check (if k not in self._tls) can't tell "user explicitly set bits" apart from "bits only has a value because a previous arch assignment put it there".Went with the fix @peace-maker sketched out in the issue thread: track which attributes were set directly by the user (a new
_tls_explicitdict-stack, mirroring_tls) and only skip thearch→bits/endiancascade for those. Also had to add_tls_explicitto__slots__, sinceContextTypedoesn't allow arbitrary attributes.What I checked
context.local(),context.clear(), andThreadcontext inheritance all push/pop/reset this new tracking in lockstep with the existing_tls— manually reproduced all three scenarios, since none of pwntools' existing doctests exercise them together with a secondarchswitch.archsetter's docstring.context.rstdoctest suite before and after my change — same 8 pre-existing failures both times (all environmental: my local macOS box is missing the cross-arch binutils packages CI installs, and macOS's/bin/bashisn't an ELF file —TESTING.mdalready says Ubuntu is the expected environment for this).mypy | mypy-baseline filter— 0 new issues.One thing worth flagging
os's setter has the exact same cascade bug for its own defaults (e.g.newline) — same root cause, just never got reported. Left it alone since the issue didn't ask for it and I didn't want to grow this PR unasked; happy to open a separate issue/PR for it if you'd rather have it fixed too.Branch
Opened against
devsince that's where I developed and tested this. I confirmed the identical cascade bug also exists onstable/beta(same code there) — let me know if you'd like this backported and I'll open that PR too, sincedevhas diverged enough fromstable(type hints, Python 2 compat removed) that I didn't want to guess at a port without it being reviewed first.Changelog commit to follow once this has a PR number, per CONTRIBUTING.md.