Skip to content

Fix stack-buffer-overflow sanitizer issue in AUIPC_JALR_TO_XQCI.test #922

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.

Sign up for GitHub

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

Open
parth-07 wants to merge 1 commit into
base: release/22.x
Choose a base branch
from
+2 −2
Open

Fix stack-buffer-overflow sanitizer issue in AUIPC_JALR_TO_XQCI.test #922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/eld/Fragment/RegionFragmentEx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class RegionFragmentEx : public Fragment {

static bool classof(const RegionFragmentEx *) { return true; }

bool replaceInstruction(uint32_t Offset, Relocation *Reloc, uint32_t Instr,
bool replaceInstruction(uint32_t Offset, Relocation *Reloc, uint64_t Instr,
Copy link
Contributor

@quic-seaswara quic-seaswara Mar 6, 2026

Choose a reason for hiding this comment

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

templatized function might be better

Copy link
Member

@lenary lenary Mar 6, 2026

Choose a reason for hiding this comment

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

Given this is just doing a memcpy, maybe we can just expose Instr as const char * to avoid templating or issues with sizing, and pass &Instr everywhere we're calling it?

Copy link
Contributor Author

@parth-07 parth-07 Mar 9, 2026

Choose a reason for hiding this comment

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

@quic-seaswara I like @lenary's suggestion. Please let me know your thoughts on it.

Copy link
Contributor

@quic-seaswara quic-seaswara Mar 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

sounds good!, const uint8_t *

Copy link
Member

@lenary lenary Mar 10, 2026

Choose a reason for hiding this comment

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

Technically a const uint8_t * isn't the same as a const char * in terms of strict aliasing, BUT you use the former so much in the codebase I'm not going to complain if you keep using it in this interface too.

Copy link
Contributor

@quic-seaswara quic-seaswara Mar 10, 2026

Choose a reason for hiding this comment

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

I agree, we should be consistent, for now const char * is ok, but we should clean it up when dealing with ELF data as const uint8_t * as much as possible

uint8_t Size);
void deleteInstruction(uint32_t Offset, uint32_t Size);
void addRequiredNops(uint32_t Offset, uint32_t NumNopsToAdd);
Expand Down
2 changes: 1 addition & 1 deletion lib/Fragment/RegionFragmentEx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ RegionFragmentEx::RegionFragmentEx(const char *Buf, size_t Sz, ELFSection *O,
RegionFragmentEx::~RegionFragmentEx() {}

bool RegionFragmentEx::replaceInstruction(uint32_t Offset, Relocation *Reloc,
uint32_t Instr, uint8_t Size) {
uint64_t Instr, uint8_t Size) {
std::memcpy((void *)(Data + Offset), &Instr, Size);
return true;
}
Expand Down