For better readability the patch is divided by 3 parts.
Part #1: for a bit better security replace
vsprintf(utmp, format, argptr)
with
vsnprintf_s(utmp, 32768, _TRUNCATE, format, argptr)
Part #2: potential memleak fixed: utf8argv[i] are not freed
when utf8argv itself is freed.
Part #3: 'if (ret != 0) break;' line seems redundant.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: win_utf8_io.zip
Type: application/zip
Size: 1209 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20140808/9d8220c8/attachment.zip
Some comments for patch #1, I chose the non-secure versions because they are faster and produce smaller binary. The functions where these printings are performed can't in my opinion ever exceed the safety margin of 32 KB. They print short help and error texts and occasionally filename, which with APIs is restricted to 260 characters. And you can't feed it longer faulty names either because maximum command line length is much shorter than 32 KB. Patch #2 is good. I was apparently not thinking when writing that. The break that patch #3 removes is there for a reason. If there is an error in string conversion there's no point in continuing the operation. All conversions are discarded if something failed so not exiting from the loop is wasted effort. On 8.8.2014 18:18, lvqcl wrote:> For better readability the patch is divided by 3 parts. > > Part #1: for a bit better security replace > vsprintf(utmp, format, argptr) > with > vsnprintf_s(utmp, 32768, _TRUNCATE, format, argptr) > > > Part #2: potential memleak fixed: utf8argv[i] are not freed > when utf8argv itself is freed. > > > Part #3: 'if (ret != 0) break;' line seems redundant. > > > _______________________________________________ > flac-dev mailing list > flac-dev at xiph.org > http://lists.xiph.org/mailman/listinfo/flac-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.xiph.org/pipermail/flac-dev/attachments/20140808/5f8724da/attachment.htm
Janne Hyv?rinen wrote:> The break that patch #3 removes is there for a reason. If there is an > error in string conversion there's no point in continuing the operation. > All conversions are discarded if something failed so not exiting from > the loop is wasted effort.Here is the current code: ret = 0; for (i=0; i<wargc; i++) { if ((utf8argv[i] = utf8_from_wchar(wargv[i])) == NULL) { ret = 1; break; } if (ret != 0) break; } I cannot see how the second break can happen. If utf8argv[i] != NULL then ret == 0; if utf8argv[i] == NULL then the first break exits the for(i=...) loop.
Janne Hyv?rinen wrote:> Some comments for patch #1, I chose the non-secure versions because they > are faster and produce smaller binary. The functions where these > printings are performed can't in my opinion ever exceed the safety > margin of 32 KB. They print short help and error texts and occasionally > filename, which with APIs is restricted to 260 characters. And you can't > feed it longer faulty names either because maximum command line length > is much shorter than 32 KB.I still like this patch. The secure versions might be slightly slower, but they are nowhere near the speed critical parts of the FLAC code. They also *document* the fact that they are safe so you really only need to look where they are called rather than thinking about maximum command line lengths and maximum file name lengths. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
lvqcl wrote:> For better readability the patch is divided by 3 parts. > > Part #1: for a bit better security replace > vsprintf(utmp, format, argptr) > with > vsnprintf_s(utmp, 32768, _TRUNCATE, format, argptr) > > > Part #2: potential memleak fixed: utf8argv[i] are not freed > when utf8argv itself is freed. > > > Part #3: 'if (ret != 0) break;' line seems redundant.Applied al three. Thanks. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/