flac dev - Mar 2013 - Patch to add Unicode filename support for win32 flac

On 18.3.2013 12:25, Erik de Castro Lopo wrote:
> JonY wrote:
>
>> Before anyone does anything, see __wgetmainargs
>> <http://msdn.microsoft.com/en-us/library/ff770599.aspx>.
>>
>> It can expand wildcards. Since it already provides argc/argv/env, it is
>> more a less a drop-in replacement for the main() arguments.
> +1
>
> Erik
Alright, here's a patch utilizing this function. There's a lot of 
changes here.
Project files have a new configuration called "Release (UTF8)",
intended
to be used when building the command line tools. This project has the 
required UTF-8 define in place so all libraries expect things in encoded 
format.
Regular Debug and Release configurations do not use any new tricks so 
existing projects won't break when compiled with those settings.

I'm at work and couldn't do extensive testing, but command line FLAC.exe
seems to perform everything right with this.

Metaflac probably requires some minor tweaks but I wanted to show some 
progress so that 1.3 doesn't slip out the door while I'm busy.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: utf8_io.zip
Type: application/x-zip-compressed
Size: 16598 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20130319/b6a01e17/attachment-0001.bin

On 3/19/2013 19:59, Janne Hyv?rinen wrote:
> On 18.3.2013 12:25, Erik de Castro Lopo wrote:
>> JonY wrote:
>>
>>> Before anyone does anything, see __wgetmainargs
>>> <http://msdn.microsoft.com/en-us/library/ff770599.aspx>.
>>>
>>> It can expand wildcards. Since it already provides argc/argv/env,
it is
>>> more a less a drop-in replacement for the main() arguments.
>> +1
>>
>> Erik
> 
> Alright, here's a patch utilizing this function. There's a lot of
> changes here.
> Project files have a new configuration called "Release (UTF8)",
intended
> to be used when building the command line tools. This project has the
> required UTF-8 define in place so all libraries expect things in encoded
> format.
> Regular Debug and Release configurations do not use any new tricks so
> existing projects won't break when compiled with those settings.
> 
> I'm at work and couldn't do extensive testing, but command line
FLAC.exe
> seems to perform everything right with this.
> 
> Metaflac probably requires some minor tweaks but I wanted to show some
> progress so that 1.3 doesn't slip out the door while I'm busy.
> 
Should the following
#if defined _MSC_VER || defined __MINGW32__

be simplified to
#ifdef WIN32

? Alternatively _WIN32. Since it's win32 and not something compiler
specific.

As for the macros:
+#define flac_vfprintf vfprintf_utf8
+#define flac_fopen fopen_utf8
+#define flac_stat _stat64_utf8
...

I leave this up for Erik to decide, though I would have preferred not
using rename macros at all.

Not sure if these were intended:
+#include <sys/stat.h> /* for flac_stat() */
+#include <sys/utime.h> /* for flac_utime() */
+#include <io.h> /* for flac_chmod() */

As for calling __wgetmainargs, I have some concerns about the security
implications:
LoadLibrary("msvcrt.dll") <- Which msvcrt? Theoretical security
exploit.

I think it is best to link it directly, please use the following
prototype and call it directly:

============================================#ifdef _DLL
#define CALL_DLLIMPORT __declspec(dllimport)
#else
#define CALL_DLLIMPORT
#endif
int __cdecl CALL_DLLIMPORT __wgetmainargs(int*, wchar_t***, wchar_t***,
int, int*);
============================================
This should simplify the error handling logic and help against
LoadLibrary handle leaks, though the leak should not be an issue in
practice since it is only called once. The symbol should also be present
in MSVCR* DLLs.

In stat_utf8, dealing with 32bit/64bit time_t? The following check
should really compile time rather than runtime:
sizeof(*buffer) == sizeof(st)

That's all for now, I have not actually tested with mingw yet.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: OpenPGP digital signature
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20130319/cbff0f5e/attachment.pgp

On 19.3.2013 15:49, JonY wrote:
> On 3/19/2013 19:59, Janne Hyv?rinen wrote:
>> On 18.3.2013 12:25, Erik de Castro Lopo wrote:
>>> JonY wrote:
>>>
>>>> Before anyone does anything, see __wgetmainargs
>>>> <http://msdn.microsoft.com/en-us/library/ff770599.aspx>.
>>>>
>>>> It can expand wildcards. Since it already provides
argc/argv/env, it is
>>>> more a less a drop-in replacement for the main() arguments.
>>> +1
>>>
>>> Erik
>> Alright, here's a patch utilizing this function. There's a lot
of
>> changes here.
>> Project files have a new configuration called "Release
(UTF8)", intended
>> to be used when building the command line tools. This project has the
>> required UTF-8 define in place so all libraries expect things in
encoded
>> format.
>> Regular Debug and Release configurations do not use any new tricks so
>> existing projects won't break when compiled with those settings.
>>
>> I'm at work and couldn't do extensive testing, but command line
FLAC.exe
>> seems to perform everything right with this.
>>
>> Metaflac probably requires some minor tweaks but I wanted to show some
>> progress so that 1.3 doesn't slip out the door while I'm busy.
>>
> Should the following
> #if defined _MSC_VER || defined __MINGW32__
>
> be simplified to
> #ifdef WIN32
>
> ? Alternatively _WIN32. Since it's win32 and not something compiler
> specific.
Indeed. Not sure what I was thinking.
>
> As for the macros:
> +#define flac_vfprintf vfprintf_utf8
> +#define flac_fopen fopen_utf8
> +#define flac_stat _stat64_utf8
> ...
>
> I leave this up for Erik to decide, though I would have preferred not
> using rename macros at all.
I think this is the sanest way. Only few #ifdefs instead of wrapper 
functions filled with them that do essentially nothing on *nix.
>
> Not sure if these were intended:
> +#include <sys/stat.h> /* for flac_stat() */
> +#include <sys/utime.h> /* for flac_utime() */
> +#include <io.h> /* for flac_chmod() */
Nope. Bug from mass replace.
>
> As for calling __wgetmainargs, I have some concerns about the security
> implications:
> LoadLibrary("msvcrt.dll") <- Which msvcrt? Theoretical
security exploit.
There is msvcrt.dll in the System32 dir in all supported Windows 
systems. That is what the function targets, but of course LoadLibrary 
searches from exe's dir first. I think security exploit concerns are 
warrantless, if you can place malicious replacement c-runtime dll in the 
exe's path you have already won.
>
> I think it is best to link it directly, please use the following
> prototype and call it directly:
>
> ============================================> #ifdef _DLL
> #define CALL_DLLIMPORT __declspec(dllimport)
> #else
> #define CALL_DLLIMPORT
> #endif
> int __cdecl CALL_DLLIMPORT __wgetmainargs(int*, wchar_t***, wchar_t***,
> int, int*);
> ============================================>
> This should simplify the error handling logic and help against
> LoadLibrary handle leaks, though the leak should not be an issue in
> practice since it is only called once. The symbol should also be present
> in MSVCR* DLLs.
This alone does nothing. It requires linking with an object file that 
then deals with the function. If we link against msvcrt.lib the flac.exe 
binary will no longer be static and it won't work without external 
runtimes (which would also be loaded from the exe's dir if they exist 
there). Linking with msvcmrt.lib won't find the function and unicode 
version msvcurt.lib causes this error:
Error    1    error LNK2005: ___iob_func already defined in 
msvcurt.lib(MSVCR110.dll)    G:\test\LIBCMT.lib(_file.obj)    test
Error    2    error LNK1169: one or more multiply defined symbols 
found    G:\test\Release\test.exe    test
>
> In stat_utf8, dealing with 32bit/64bit time_t? The following check
> should really compile time rather than runtime:
> sizeof(*buffer) == sizeof(st)
Entire stat_utf8 function can be removed. The code was changed later to 
always use stat64 one. Though I'd assume compiler to optimize sizeof 
checks appropriately.