Diskswapper WinUAE Bug report!

Hi Winuae code/bug maintainer,

I am using Winuae version 2.0.1 2009.12.23. First of all I really like this Amiga Emulator, and I use it with a lot of pleasure. I found some bugs with the diskswapper feature of winuae. In perticilar the multidisk image selection of the diskswapper. It wouldn't display the first string
correctly, and it stopped displaying arround the half of the complete string
size that I selected (all the filenames with path, seperated by a 0). So I started debugging, and found that bug of the missing first string was burried in the multiloop function in win32gui.c line 1905

so I rewrote the function, now it takes the first string in consideration.
Please replace the old multiloop with this one. no changes to behaviour are made, it still needs the first call with an NULL parameter for the out parameter, this is and was needed to initialize the static indx to 0 (still works that way)

int loopmulti (TCHAR *s, TCHAR *out)
{
size_t slen;
static int indx;

if (out == NULL) {
// initialize indx to zero, this is where we are starting from
indx = 0;
return 1;
}

// get the length of the string
slen = _tcslen(&s[indx]);

if (slen > 0) {
// we have a valid stringlength, copy it, and add zero to the end
_tcscpy(out, &s[indx]);
_tcscat(out, L"\0");

// increase the indx for the next loop, by slen + 1 so it points to
// the following string, when it exists
indx += (slen + 1);
return 1;
}

// there is no string at s[indx]
return 0;
}

the second bug is in displaying only half of the multiple selected files in the
diskswapper. I debugged this to line 1763 of win32gui.c. These two calls only move half of the complete full_path buffer. The old code got the complete buffer size, and divided it with the sizeof TCHAR which is 2. I replace the calls with the following
code;

// bug, was copying the half of the string to full_path, the same for stored_path
memcpy (full_path2, full_path, sizeof (full_path));
memcpy (stored_path, full_path, sizeof (stored_path));

the last bug was garbage at the end of the half copied multiple selected string. That is because the string buffers weren't emptied and had cc's in them. So I added the following code before the ZeroMemory (&openFileName, sizeof (OPENFILENAME)); at line 1533 in the win32gui.c file. The way the code is set up, requires the buffers to be filled with zero's
because the ending zero for a string is almost never set, but expected to be in the buffer already.

ZeroMemory (&previousfilter, sizeof (int) * 20);
ZeroMemory (&filtername, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&full_path, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&full_path2, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&file_name, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&init_path, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&description, sizeof (TCHAR) * CFG_DESCRIPTION_LENGTH);
ZeroMemory (&szTitle, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&szFormat, sizeof (TCHAR) * MAX_DPATH);
ZeroMemory (&szFilter, sizeof (TCHAR) * MAX_DPATH);

These changes repair the 3 problems above.

(nizchka) Lars Van Jeurissen

[hit]

hi & welcome,

nice first posting - i guess toni will appreciate it

Hi Hit,

Thanks for the welcome. Here is an extra adition I would like to add.
I would like to add a maxsize parameter to this call, so that you won't get over the array boundary. But I think to do that, you have to make changes
in for instance the DiskSelection_2 where the multiple disk selection is also copied to an MAX_DPATH buffer. When you select more as 20 files there it
probably will crash. This is for the developer to decide. But here is possible addition to the loopmulti function.
There a two calls made to loopmulti, it can be done easily.

int loopmulti (TCHAR *s, TCHAR *out, int maxsize)
{
size_t slen;
static int indx;

if (out == NULL) {
// initialize indx to zero, this is where we are starting from
indx = 0;
return 1;
}

// get the length of the string
slen = _tcslen(&s[indx]);

// check if we have space in the out buffer sized by maxsize - 2
// maxsize - 2 (1 for the array index is zero based)
// and 1 because we need a zero at the end of the string
// we should drop the string because it isn't complete
if (indx + slen >= maxsize - 2) {
return 0;
}

if (slen > 0) {
// we have a valid stringlength, copy it, and add zero to the end
_tcscpy(out, &s[indx]);
_tcscat(out, L"\0");

// increase the indx for the next loop, by slen + 1 so it points to
// the following string, when it exists
indx += (slen + 1);
return 1;
}

// there is no string at s[indx]
return 0;
}

[Toni Wilen]

It is "usual" unicode conversion bug, not the first and probably not the last either..

DPATH overflow: I can fix the overflow, I don't think anyone really need to select huge number of images and dynamic buffer allocation is annoying without using some smarter strings.. (and that won't happen until/if I decide to replace GUI with portable wxwidgets GUI)

[hit]

@nizchka: thnx again for your contribution, but as you see, we have to leave it to Toni, whether or not he wants to add such improvement

[Toni Wilen]

Originally Posted by nizchka

the last bug was garbage at the end of the half copied multiple selected string. That is because the string buffers weren't emptied and had cc's in them. So I added the following code before the ZeroMemory (&openFileName, sizeof (OPENFILENAME)); at line 1533 in the win32gui.c file. The way the code is set up, requires the buffers to be filled with zero's because the ending zero for a string is almost never set, but expected to be in the buffer already.

What code requires buffers filled completely with zeros? (btw, previousfilter is static and is meant to be not initialized every time DiskSelection_2() gets called)

// bug, was copying the half of the string to full_path, the same for stored_path
memcpy (full_path2, full_path, sizeof (full_path));
memcpy (stored_path, full_path, sizeof (stored_path));

This was stupid bug.. I did some search&replace changes and I guess I missed it was memcpy(), not some string copy

_tcscpy(out, &s[indx]);
_tcscat(out, L"\0");

This does not make any sense. strcpy (_tcscpy) always copies the terminating zero. (or does it attempt to add second terminating zero? it isn't going to work either)

[Toni Wilen]

So I started debugging, and found that bug of the missing first string was burried in the multiloop function in win32gui.c line 1905

Ah, there is a bug but multiloop() is fine. It is Vista+ IID_IFileOpenDialog/IID_IFileSaveDialog OPENFILENAME "emulation" code that forgots to emulate nFileOffset.

You'll notice it works fine if you fallback to normal GetSaveFileName() or GetOpenFileName() in GetFileDialog(). (or run it under WinXP)

This also "fixes" MAX_DPATH limit because only first file name is supposed to include full path.

Hi Tony,

I read the OFN_ALLOWMULTISELECT flag and yes you have to register the nFileOffset. Oke thanks problem solved. That explains loopmulti as it is.

Nizchka

[Toni Wilen]

Will be fixed in next beta. Thanks for the report, I'd have missed that memcpy() length error without your debugging.

[Mclane]

Sigh, now THAT's how to do a bug report, you even made Toni smile......A rare thing

[cosmicfrog]

indeed no need for the crystal ball this time

Originally Posted by Toni Wilen

What code requires buffers filled completely with zeros? (btw, previousfilter is static and is meant to be not initialized every time DiskSelection_2() gets called)



This was stupid bug.. I did some search&replace changes and I guess I missed it was memcpy(), not some string copy

This does not make any sense. strcpy (_tcscpy) always copies the terminating zero. (or does it attempt to add second terminating zero? it isn't going to work either)

I found some places where the code uses half the buffer size. / sizeof( TCHAR ). From some of these I thought that it was perculiar to give only half of the TCHAR buffersize. But I'm not familiar with this code, it can be that this is intentionally.

Late at night isn't allways the best time to be playing with code.

Anyway thanks. for the great work at winuae. I really like it!

[Toni Wilen]

Originally Posted by nizchka

I found some places where the code uses half the buffer size. / sizeof( TCHAR ). From some of these I thought that it was perculiar to give only half of the TCHAR buffersize. But I'm not familiar with this code, it can be that this is intentionally.

string functions require number of characters, memory functions require number of bytes. They are different in unicode builds (1 character is 2 bytes)

Real solution is C++ strings or similar that would hide implementation details but WinUAE is still more or less plain C.. (yet another reason why at least GUI stuff should be rewritten in some C++ thing)

[Leffmann]

Originally Posted by Toni Wilen

Real solution is C++ strings or similar that would hide implementation details but WinUAE is still more or less plain C.. (yet another reason why at least GUI stuff should be rewritten in some C++ thing)

If you decide to go through with this, check out Qt. It's been around for some time and has turned into a fully featured write-once-compile-anywhere platform, and is also the perfect choice if portability would seem meaningful at some point eventhough this is WinUAE. If you don't care about portability then it still offers excellent GUI and string/text handling among other things.

[Toni Wilen]

Originally Posted by Leffmann

If you decide to go through with this, check out Qt. It's been around for some time and has turned into a fully featured write-once-compile-anywhere platform, and is also the perfect choice if portability would seem meaningful at some point eventhough this is WinUAE. If you don't care about portability then it still offers excellent GUI and string/text handling among other things.

I have been looking at wxwidgets because it is portable (why rewrite GUI without making it portable?), uses host OS native gui elements (non-native elements = looks alien, not acceptable for me) and supports os-specific "special" features (lowest common denominator feature set = no thanks) and can be statically linked, GPL compatible (plain winuae.exe MUST run without extra dlls)

(I think I wrote similar reply few months ago?)

[andreas]

@Leffmann
Oomphhh...Qt. It's a great product (and "Made in Scandinavia" [LOL]), but could get Toni in serious trouble with licensing I think.

"But Qt is GPL!"

No, it was GPL. But you will have to stick to version 4.4 for the rest of your (resp. WinUAE's) life.
Or be doomed to mix GPL with LGPL, since only the latter licensing type is available in 4.5+.

[thor]

No, you don't have to use <4.5 in a GPL project and you aren't doomed when using compatible licences (and what does this have to do with the original topic?).