Skip to content

Aliases for standard C functions (memset/memcpy/memcmp) #654

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

Draft
rmikielis wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Aliases for standard C functions (memset/memcpy/memcmp) #654

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
3 changes: 3 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ include ../phoenix-rtos-build/Makefile.common
CFLAGS += -I. -ffreestanding
CPPFLAGS += -DVERSION=\"$(VERSION)\" -DRELEASE=\"$(RELEASE)\" -DTARGET_FAMILY=\"$(TARGET_FAMILY)\"

# Add aliases to standard C library functions as required by GCC
LDFLAGS += -Wl,--defsym=memcpy=hal_memcpy,--defsym=memcmp=hal_memcmp,--defsym=memset=hal_memset
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2025

Choose a reason for hiding this comment

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

critical

While aliasing is necessary, there's a critical issue concerning memcpy's behavior with overlapping memory regions and the current hal_memcpy implementations.

  • The C standard memcpy has undefined behavior for overlapping source/destination regions. memmove should be used when overlap is possible.
  • The aarch64 hal_memcpy (in hal/aarch64/_memcpy.S) handles overlaps, acting like memmove. However, the ia32 hal_memcpy (in hal/ia32/string.c) uses a forward-only copy (rep movsl/movsb with cld), which is not safe for all overlap scenarios (e.g., dst > src). The riscv64 hal_memcpy also doesn't explicitly guarantee overlap safety.
  • Aliasing memcpy to a hal_memcpy that isn't universally overlap-safe (especially on ia32) introduces a high risk of data corruption if the compiler generates memcpy calls or if developers use memcpy for operations that might overlap.

To address this, consider these options:

  1. Option A (Make hal_memcpy universally overlap-safe):
    Update all hal_memcpy implementations (especially for ia32 and riscv64, if affected) to handle overlaps correctly, similar to memmove. If this is done, consider aliasing memmove to this now-safe hal_memcpy:
    LDFLAGS += -Wl,--defsym=memcpy=hal_memcpy,--defsym=memcmp=hal_memcmp,--defsym=memset=hal_memset,--defsym=memmove=hal_memcpy
    (Assuming hal_memcpy becomes fully overlap-safe on all platforms).

  2. Option B (Provide a separate, safe hal_memmove - Recommended):
    Keep hal_memcpy non-overlap-safe. Provide a separate, overlap-safe hal_memmove function for all architectures, and alias memmove to it. This is crucial because code might rely on memmove for overlap safety. The change would be:
    LDFLAGS += -Wl,--defsym=memcpy=hal_memcpy,--defsym=memcmp=hal_memcmp,--defsym=memset=hal_memset,--defsym=memmove=hal_memmove
    (This requires hal_memmove to be implemented correctly and be overlap-safe across all platforms).

Clarify the intended strategy for memory operation safety and ensure a mechanism for safe overlapping memory copies (memmove semantics) is reliably available across all target architectures. Without this, the change poses a significant risk of data integrity issues.

LDFLAGS += -Wl,--defsym=memcpy=hal_memcpy,--defsym=memcmp=hal_memcmp,--defsym=memset=hal_memset,--defsym=memmove=hal_memmove


# uncomment to enable stack canary checking
# CPPFLAGS += -DSTACK_CANARY

Expand Down
Loading