fix romimg bundled on docker containers #673

israpps · 2024-11-06T04:22:40Z

instead of relying on strlen, go straight away with max bufsize. before this change the romimg from docker would segfault while local build works like a charm

New feature, -c and -a commands will have new variants, -C and -A, these will manipulate the filename written to ROMFS removing extension and converting to uppercase

So for example

romimg -C IOPRP.IMG $PS2SDK/iop/irx/secrman.irx

Will register secrman.irx as SECRMAN on the image, this seems to be needed when the IOPRP is replacing startup IOP modules

fjtrujy · 2024-11-06T13:00:51Z

tools/romimg/src/romimg.c

-#else
-	char* UserName;
-#endif
+	char LocalhostName[32] = "\0", cwd[128] = "\0", UserName[32] = "\0";


I still think that 128 is too short for computer usage. You should use MAX_PATH instead

I guess there is no cross platform definition of MAX_PATH. linux seems to use PATH_MAX

Yeah sorry PATH_MAX or whatever is called, I don't remember right now the POSIX solution for that hehe

tools/romimg/src/romimg.c

instead of relying on strlen, go straight away with max bufsize

israpps · 2024-11-06T15:05:15Z

@fjtrujy any idea with this compiler warning? not sure what to do to get rid of it.

My local build (WSL) does not complain of this

uyjulian · 2024-11-06T16:20:48Z

Add or remove one from either buffer or length

israpps · 2024-11-06T16:28:19Z

Add or remove one from either buffer or length

will take a look. buffer is 11 and i'm reading 10 already

uyjulian · 2024-11-06T17:46:38Z

The buffer is being truncated.

That is, the destination buffer is smaller than the source buffer. So increase the destination buffer size or decrease the source buffer size

israpps · 2024-11-06T18:13:55Z

im kinda lost now

/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:10: error: ‘__builtin_strncpy’ output may be truncated copying 9 bytes from a string of length 9 [-Werror=stringop-truncation]

fjtrujy · 2024-11-07T08:08:42Z

im kinda lost now

/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:10: error: ‘__builtin_strncpy’ output may be truncated copying 9 bytes from a string of length 9 [-Werror=stringop-truncation]

The error is quite easy, you do something like:

strncpy(file->RomDir.name, fname, sizeof(file->RomDir.name) - 1);

This -1 at the end is the root of the issue.
It should be directly:

strncpy(file->RomDir.name, fname, sizeof(file->RomDir.name));

And you also have it in other places as:

strncpy(tbuf, fname, sizeof(tbuf) - 1);

I think that in general the way of using strncpy should be:

strncpy(a, b, sizeof(a));

fjtrujy · 2024-11-07T08:11:03Z

tools/romimg/src/romimg.c


 #include "platform.h"
 #include "romimg.h"
 #include "SonyRX.h"

+#define BUFCHK(X) (X[0] == '\0') ? "" : X


Do you think this is really needed?
I think it makes no sense at all, the behavior should be the same, no matter if the first character is \0

yeah, it remained from older approach. will deal with it now

I realised I placed this recipe into the C makefile, leaving C++ and IOP C without this recipe

israpps · 2024-11-11T03:20:52Z

fixed. seems to work as expected!

fjtrujy · 2024-11-11T07:50:01Z

Finally 😄

fjtrujy · 2024-11-11T08:05:01Z

tools/romimg/src/romimg.c

-	CommentLength = 31 + strlen(filename) + strlen(UserName) + strlen(LocalhostName) + strlen(cwd);
-	ROMImg->comment = (char *)malloc(CommentLength);
-	sprintf(ROMImg->comment, "%08x,conffile,%s,%s@%s/%s", ROMImg->date, filename, (UserName[0] =='\0')?"":UserName, LocalhostName, cwd);
+	CommentLength = IMAGE_COMMENT_BASESIZE + strlen(filename) + sizeof(LocalhostName) + sizeof(UserName) + MAX_PATH;


use sizeof at the end

fjtrujy · 2024-11-11T08:06:23Z

tools/romimg/src/platform.h


 #if defined(_WIN32) || defined(WIN32)
 #define PATHSEP '\\'
 #else
 #define PATHSEP '/'
 #endif
+#ifndef MAX_PATH
+#define MAX_PATH 4096


PATH_MAX is available in all the OS, so use it instead and remove this from here

windows has the name flipped MAX_PATH instead of PATH_MAX

I'm using MAX_PATH in my fork and all the CI/CD was passing.
You are probably speaking about compiling natively, not through MinGW, in theory, we shouldn't care about these platforms...

not sure where it is defined then... everything in google says on limits.h but it's not the case

fjtrujy · 2024-11-11T08:07:02Z

tools/romimg/src/romimg.c

-#else
-	char* UserName;
-#endif
+	char LocalhostName[32] = "\0", cwd[MAX_PATH] = "\0", UserName[32] = "\0";


I recommend to initializate them with = {0} instead, it is equivalent to put all the fields with \0 directly

fjtrujy · 2024-11-11T08:08:23Z

tools/romimg/src/romimg.c

 	FILE *InputFile;
 	int result;
 	unsigned int FileDateStamp;
 	unsigned short FileVersion;
 	if ((InputFile = fopen(path, "rb")) != NULL) {
 		const char* fname = strrchr(path, PATHSEP);
 		if (fname == NULL) fname = path; else fname++;
+        if (upperconv) {
+            strncpy(tbuf, fname, sizeof(tbuf));
+            tbuf[sizeof(tbuf) - 1] = '\0';


if you initialize with = {0} then this line shouldn't be required either

fjtrujy reviewed Nov 6, 2024

View reviewed changes

tools/romimg/src/romimg.c Outdated Show resolved Hide resolved

fjtrujy reviewed Nov 6, 2024

View reviewed changes

tools/romimg/src/romimg.c Outdated Show resolved Hide resolved

israpps force-pushed the romimg-fix branch 2 times, most recently from 5029ea8 to 699e6f2 Compare November 6, 2024 13:34

fix romimg bundled on docker containers

c848b03

instead of relying on strlen, go straight away with max bufsize

israpps force-pushed the romimg-fix branch from 699e6f2 to c848b03 Compare November 6, 2024 13:43

israpps marked this pull request as draft November 6, 2024 13:59

israpps marked this pull request as ready for review November 6, 2024 14:44

israpps force-pushed the romimg-fix branch 2 times, most recently from 7c8a04d to 2edbeef Compare November 6, 2024 15:00

israpps force-pushed the romimg-fix branch from 2edbeef to 986a387 Compare November 6, 2024 16:28

israpps force-pushed the romimg-fix branch 9 times, most recently from 29f4ef8 to ce264c6 Compare November 6, 2024 17:14

israpps force-pushed the romimg-fix branch from ce264c6 to aa14f88 Compare November 6, 2024 18:12

fjtrujy reviewed Nov 7, 2024

View reviewed changes

israpps force-pushed the romimg-fix branch from aa14f88 to cf1a096 Compare November 7, 2024 11:41

[romimg] implement filename transformation

ec79a39

israpps force-pushed the romimg-fix branch from cf1a096 to ec79a39 Compare November 7, 2024 11:45

move ioprp recipe to a separate makefile

af00502

I realised I placed this recipe into the C makefile, leaving C++ and IOP C without this recipe

israpps force-pushed the romimg-fix branch 2 times, most recently from d8ce025 to b575b96 Compare November 11, 2024 03:18

israpps requested a review from fjtrujy November 11, 2024 03:20

fjtrujy reviewed Nov 11, 2024

View reviewed changes

israpps force-pushed the romimg-fix branch 2 times, most recently from 1b630de to 8dbb79a Compare November 11, 2024 12:40

fix romimg error

b10e8ba

israpps force-pushed the romimg-fix branch from 8dbb79a to b10e8ba Compare November 11, 2024 12:47

fjtrujy approved these changes Nov 11, 2024

View reviewed changes

fjtrujy merged commit ed5ead7 into ps2dev:master Nov 11, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix romimg bundled on docker containers #673

fix romimg bundled on docker containers #673

israpps commented Nov 6, 2024 •

edited

Loading

fjtrujy Nov 6, 2024

israpps Nov 6, 2024

fjtrujy Nov 6, 2024

israpps commented Nov 6, 2024 •

edited

Loading

uyjulian commented Nov 6, 2024

israpps commented Nov 6, 2024

uyjulian commented Nov 6, 2024

israpps commented Nov 6, 2024

fjtrujy commented Nov 7, 2024

fjtrujy Nov 7, 2024

israpps Nov 7, 2024

israpps commented Nov 11, 2024

fjtrujy commented Nov 11, 2024

fjtrujy Nov 11, 2024

fjtrujy Nov 11, 2024

israpps Nov 11, 2024

fjtrujy Nov 11, 2024

israpps Nov 11, 2024

fjtrujy Nov 11, 2024

fjtrujy Nov 11, 2024

fix romimg bundled on docker containers #673

fix romimg bundled on docker containers #673

Conversation

israpps commented Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

israpps commented Nov 6, 2024 • edited Loading

uyjulian commented Nov 6, 2024

israpps commented Nov 6, 2024

uyjulian commented Nov 6, 2024

israpps commented Nov 6, 2024

fjtrujy commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

israpps commented Nov 11, 2024

fjtrujy commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

israpps commented Nov 6, 2024 •

edited

Loading

israpps commented Nov 6, 2024 •

edited

Loading