Page 1 of 1

2 bugs found in string.c of PS2SDK

Posted: Tue Jan 09, 2007 3:13 pm
by dlanor
In my recent work on uLaunchELF I've discovered that two functions in the PS2SDK source file "ee/libc/src/string.c" are bugged in identical ways.

The affected functions are "strcpy_sjis" and "strcpy_ascii", which are intended to translate strings between SJIS and ASCII formats. Both functions do that by handling one character per pass of a for loop, and exiting that loop with the loop index indicating the next character in the destination array, ready for use in storing the terminating zero. But strangely enough the current code uses an offset of +1 added to that index, thus storing the terminator in the next array element beyond the one where it should have been. This makes the translated string contain one additional character at the end, this being whatever garbage happened to occupy that RAM location before the function call.

The reason why this has not been discovered earlier is probably that the most common use of strcpy_sjis is to create icon titles, and the Sony browser simply ignores invalid SJIS characters at the end of an icon title (and in most cases garbage will be invalid for SJIS), while the strcpy_ascii function probably hasn't been used much at all, as it's only needed to display SJIS as ascii, which few programs need. (uLE uses another function for that.)

Best regards: dlanor

Posted: Wed Jan 10, 2007 4:01 am
by evilo
great (although I actually never used it), don't hesitate to correct the source in the svn with your modifications :)

I don't know why, but I'm always excited with ps2sdk bug corrections... !

Posted: Fri Jan 12, 2007 2:18 pm
by dlanor
evilo wrote:great (although I actually never used it), don't hesitate to correct the source in the svn with your modifications :)
Actually I think some 'hesitation' on my part is inevitable, as I'm not aware of having write access to SVN, and have never performed such corrections before.

With TortoiseSVN I believe I should only have to store the changed files in the proper project subfolder of my local SVN repository folder, thus replacing any older versions of the same files, and then rightclick that project subfolder and select 'SVN Commit' to start uploading, just as I normally select 'SVN Update' to download updates to such a project. Can someone please confirm this procedure ? (as I never did this before)

In any case it will obviously have no effect unless my account has the proper rights setup for the SVN server.

I don't know why, but I'm always excited with ps2sdk bug corrections... !
Changes to an SDK has the potential of improving all the applications and drivers recompiled with it, so I agree that it's something to be excited about, even if this particular change only affects very few programs.

Best regards: dlanor

Posted: Fri Jan 12, 2007 5:09 pm
by jbit
I haven't had a chance to check this, but it looks like you're right. I committed what I think is the change you suggested.
Can you check over the svn log http://svn.ps2dev.org/listing.php?repna ... =1369&sc=1

I will test this tomorrow with some shift-jis test cases..

Thanks for the report :)

Posted: Fri Jan 12, 2007 7:10 pm
by EP
Yes, that's it.

Posted: Sun Jan 14, 2007 8:28 pm
by dlanor
jbit wrote:I haven't had a chance to check this, but it looks like you're right. I committed what I think is the change you suggested.
Can you check over the svn log http://svn.ps2dev.org/listing.php?repna ... =1369&sc=1
I've just inspected your changes, and I concur with EP. Those are just the changes I had in mind.

I will test this tomorrow with some shift-jis test cases..
Good, but I'm already sure those tests will come out fine.


@EP:
So now we can remove string.c from the collection of 'Changed source' files for uLE, without having had it in any public release even... Surely a record, compared to results for our other changes ! ;)

Best regards: dlanor

Posted: Mon Jan 15, 2007 1:38 pm
by EP
dlanor wrote:@EP:
So now we can remove string.c from the collection of 'Changed source' files for uLE, without having had it in any public release even... Surely a record, compared to results for our other changes ! ;)
Yes, definitely a record. :) Maybe it's a sign of things to come.

Posted: Mon Jan 15, 2007 2:17 pm
by jbit
If you have any fixes for ps2sdk just put them on the forum or message me and assuming they're sane I'll put them in svn when I get time.

Posted: Tue Jan 16, 2007 12:25 am
by dlanor
jbit wrote:If you have any fixes for ps2sdk just put them on the forum or message me and assuming they're sane I'll put them in svn when I get time.
In the work on uLaunchELF we have developed a number of different fixes for external projects. Some of these relate to parts of PS2SDK, while others relate to non-SDK projects also hosted at ps2dev.org, those being: gsKit, libcdvd, libjpg, ps2client and ps2ftpd.

However, most of those changes are a little harder to describe concisely than the recent fix you made of those incorrect index offsets. And some of our changes have already been posted here a very long time ago, without any results.

Those were the fixes to allow EE libs for pad and MC access to be used both before and after an IOP reset. Without our fixes this will 'kill' the EE libs if they were initialized before the IOP reset, thus losing all access to MC and gamepad thereafter. Unfortunately those fixes were fully successful only for the MC driver, but even that was an important gain for us, as it allows us to read the CNF file of uLE before deciding whether an IOP reset is to be used or not, based on settings in that file. With standard PS2SDK libs that could never work.

Perhaps those posts were merely overlooked at the time, so I guess we should try again then, both for those fixes and the other ones we've made.

Best regards: dlanor

Posted: Tue Jan 16, 2007 6:36 am
by EP
Hum, well there are a lot of changes. If memory serves correctly, this a basic rundown:

ps2sdk related:
EE/rpc includes fixes for a more proper IOP reset. There are also notes and a correction of the data type in mcSetFileInfo().

IOP/hdd includes changes to use standard Tokyo time for timestamps in both hdd/misc.c and pfs/misc.c. A proper timezone function doesn't exist for the drivers and the standard time the PS2 uses is Tokyo time. It was changed to be consistent with the internal memory card driver.

IOP/system/iomanX was changed back to an earlier version of the ps2sdk since we had issues with the driver stealing focus. In other words, once a driver was accessed by one part of uLE, it would prevent the same driver from being accessed by another part of uLE. Ex. usb_mass.

IOP/tcpip was changed back to use the older lwip because it broke host:, ps2ftpd, and pretty much all network related functionality.

The other changes are to other outside things such as gskit, libcdvd, libjpg, ps2client, and ps2ftpd. If you want we could post all of this stuff in another thread if you're still interested.:) There's quite a bit of stuff here and there.

Posted: Tue Jan 16, 2007 6:46 am
by jbit
Yeah, start another thread and post details on the bugs and fixes.
So just say what the bug is, how to repeat it, what the fix is, what the fix achieves, etc.
Some of the "revert back to old version" ones I won't do, but I'll look into what the problem is and see if i can fix it in the new version.

Posted: Tue Jan 16, 2007 7:22 pm
by dlanor
jbit wrote:Some of the "revert back to old version" ones I won't do, but I'll look into what the problem is and see if i can fix it in the new version.
I agree completely on this. PS2SDK should always go forward, never back, even if some recent step forward went astray. As you say, the right thing to do is to find a way to fix it without 'backing'.

As for the ps2ip stuff, the latest uLE betas use the experimental modules by EEUG, so the lwip 'backing' we did before those may no longer be relevant. Since his modules have allowed us to almost triple the speed of 'host:', I'm very much inclined to stick with them for the coming stable uLE releases as well.

Best regards: dlanor

Posted: Wed Jan 17, 2007 7:56 am
by EP
dlanor wrote:I agree completely on this. PS2SDK should always go forward, never back, even if some recent step forward went astray. As you say, the right thing to do is to find a way to fix it without 'backing'.
Yes, but finding a workaround first is usually the best first step.:)
dlanor wrote:As for the ps2ip stuff, the latest uLE betas use the experimental modules by EEUG, so the lwip 'backing' we did before those may no longer be relevant. Since his modules have allowed us to almost triple the speed of 'host:', I'm very much inclined to stick with them for the coming stable uLE releases as well.
Sure, but we'll likely have to get rid of ps2ftpd if we do. Unfortunately, some are already using those early betas as official releases. So I'm already expecting people to come in yelling about uLE causing data corruption.

Posted: Wed Jan 17, 2007 11:46 pm
by dlanor
----- snip ----- re: PS2SDK going forward, never backing
EP wrote:Yes, but finding a workaround first is usually the best first step.:)
Sure, and such workarounds can give important clues as to how to take the next step forward. Also, in making an application work it's always OK to take a step back in its usage of libs and/or modules. It's only for the SDK release as such that I'd consider backing wrong. (And even that could be acceptable if applied to a specific known bug.)


----- snip ----- re: sticking with EEUGs high-speed TCP/IP modules
Sure, but we'll likely have to get rid of ps2ftpd if we do.
Perhaps, but perhaps not. Once those new modules are in SVN we may be able to find out what causes those problems and fix them.

But if not, then I'm afraid we have to kiss that server goodbye... ;) Its presence in uLE is not worth going back to a 'host:' speed of 500 KByte/s after experiencing 1450 KByte/s.

Unfortunately, some are already using those early betas as official releases. So I'm already expecting people to come in yelling about uLE causing data corruption.
They can yell all they like. While I'll always accept such reports as feedback for improvements, I won't even consider us being held responsible for what they've done with a clearly labled beta release.

But perhaps we've gone a bit far off-topic here, and should continue this elsewhere.

Best regards: dlanor