[libdvdcss-devel] [PATCH] Win32: move to WideChars for cache folder

[Jean-Baptiste Kempf]

---
src/libdvdcss.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/libdvdcss.c b/src/libdvdcss.c
index a5014b4..9c9a1e1 100644
--- a/src/libdvdcss.c
+++ b/src/libdvdcss.c
@@ -244,15 +244,21 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( char *psz_target )
{
#if defined(_WIN32_IE) && _WIN32_IE >= 0x500
char psz_home[MAX_PATH];
+ wchar_t wdir[MAX_PATH];

/* Cache our keys in
* C:\Documents and Settings\$USER\Application Data\dvdcss\ */
- if (SHGetFolderPathA (NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE,
- NULL, SHGFP_TYPE_CURRENT, psz_home ) == S_OK)
+ if (SHGetFolderPathW (NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE,
+ NULL, SHGFP_TYPE_CURRENT, wdir ) == S_OK)
{
- snprintf( psz_buffer, PATH_MAX, "%s\\dvdcss", psz_home );
- psz_buffer[PATH_MAX-1] = '\0';
- psz_cache = psz_buffer;
+ int size_needed = WideCharToMultiByte (CP_UTF8, 0, wdir, -1, NULL, 0, NULL, NULL);
+ if (size_needed != 0)
+ {
+ WideCharToMultiByte( CP_UTF8, 0, wdir, -1, psz_home, size_needed, NULL, NULL);
+ snprintf( psz_buffer, PATH_MAX, "%s\\dvdcss", psz_home );
+ psz_buffer[PATH_MAX-1] = '\0';
+ psz_cache = psz_buffer;
+ }
}
#else
char *psz_home = NULL;

[Reimar Döffinger]

Does this actually fix anything? If so it would be good if the commit message mentioned that.

[Jean-Baptiste Kempf]

Post by Reimar Döffinger
Does this actually fix anything? If so it would be good if the commit message mentioned that.

Of course it does. What happens if your username is "Döffinger 福島第" for
example?

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger
Does this actually fix anything? If so it would be good if the commit

message mentioned that.
Of course it does. What happens if your username is "Döffinger 福島第" for
example?

I would expect the A variant of the function to return the 8.3 compatibility file name and it will work just fine.
I seriously wasn't asking just for fun.

[Jean-Baptiste Kempf]

Post by Reimar Döffinger
I would expect the A variant of the function to return the 8.3 compatibility file name and it will work just fine.

I don't think that would account for all cases, to be honest. According
to my limited VLC on Win32 experience, it doesn't.

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger
I would expect the A variant of the function to return the 8.3

compatibility file name and it will work just fine.
I don't think that would account for all cases, to be honest. According
to my limited VLC on Win32 experience, it doesn't.

It's not that I don't believe you, but it would be nice to know where specifically the old code failed.
For example it is possible to turn off support for 8.3 compatibility names.
If this change only affects those systems that makes it a lot less critical and IMHO that would be information that is really nice to have in the commit message.
Of course it's also possible that my assumption of it falling back to 8.3 is wrong.

[Jean-Baptiste Kempf]

Post by Reimar Döffinger
Of course it's also possible that my assumption of it falling back to 8.3 is wrong.

Well, I don't see any documentation of this assumption on MSDN.
If you have a link, please share.

Best regards,

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger
Of course it's also possible that my assumption of it falling back to

8.3 is wrong.
Well, I don't see any documentation of this assumption on MSDN.
If you have a link, please share.

Well, it doesn't seem unusual for it to return the short path, which means reproducing the issue might be hard:
http://www.powerbasic.com/support/pbforums/showthread.php?t=8994
On the other hand the opposite seems possible as well:
http://stackoverflow.com/questions/4547917/shgetfolderpath-returns-path-with-question-marks-in-it
Now I see that you just make the function return a utf8 string, does that mean dvdcss uses a wrapper around the Windows open function? (can't look at the code currently)
In that case it its also possible that this wrapper might actually break things...
If we do not use such a wrapper yet, the solution in above link of getting the short name might be more reliable...

[Jean-Baptiste Kempf]

Post by Reimar Döffinger
If we do not use such a wrapper yet, the solution in above link of getting the short name might be more reliable...

I hope you are kidding.

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger
If we do not use such a wrapper yet, the solution in above link of

getting the short name might be more reliable...
I hope you are kidding.

Well, better than using something that will not work on several MinGW setups and won't work on VisualStudio either!
Using _wopen is of course better, though I don't know how much work that would be.

[Jean-Baptiste Kempf]

Post by Reimar Döffinger
Using _wopen is of course better, though I don't know how much work that would be.

Shouldn't be that hard. I will have a look at it.

Best regards,

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger
Using _wopen is of course better, though I don't know how much work

that would be.
Shouldn't be that hard. I will have a look at it.

Btw. I just discovered an interesting feature: the C runtime from VC 2005 and later can use UTF-8 in fopen by specifying ccs=UTF-8 in the mode string.
Maybe not worth relying on it though.

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger

Post by Reimar Döffinger
Using _wopen is of course better, though I don't know how much work

that would be.
Shouldn't be that hard. I will have a look at it.

Btw. I just discovered an interesting feature: the C runtime from VC
2005 and later can use UTF-8 in fopen by specifying ccs=UTF-8 in the
mode string.
Maybe not worth relying on it though.

_O_UTF8 is the equivalent for open()

[Reimar Döffinger]

Post by Reimar Döffinger

Post by Reimar Döffinger

Post by Reimar Döffinger

Post by Reimar Döffinger
Using _wopen is of course better, though I don't know how much work

that would be.
Shouldn't be that hard. I will have a look at it.

Btw. I just discovered an interesting feature: the C runtime from VC
2005 and later can use UTF-8 in fopen by specifying ccs=UTF-8 in the
mode string.
Maybe not worth relying on it though.

_O_UTF8 is the equivalent for open()

Ok, I will shut up now, that was nonsense. That's only for the content when opening in text mode.
Sorry for the mess.

[Ivan Kalvachev]

Post by Jean-Baptiste Kempf
---
src/libdvdcss.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/libdvdcss.c b/src/libdvdcss.c
index a5014b4..9c9a1e1 100644
--- a/src/libdvdcss.c
+++ b/src/libdvdcss.c
@@ -244,15 +244,21 @@ LIBDVDCSS_EXPORT dvdcss_t dvdcss_open ( char *psz_target )
{
#if defined(_WIN32_IE) && _WIN32_IE >= 0x500
char psz_home[MAX_PATH];
+ wchar_t wdir[MAX_PATH];
/* Cache our keys in
* C:\Documents and Settings\$USER\Application Data\dvdcss\ */
- if (SHGetFolderPathA (NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE,
- NULL, SHGFP_TYPE_CURRENT, psz_home ) == S_OK)
+ if (SHGetFolderPathW (NULL, CSIDL_APPDATA | CSIDL_FLAG_CREATE,
+ NULL, SHGFP_TYPE_CURRENT, wdir ) == S_OK)
{
- snprintf( psz_buffer, PATH_MAX, "%s\\dvdcss", psz_home );
- psz_buffer[PATH_MAX-1] = '\0';
- psz_cache = psz_buffer;
+ int size_needed = WideCharToMultiByte (CP_UTF8, 0, wdir, -1,
NULL, 0, NULL, NULL);
+ if (size_needed != 0)
+ {
+ WideCharToMultiByte( CP_UTF8, 0, wdir, -1, psz_home,
size_needed, NULL, NULL);
+ snprintf( psz_buffer, PATH_MAX, "%s\\dvdcss", psz_home );
+ psz_buffer[PATH_MAX-1] = '\0';
+ psz_cache = psz_buffer;
+ }
}
#else
char *psz_home = NULL;

When you convert 2 byte unicode to utf8, the UTF-8 may be 1, 2 or 3
bytes per character. (Anything above U+0x800 will be 3 bytes).
In short, this opens the possibility for buffer overflow.
(Not likely to be exploitable, it would likely overflow into the other array).

Possible solutions:
- make psz_home bigger (2*MAX_PATH),

- malloc psz_home, we have the size_needed. Bonus, psz_home
definition could be shared with the unix codepath.

- clip size_needed (it will be clipped in snprintf anyway).


BTW, are mingw libc functions expecting utf8 encoding?
Aka, would open() convert filename from utf8 to wchar and use *W
windows functions or would they use *A functions directly?

[Jean-Baptiste Kempf]

Post by Ivan Kalvachev
- malloc psz_home, we have the size_needed. Bonus, psz_home
definition could be shared with the unix codepath.

Should be doable.

Post by Ivan Kalvachev
BTW, are mingw libc functions expecting utf8 encoding?

Yes.

Best regards,

[Reimar Döffinger]

Post by Jean-Baptiste Kempf

Post by Ivan Kalvachev
- malloc psz_home, we have the size_needed. Bonus, psz_home
definition could be shared with the unix codepath.

Should be doable.

Post by Ivan Kalvachev
BTW, are mingw libc functions expecting utf8 encoding?

Yes.

What? Definitely not all MingW versions. In fact I just a few days ago cross-compiled MPlayer under Debian (unstable) and open() definitely did not work with a UTF8 path.

[Jean-Baptiste Kempf]

Post by Reimar Döffinger

Post by Jean-Baptiste Kempf

Post by Ivan Kalvachev
- malloc psz_home, we have the size_needed. Bonus, psz_home
definition could be shared with the unix codepath.

Should be doable.

Post by Ivan Kalvachev
BTW, are mingw libc functions expecting utf8 encoding?

Yes.

What? Definitely not all MingW versions. In fact I just a few days ago cross-compiled MPlayer under Debian (unstable) and open() definitely did not work with a UTF8 path.

Probably with a DEFINE. _wopen will always work, though.