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

cxbe: Fix broken XBE sections sizes #665

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

LoveMHz
Copy link
Contributor

@LoveMHz LoveMHz commented Apr 16, 2024

A simple test with the hello world example is enough to generate an XBE with an invalid header.

#include <hal/debug.h>
#include <hal/video.h>
#include <windows.h>

__attribute__((section("TEST")))
void test() {
    Sleep(1000);
}

int main(void) {
    XVideoSetMode(640, 480, 32, REFRESH_DEFAULT);

    while(1) {
        debugPrint("Hello nxdk!\n");
        Sleep(2000);
    }

    return 0;
}

This results in the final section, TEST, being generated with a physical section size greater than the virtual size.

image

The physical size is calculated here. The calculated physical size is 512 bytes in this case since the code section is filled with 0xCC.

Since this is the last section in the PE, the virtual size is pulled from the PE directly. here.


Calculating the physical size from the last zero-byte makes sense since the Xbox kernel will set the memory tail end of the section to zero based on the difference between physical and virtual size.

This PR attempts to fix this issue by ensuring the section's virtual size is at least as large as the physical size.

@LoveMHz LoveMHz changed the title cxbx: Fix Broken XBE Sections Sizes cxbx: Fix broken XBE sections sizes Apr 16, 2024
Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

nit: The commit title prefix should be cxbe instead of cxbx

I think would be nice if we could lose some of the unnecessary `0xCC' in the file, but that's something we can think about in the future.
Code looks fine, I've left some comments about details.

tools/cxbe/Xbe.cpp Outdated Show resolved Hide resolved
Comment on lines +409 to +410
if(v < m_Header.dwSections - 1)
m_SectionHeader[v].dwVirtualSize =
x_Exe->m_SectionHeader[v + 1].m_virtual_addr -
x_Exe->m_SectionHeader[v].m_virtual_addr;
Copy link
Member

Choose a reason for hiding this comment

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

I've never really noticed how weird and seemingly unnecessary this code is - unless it causes some breakage I'm not seeing rn we should imho just remove this part entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't be opposed to making this a separate issue/pr.

In my opinion, there's a lot more of cxbe that should be simplified to trust the generated PE from the linker.

Comment on lines 414 to 417
m_SectionHeader[v].dwVirtualSize = MAX(
RoundUp(x_Exe->m_SectionHeader[v].m_virtual_size, 4),
m_SectionHeader[v].dwSizeofRaw
);
Copy link
Member

Choose a reason for hiding this comment

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

I always kind of assumed raw size > virtual size being fine because it's allowed in the PE format (SizeOfRawData is rounded to FileAlignment, VirtualSize is exact). Does it cause an issue in the XBE loader?

Copy link
Contributor Author

@LoveMHz LoveMHz Apr 16, 2024

Choose a reason for hiding this comment

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

It appears so. XepLoadSection takes in an XBE section and, after loading, attempts to clear the tail end of the section.

Retail 5838
image

@LoveMHz LoveMHz changed the title cxbx: Fix broken XBE sections sizes cxbe: Fix broken XBE sections sizes Apr 16, 2024
@thrimbor thrimbor force-pushed the fix-xbe-section-size branch from b5ddbdf to 69f6c7d Compare April 17, 2024 12:39
@thrimbor thrimbor merged commit 463647e into XboxDev:master Apr 17, 2024
6 checks passed
@LoveMHz LoveMHz deleted the fix-xbe-section-size branch April 19, 2024 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants