Skip to content

Dynamic determination of buffer size at compile time for easier porting to distros with non-standard shebangs - #4426

Open
libx264 wants to merge 4 commits into
ptitSeb:mainfrom
libx264:main
Open

libx264 wants to merge 4 commits into
ptitSeb:mainfrom
libx264:main

Conversation

@libx264

@libx264 libx264 commented Sep 18, 2026 •

Copy link
Copy Markdown

Change signature strings from const char* to const char[] in src/utils/fileutils.c and derive buffer sizes using sizeof().

This removes hardcoded buffer sizes (20, 25) and allows stack buffers to automatically adapt at compile time if signature paths are patched for distros with non-standard prefixes (such as Termux or NixOS), preventing buffer overflows.

…nlarge stack buffers to 64 bytes and use safe fread bounds in FileIsX64ELF, FileIsX86ELF, FileIsX64X86ELF, FileIsShell, and FileIsPython. Prevents __fread_chk fortify aborts and stack smashing on ARM64 during execve.
@libx264

libx264 commented Sep 18, 2026

Copy link
Copy Markdown
Author

Should fix #4382

@ptitSeb

ptitSeb commented Sep 18, 2026

Copy link
Copy Markdown
Owner

I'm sorry, but I don't understand were is the fix? the fread change you do is ... strange. The form used in box64 is perfectly legal and correct, so please expand (and please, don't use AI to explain, it's your PR, not an AI one)

Comment thread src/tools/fileutils.c Outdated
Comment thread src/tools/fileutils.c
char head[64] = {0};
size_t sz = fread(head, 1, 20, f);

char head[sizeof(x64lib)] = {0};

@ptitSeb ptitSeb Sep 18, 2026 •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

what was wrong with the 64-bytes size of the array before, because I think that was the onyl fix there

@libx264 libx264 Sep 18, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thought it would be good to implement dynamic head buffer size

To reduce the number of magic numbers

Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
Comment thread src/tools/fileutils.c
@libx264

libx264 commented Sep 18, 2026

Copy link
Copy Markdown
Author

I must apologize, at least for ai description and unnecessary changes. The issue persist on termux, because of patches that change system prefix, but they forgot to modify head size. I should have edited their patch, but I thought it would be good to implement dynamic head buffer size.

@libx264
libx264 marked this pull request as draft September 18, 2026 19:01
@libx264
libx264 marked this pull request as ready for review September 18, 2026 19:49
@ptitSeb

ptitSeb commented Sep 21, 2026

Copy link
Copy Markdown
Owner

That looks ok for now, but I still don't see any "Fix buffer overflow and stack smashing". Where is the buffer overflow in the original code?

@libx264

libx264 commented Sep 23, 2026

Copy link
Copy Markdown
Author

The description and title were generated by an AI, so they are incorrect and outdated. Initially, I simply increased the buffer size and saw that it fixed the issue. After further testing, I realized the bug was actually on Termux's side, as they were blindly replacing shebangs at the beginning of the file with patch in pkg repo. However, I still think that dynamically determining the buffer size at compile time is a good idea. I could close this and open a new pull request, but I can also just edit the current title and description instead.

@libx264 libx264 changed the title Fix buffer overflow and stack smashing in FileIs* functions during execve Dynamic determination of buffer size at compile time for easier porting to distros with non-standard shebangs Sep 23, 2026
@ptitSeb

ptitSeb commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Well, I would appreciate some cleanup yes. Title and main description are really heavy and confusing. Either edit or create a new ticket is up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants