Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add guard word before local var CMiniColDef[9] #73736

Conversation

RobertHenry6bev
Copy link
Contributor

Initialize the guard word so it does not pass the UsesAllocatedMemory
test.

Found running output of clang14 -fsanitize=address, then inspection

Fixes #73718

Initialize the guard word so it does not pass the UsesAllocatedMemory
test.

Found running output of clang14 -fsanitize=address, then inspection
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Initialize the guard word so it does not pass the UsesAllocatedMemory
test.

Found running output of clang14 -fsanitize=address, then inspection

Fixes #73718

Author: RobertHenry6bev
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: -

Copy link
Member

@AntonLapounov AntonLapounov left a comment

Choose a reason for hiding this comment

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

Please use something like below for consistency with other places:

    const int MaxCols = 9;
    BYTE tempCols[MaxCols * sizeof(CMiniColDef) + 1];

    _ASSERTE(MaxCols >= pTable->m_cCols);
    // Mark the array of columns as not allocated (not ALLOCATED_MEMORY_MARKER) for SetNewColumnDefinition call bellow
    tempCols[0] = 0;
    CMiniColDef *pCols = BYTEARRAY_TO_COLDES(tempCols);

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 11, 2022
@tommcdon tommcdon added this to the 8.0.0 milestone Aug 11, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 12, 2022
CMiniColDef pCols[9]; // The col defs to init.
const int MaxCols = 9;
typedef uint32_t markword_t;
BYTE tempCols[sizeof(markword_t) + MaxCols * sizeof(CMiniColDef)]; // keep aligned
Copy link
Member

Choose a reason for hiding this comment

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

We use a single-byte marker everywhere in this code. I see no reason to use four bytes just at this particular place. This array contains 3-byte structures, so you cannot align them.

Copy link
Member

Choose a reason for hiding this comment

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

Here is similar code in another function:

// Mark the array of columns as not allocated (not ALLOCATED_MEMORY_MARKER) for SetNewColumnDefinition
// call bellow (code:#SetNewColumnDefinition_call)
*(BYTE *)(qbTempCols.Ptr()) = 0;
sTempTable.m_pColDefs = (CMiniColDef *)((BYTE *)(qbTempCols.Ptr()) + 1);

Copy link
Member

Choose a reason for hiding this comment

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

And here:

BYTE *newMemory = new (nothrow) BYTE[(sizeof(CMiniColDef)*pTable->m_cCols)+1];
if (newMemory == NULL)
return E_OUTOFMEMORY;
// Mark the first byte in this as with the "allocated memory marker"
*newMemory = ALLOCATED_MEMORY_MARKER;
// Have the pointer point to the first Column Descriptor
pTable->m_pColDefs = BYTEARRAY_TO_COLDES(newMemory);


_ASSERTE(MaxCols >= pTable->m_cCols);
//
// Mark the array of columns as not allocated (eg, not ALLOCATED_MEMORY_MARKER)
Copy link
Member

Choose a reason for hiding this comment

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

E.g. means “for example”. Did you actually mean "i.e."?

@tommcdon
Copy link
Member

tommcdon commented Sep 5, 2022

@RobertHenry6bev thanks for your contribution. Would you mind looking at the comments @AntonLapounov left? If you do not have time right now to respond we can move this PR to draft mode.

@tommcdon tommcdon marked this pull request as draft September 12, 2022 17:30
@tommcdon
Copy link
Member

Hi @RobertHenry6bev, I've moved this PR to draft mode. Once it is ready for review, please feel free to move out of draft mode. Thanks for your contributions!

@ghost ghost closed this Oct 12, 2022
@ghost
Copy link

ghost commented Oct 12, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

illegal read of byte preceding an automatic (stack allocate) variable
3 participants