Repository navigation
Overwrite an existing attribute instead of writing through an uninitialised pointer - #1
Merged
Merged
Conversation
added 2 commits
October 8, 2026 17:42
TSyntaxNode.SetAttribute pointed its entry pointer at a slot only when it added the key. For a key the node already carried the pointer was left uninitialised and the value was written through it anyway; dcc32 says so with W1036 on AttributeEntry. Any directive stored twice reaches it: procedure x(a: Integer); stdcall; stdcall; external 'a.dll' index 93; is legal Delphi, stores anCallingConvention twice and ends in an access violation, or in a silent write to whatever address the stack held. SetAttribute now looks the existing entry up with TryGetAttributeEntry and overwrites its value, and adds an entry only for a new key. An empty value removes the attribute and returns, instead of falling through to the same uninitialised pointer after RemoveAttribute. The last value stored wins, so `stdcall; cdecl;` records cdecl.
SetAttribute with an empty value goes through RemoveAttribute, which did not remove anything. It computed the byte offset of the entry past the one being removed and then moved the entries one slot further along rather than back, so the removed entry stayed, the one before the last was overwritten, and the array kept its length. Only the key's bit in FAttributesInUse was cleared, which hid the stale entry from HasAttribute but not from Attributes, so the writers and the binary serializer still saw it. The Move also copied the string values bit for bit, leaving two entries owning one reference. RemoveAttribute now shifts the entries after the removed one back by assignment, which keeps the string reference counts right, and shortens the array by one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug.
TSyntaxNode.SetAttributeonly pointedAttributeEntryat a slot when it added a new key. For a key the node already carried, the value was written through the uninitialised pointer (dcc32 reports W1036 onAttributeEntry). Any directive stored twice reaches it.Repro.
This is legal Delphi. It ends in an access violation ("write of address 0000000B"), or in a silent stray write when the stack happens to hold a valid address.
stdcall; cdecl;hits the same path.The
Value = ''path was broken as well. It calledRemoveAttributeand then wrote through the same uninitialised pointer.RemoveAttributeitself did not remove anything: it moved the entries one slot forward instead of back, bit-copied the managed strings, and never shortened the array. Only theFAttributesInUsebit was cleared, soAttributes(and with it the writers and the binary serializer) still listed the stale entry.Fix.
SetAttributelooks up an existing entry withTryGetAttributeEntryand overwrites its value, and it adds an entry only for a new key. An empty value removes the attribute and returns. The last value stored wins. W1036 is gone.RemoveAttributeshifts the later entries back by assignment and shortens the array by one.Tests.
Node.AttributeOverwrite,Node.AttributeRemoveandAST.RepeatedCallingConventionare added toTest/UnitTests. All three fail onmain(two access violations and a stale entry) and pass with this change.Serialization.BinaryRoundTripfails onmaintoo when built with Delphi 10.4 (line_seqvalues) and is not affected by this change.