Skip to content

Added various features #655

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
PQCraft wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added various features #655

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd30f4b
cxbe: Add the ability to handle long section names
PQCraft Jul 20, 2023
2f0deb5
cxbe: Add -TITLEID:{%c%c-%u|%x}
PQCraft Jul 20, 2023
c0a40d6
cxbe: Add -REGION:{-|[n][j][w][m]}
PQCraft Sep 19, 2023
b62a0ae
cxbe: Add -VERSION
PQCraft Jul 20, 2023
fc892e7
cxbe: Set the proper flags for *IMAGE sections
PQCraft Jul 21, 2023
25c32e4
cxbe: Set proper flags on debug sections
PQCraft Jul 22, 2023
e13a346
Add features to Makefile
PQCraft Jul 20, 2023
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
47 changes: 46 additions & 1 deletion tools/cxbe/Exe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ Exe::Exe(const char *x_szFilename)
// read section headers
{
m_SectionHeader = new SectionHeader[m_Header.m_sections];
m_SectionHeader_longname = new SectionHeader_longname[m_Header.m_sections];

printf("Exe::Exe: Reading Section Headers...\n");

Expand All @@ -115,7 +116,46 @@ Exe::Exe(const char *x_szFilename)
goto cleanup;
}

printf("OK %d\n", v);
// interpret long section names
if(m_SectionHeader[v].m_name[0] == '/')

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How was this commit tested?
In all my testing, I could not get lld-link to produce a section that exceeds 8 characters, even with trickery. This matches Microsoft's documentation, which says that executable images do not use a string table and do not support section names longer than 8 characters.

@PQCraft PQCraft Sep 30, 2023 •
edited
Loading

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 used objcopy to rename a section I made in some inline asm
XTIMAGE -> $$XTIMAGE

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have to say I'm not a fan, I'd rather stay a mile away from intentionally breaking the PE spec (or giving the impression that we support doing so).

@PQCraft PQCraft Sep 30, 2023 •
edited
Loading

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.

The thing with / is documented in the spec on microsoft's page. While it says that long section names are not supported with executables, objcopy can still produce executables with long section names and they load and run just fine.

Oh also debug sections use long section names

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it is documented as not being valid for executable images. While it may be possible to hack around the linker for that, it is breaking the spec.

@PQCraft PQCraft Sep 30, 2023

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.

Also I added this because I actually need this to have a title image.
It would incorrectly add debug sections and the image sections as something like /43 which ofc would not be picked up by the dashboard as an icon.

@JayFoxRox JayFoxRox Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. How does the XDK do it?
  2. Can't we tell cxbe to add or rename sections? (Not that I'd like the added complexity, but it's probably better than breaking the spec)

{
m_SectionHeader_longname[v].m_offset = 0;
for(uint32 i = 1; i < 8; ++i)
{
char c = m_SectionHeader[v].m_name[i];
if(!c)
break;
if(c < '0' || c > '9') // not a long section after all?
goto notlong;
m_SectionHeader_longname[v].m_offset *= 10;
m_SectionHeader_longname[v].m_offset += c - '0';
}
m_SectionHeader_longname[v].m_longname = new char[256]();

long tmppos = ftell(ExeFile);
fseek(ExeFile, m_Header.m_symbol_table_addr + m_SectionHeader_longname[v].m_offset,
SEEK_SET);

uint32 i;
for(i = 0; i < 255; ++i)
{
int c = fgetc(ExeFile);
if(!c || c == EOF)
break;
m_SectionHeader_longname[v].m_longname[i] = c;
}
m_SectionHeader_longname[v].m_longname[i] = 0;

fseek(ExeFile, tmppos, SEEK_SET);
printf("OK %d (long)\n", v, m_SectionHeader_longname[v].m_offset,
m_SectionHeader_longname[v].m_longname);
}
else
{
notlong:;
m_SectionHeader_longname[v].m_longname = NULL;
printf("OK %d\n", v);
}
}
}

Expand Down Expand Up @@ -184,6 +224,7 @@ Exe::Exe(const char *x_szFilename)
void Exe::ConstructorInit()
{
m_SectionHeader = NULL;
m_SectionHeader_longname = NULL;
m_bzSection = NULL;
}

Expand All @@ -193,12 +234,16 @@ Exe::~Exe()
if(m_bzSection != 0)
{
for(uint32 v = 0; v < m_Header.m_sections; v++)
{
delete[] m_SectionHeader_longname[v].m_longname;
delete[] m_bzSection[v];
}

delete[] m_bzSection;
}

delete[] m_SectionHeader;
delete[] m_SectionHeader_longname;
}

// export to Exe file
Expand Down
7 changes: 7 additions & 0 deletions tools/cxbe/Exe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ class Exe : public Error
uint32 m_characteristics; // characteristics for this segment
} __attribute((packed)) * m_SectionHeader;

// array to store long section names
struct SectionHeader_longname
{
char *m_longname;
uint32 m_offset;
} __attribute((packed)) * m_SectionHeader_longname;

// array of section data
uint08 **m_bzSection;

Expand Down
113 changes: 108 additions & 5 deletions tools/cxbe/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "Exe.h"
#include "Xbe.h"

#include <stdlib.h>
#include <string.h>

// program entry point
Expand All @@ -19,18 +20,34 @@ int main(int argc, char *argv[])
char szXbeFilename[OPTION_LEN + 1] = { 0 };
char szDumpFilename[OPTION_LEN + 1] = { 0 };
char szXbeTitle[OPTION_LEN + 1] = "Untitled";
char szXbeTitleID[OPTION_LEN + 1] = "";
char szXbeRegions[OPTION_LEN + 1] = "";
char szXbeVersion[OPTION_LEN + 1] = "";
char szMode[OPTION_LEN + 1] = "retail";
char szLogo[OPTION_LEN + 1] = "";
char szDebugPath[OPTION_LEN + 1] = "";

bool bRetail;
uint32 dwTitleId = 0xFFFF0002;
uint32 dwRegions;

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to get initialized or lines 121+ will read undefined values.

@PQCraft PQCraft Sep 30, 2023

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.

It does in line 141 under the else

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a different control flow path, I'm talking about 121+

@PQCraft PQCraft Sep 30, 2023

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.

Ah ok I see
The fix was put in the wrong commit I think

uint32 dwVersion;

const char *program = argv[0];
const char *program_desc = "CXBE EXE to XBE (win32 to Xbox) Relinker (Version: " VERSION ")";
Option options[] = {
{ szExeFilename, NULL, "exefile" }, { szXbeFilename, "OUT", "filename" },
{ szDumpFilename, "DUMPINFO", "filename" }, { szXbeTitle, "TITLE", "title" },
{ szMode, "MODE", "{debug|retail}" }, { szLogo, "LOGO", "filename" },
{ szDebugPath, "DEBUGPATH", "path" }, { NULL }
{ szExeFilename, NULL, "exefile" },
{ szXbeFilename, "OUT", "filename" },
{ szDumpFilename, "DUMPINFO", "filename" },
{ szXbeTitle, "TITLE", "title" },
{ szXbeTitleID, "TITLEID", "{%c%c-%u|%x}" },
{ szXbeRegions, "REGION",
"{-|[n][j][w][m]}\n"
" -=none, n=North America, j=Japan, w=world, m=manufacturing" },
{ szXbeVersion, "VERSION", "version" },
{ szMode, "MODE", "{debug|retail}" },
{ szLogo, "LOGO", "filename" },
{ szDebugPath, "DEBUGPATH", "path" },
{ NULL }
};

if(ParseOptions(argv, argc, options, szErrorMessage))
Expand All @@ -54,6 +71,91 @@ int main(int argc, char *argv[])
szXbeTitle[40] = '\0';
}

// interpret title id
if(szXbeTitleID[0])
{
bool hex = true;
for(int i = 0; szXbeTitleID[i]; ++i)
{
if(szXbeTitleID[i] == '-')
{
hex = false;
break;
}
}
if(hex)
{
sscanf(szXbeTitleID, "%x", &dwTitleId);
}
else
{
char titlechar[2];
unsigned titleno;
Comment on lines +93 to +94

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using a not explicitly unsigned char can lead to incorrect results when shifting, better use the types defined in Cxbx.h or stdint.h for these variables.

@PQCraft PQCraft Sep 30, 2023

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.

Will adding unsigned do?
I prefer to use stdlib types with an stdlib function

if (sscanf(szXbeTitleID, "%c%c-%u", &titlechar[0], &titlechar[1], &titleno) != 3)

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line doesn't follow clang-format rules.

@PQCraft PQCraft Sep 30, 2023

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'll see about doing a rebase and running clang-format on all the commits

{
strncpy(szErrorMessage, "invalid TITLEID", ERROR_LEN);
goto cleanup;
}
if(titleno > 0xFFFF)
{
printf("WARNING: Title ID number too high (max is 65535)\n");
titleno = 0xFFFF;
}
dwTitleId = (titlechar[0] << 24) | (titlechar[1] << 16) | titleno;
}
}

// interpret region flags
if(szXbeRegions[0])
{
char c;
for(int i = 0; (c = szXbeRegions[i]); ++i)
{
switch(c)
{
case '-':;
dwRegions = 0;
goto breakfor;
case 'a':;
dwRegions = XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD |
XBEIMAGE_GAME_REGION_MANUFACTURING;
goto breakfor;
case 'n':;
dwRegions |= XBEIMAGE_GAME_REGION_NA;
break;
case 'j':;
dwRegions |= XBEIMAGE_GAME_REGION_JAPAN;
break;
case 'w':;
dwRegions |= XBEIMAGE_GAME_REGION_RESTOFWORLD;
break;
case 'm':;
dwRegions |= XBEIMAGE_GAME_REGION_MANUFACTURING;
break;
default:;
printf("WARNING: Invalid region char: %c\n", c);
break;
}
}
breakfor:;
}
else
{
dwRegions = XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD | XBEIMAGE_GAME_REGION_MANUFACTURING;
}

// interpret version
if(szXbeVersion[0])
{
dwVersion = strtoul(szXbeVersion, NULL, 0);
}
else
{
dwVersion = 0;
}

// verify we received the required parameters
if(szExeFilename[0] == '\0')
{
Expand Down Expand Up @@ -90,7 +192,8 @@ int main(int argc, char *argv[])
LogoPtr = &logo;
}

Xbe *XbeFile = new Xbe(ExeFile, szXbeTitle, bRetail, LogoPtr, szDebugPath);
Xbe *XbeFile =
new Xbe(ExeFile, szXbeTitle, dwTitleId, dwRegions, dwVersion, bRetail, LogoPtr, szDebugPath);

if(XbeFile->GetError() != 0)
{
Expand Down
71 changes: 49 additions & 22 deletions tools/cxbe/Xbe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ static size_t BasenameOffset(const std::string &path)
}

// construct via Exe file object
Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vector<uint08> *logo,
const char *x_szDebugPath)
Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, uint32 x_dwTitleID, uint32 x_dwRegions,
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo, const char *x_szDebugPath)

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not follow clang-format rules.

{
ConstructorInit();

Expand Down Expand Up @@ -125,8 +125,16 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
{
uint32 s = 0;

while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
s++;
if(x_Exe->m_SectionHeader_longname[v].m_longname)
{
while(s < 255 && x_Exe->m_SectionHeader_longname[v].m_longname[s] != '\0')
s++;
}
else
{
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
s++;
}

mrc += s + 1;
}
Expand Down Expand Up @@ -271,8 +279,7 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

m_Certificate.dwTimeDate = CurrentTime;

// TODO: generate in the form CX-9999
m_Certificate.dwTitleId = 0xFFFF0002;
m_Certificate.dwTitleId = x_dwTitleID;

// title name
memset(m_Certificate.wszTitleName, 0, sizeof(m_Certificate.wszTitleName));
Expand All @@ -297,19 +304,15 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
XBEIMAGE_MEDIA_TYPE_MEDIA_BOARD | XBEIMAGE_MEDIA_TYPE_NONSECURE_HARD_DISK |
XBEIMAGE_MEDIA_TYPE_NONSECURE_MODE;

// TODO: allow configuration
m_Certificate.dwGameRegion = XBEIMAGE_GAME_REGION_MANUFACTURING |
XBEIMAGE_GAME_REGION_NA | XBEIMAGE_GAME_REGION_JAPAN |
XBEIMAGE_GAME_REGION_RESTOFWORLD;
m_Certificate.dwGameRegion = x_dwRegions;
Comment on lines -300 to +308

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change probably belongs to a different commit and breaks building this one.

@PQCraft PQCraft Sep 30, 2023

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.

Probably
My rebase was kind of sketchy and one of my commits disappeared into thin air
This was probably a result of me trying to re-create the commit


// TODO: allow configuration
m_Certificate.dwGameRatings = 0xFFFFFFFF;

// always disk 0, AFAIK
m_Certificate.dwDiskNumber = 0;

// TODO: allow configuration
m_Certificate.dwVersion = 0;
m_Certificate.dwVersion = x_dwVersion;

// generate blank LAN, signature, and alternate signature keys
{
Expand Down Expand Up @@ -345,7 +348,7 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

// write section headers / section names
{
m_szSectionName = new char[m_Header.dwSections][9];
m_szSectionName = new char[m_Header.dwSections][256];

m_SectionHeader = new SectionHeader[m_Header.dwSections];

Expand Down Expand Up @@ -373,14 +376,27 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec

memset(&m_SectionHeader[v].dwFlags, 0, sizeof(m_SectionHeader->dwFlags));

if(characteristics & IMAGE_SCN_MEM_WRITE)
m_SectionHeader[v].dwFlags.bWritable = true;
// check for $$XTIMAGE or $$XSIMAGE and set the correct flags
if(x_Exe->m_SectionHeader_longname[v].m_longname &&
(!strcmp(x_Exe->m_SectionHeader_longname[v].m_longname, "$$XTIMAGE") ||
!strcmp(x_Exe->m_SectionHeader_longname[v].m_longname, "$$XSIMAGE")))
Comment on lines +382 to +383

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not a fan of this hack. I'd rather like to see proper file insertion support in cxbe and something like -TITLEIMAGE: / -SAVEIMAGE: or similar.

@PQCraft PQCraft Sep 30, 2023

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.

There is currently no code to do section insertion as far as i'm aware, and that could probably be removed later once there is code made to add sections.

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There isn't, as I said I'd rather see it implemented than a name-based hack on top of violating the PE spec. nxdk does not require the usage of these sections nor does it implement the APIs relying on them yet, so we might as well take the time to do things properly.

@PQCraft PQCraft Sep 30, 2023

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 don't have the time to write something like that and I need the feature now. Loading the image sections takes up ram to do nothing. This code can be removed later when the better code is added.

@thrimbor thrimbor Sep 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This code can be removed later when the better code is added.

I'm not really interested in nxdk acquiring even more technical debt. We haven't even gotten rid of the one inherited from OpenXDK yet.

{
m_SectionHeader[v].dwFlags.bInsertedFile = true;
m_SectionHeader[v].dwFlags.bHeadPageRO = true;
m_SectionHeader[v].dwFlags.bTailPageRO = true;
Comment on lines +385 to +387

@thrimbor thrimbor Sep 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How were these flag settings determined? They don't match Halo 2.

@PQCraft PQCraft Sep 30, 2023

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 used an XBE header dumper on a few XBEs. I think it was Doom 3 and a Quake homebrew XBE I had laying around

}
else
{
if(characteristics & IMAGE_SCN_MEM_WRITE)
m_SectionHeader[v].dwFlags.bWritable = true;

if((characteristics & IMAGE_SCN_MEM_EXECUTE) ||
(characteristics & IMAGE_SCN_CNT_CODE))
m_SectionHeader[v].dwFlags.bExecutable = true;

if((characteristics & IMAGE_SCN_MEM_EXECUTE) ||
(characteristics & IMAGE_SCN_CNT_CODE))
m_SectionHeader[v].dwFlags.bExecutable = true;
m_SectionHeader[v].dwFlags.bPreload = true;
}

m_SectionHeader[v].dwFlags.bPreload = true;
m_SectionHeader[v].dwVirtualAddr =
x_Exe->m_SectionHeader[v].m_virtual_addr + m_Header.dwPeBaseAddr;

Expand Down Expand Up @@ -431,10 +447,21 @@ Xbe::Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail, const std::vec
memset(secn, 0, 8);

m_SectionHeader[v].dwSectionNameAddr = hwc_secn;
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
if(x_Exe->m_SectionHeader_longname[v].m_longname)
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader[v].m_name[s];
s++;
while(s < 255 && x_Exe->m_SectionHeader_longname[v].m_longname[s] != '\0')
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader_longname[v].m_longname[s];
s++;
}
}
else
{
while(s < 8 && x_Exe->m_SectionHeader[v].m_name[s] != '\0')
{
m_szSectionName[v][s] = secn[s] = x_Exe->m_SectionHeader[v].m_name[s];
s++;
}
}

m_szSectionName[v][s] = '\0';
Expand Down
8 changes: 4 additions & 4 deletions tools/cxbe/Xbe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ class Xbe : public Error
{
public:
// construct via Exe file object
Xbe(class Exe *x_Exe, const char *x_szTitle, bool x_bRetail,
const std::vector<uint08> *logo = nullptr, const char *x_szDebugPath = nullptr);
Xbe(class Exe *x_Exe, const char *x_szTitle, uint32 x_dwTitleID, uint32 x_dwRegions,
uint32 x_dwVersion, bool x_bRetail, const std::vector<uint08> *logo = nullptr, const char *x_szDebugPath = nullptr);

// deconstructor
~Xbe();
Expand Down Expand Up @@ -170,8 +170,8 @@ class Xbe : public Error
uint32 dwCharacteristics; // characteristics
} __attribute((packed)) * m_TLS;

// Xbe section names, each 8 bytes max and null terminated
char (*m_szSectionName)[9];
// Xbe section names, each 255 bytes max and null terminated
char (*m_szSectionName)[256];

// Xbe sections
uint08 **m_bzSection;
Expand Down