April 2017 - Wine-Devel - list.winehq.org

Re: [PATCH 2/3] dwrite: Fix name section strings lookup

by Piotr Caban

On 04/27/17 16:52, Nikolay Sivov wrote: > On 27.04.2017 17:03, Piotr Caban wrote: >> Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> >> --- >> dlls/dwrite/opentype.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> > > The problem with this patch is that we want to continue and add as many > strings for different locales as possible. So the condition should set > 'exists' to TRUE after first pair was added successfully and never touch > it after. > > Yes, sorry, this patch is incorrect. I think that it would be nice to not fail in hard way if any of the names were decoded. Also I'll check why name decoding fails here. Thanks, Piotr

April 27, 2017

Re: [PATCH 2/3] dwrite: Fix name section strings lookup

by Nikolay Sivov

On 27.04.2017 17:03, Piotr Caban wrote: > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/dwrite/opentype.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > The problem with this patch is that we want to continue and add as many strings for different locales as possible. So the condition should set 'exists' to TRUE after first pair was added successfully and never touch it after.

April 27, 2017

Re: [PATCH 1/3] dwrite: Don't crash on uninitialized cached and factory fields in dwritefontface_Release

by Piotr Caban

On 04/27/17 16:41, Nikolay Sivov wrote: > On 27.04.2017 17:21, Piotr Caban wrote: >> On 04/27/17 16:18, Nikolay Sivov wrote: >>> By the way, did you check why init_font_data() fails? If it's because >>> opentype_get_font_familyname() fails, I'm interested in a log, or font >>> file that causes it. >> Yes, the opentype_get_font_familyname was failing. Patch 2 fixes the >> issue for the font I found it with. Do you want the log anyway? > > Yes, please send a log, or file itself directly to this address. Here's the log (I've copied the font to tests and reproduced the issue there, the original log is way to big).

April 27, 2017

Re: [PATCH 1/3] dwrite: Don't crash on uninitialized cached and factory fields in dwritefontface_Release

by Nikolay Sivov

On 27.04.2017 17:21, Piotr Caban wrote: > On 04/27/17 16:18, Nikolay Sivov wrote: >> By the way, did you check why init_font_data() fails? If it's because >> opentype_get_font_familyname() fails, I'm interested in a log, or font >> file that causes it. > Yes, the opentype_get_font_familyname was failing. Patch 2 fixes the > issue for the font I found it with. Do you want the log anyway? Yes, please send a log, or file itself directly to this address. > > Thanks, > Piotr >

April 27, 2017

Re: [PATCH 3/3] dwrite: Fix EUDC font collection use after free issue

by Nikolay Sivov

On 27.04.2017 17:04, Piotr Caban wrote: > > Without this patch factory_detach_fontcollection incorrectly releases > factory reference. > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/dwrite/dwrite_private.h | 2 +- > dlls/dwrite/font.c | 6 ++++-- > dlls/dwrite/main.c | 15 ++++++++------- > 3 files changed, 13 insertions(+), 10 deletions(-) > This makes sense, thanks. Please add some test for that in test_GetEudc*, similar to what test_object_lifetime() does in the beginning - testing that collection gets factory reference, but only once.

April 27, 2017

Re: [PATCH 2/3] dwrite: Fix name section strings lookup

by Nikolay Sivov

On 27.04.2017 17:03, Piotr Caban wrote: > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/dwrite/opentype.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > > - if (!(exists = opentype_decode_namerecord(header, storage_area, i, *strings))) > - continue; > + if ((exists = opentype_decode_namerecord(header, storage_area, i, *strings))) > + break; What happens exactly? I guess I'll need a font file for that too.

April 27, 2017

Re: [PATCH 1/3] dwrite: Don't crash on uninitialized cached and factory fields in dwritefontface_Release

by Piotr Caban

On 04/27/17 16:18, Nikolay Sivov wrote: > By the way, did you check why init_font_data() fails? If it's because > opentype_get_font_familyname() fails, I'm interested in a log, or font > file that causes it. Yes, the opentype_get_font_familyname was failing. Patch 2 fixes the issue for the font I found it with. Do you want the log anyway? Thanks, Piotr

April 27, 2017

Re: [PATCH 1/3] dwrite: Don't crash on uninitialized cached and factory fields in dwritefontface_Release

by Nikolay Sivov

On 27.04.2017 17:03, Piotr Caban wrote: > > The function may be called with the fields uninitialized from > create_fontface when init_font_data fails. > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/dwrite/font.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > > - factory_release_cached_fontface(This->cached); > - IDWriteFactory4_Release(This->factory); > + if (This->cached) > + factory_release_cached_fontface(This->cached); > + if (This->factory) > + IDWriteFactory4_Release(This->factory); Yes, this makes sense. The problem with this change is that 'cached' and 'factory' are not necessarily NULL on this failure path. I think at this point it would be better to use heap_alloc_zero(), and remove all explicit 0 initialization from create_fontface(). By the way, did you check why init_font_data() fails? If it's because opentype_get_font_familyname() fails, I'm interested in a log, or font file that causes it.

April 27, 2017

Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

by Hans Leidekker

On Thu, 2017-04-27 at 07:47 -0600, Erich E. Hoover wrote: > On Wed, Apr 26, 2017 at 3:39 AM, Hans Leidekker <hans(a)codeweavers.com> wrote: > > On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote: > >> +static int open_database( struct msidb_state *state ) > >> +{ > >> + LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT; > >> + UINT ret; > >> + > >> + ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle ); > > > > Databases opened in direct mode should always be committed. You should either do that > > in following patches, or open them in transacted mode. > > Thanks for the review! MsiDatabaseCommit is called by the > close_database routine (called as long as we actually open a > database). I did things this way so that if any of the requested > operations (in the next patches) fail then the database does not get > committed (to prevent creating "partially updated" databases). Is > there something that I can do to improve this or make that clearer? Opening the database in direct mode does not guarantee that partial updates are not written if you don't commit. MSDN warns that the database may become corrupt if you don't always commit before closing. You should to open the database in transacted mode if you want to avoid partially updated databases.

April 27, 2017

Re: [PATCH 1/13] msidb: Add stub tool for manipulating MSI databases.

by Erich E. Hoover

On Wed, Apr 26, 2017 at 3:39 AM, Hans Leidekker <hans(a)codeweavers.com> wrote: > On Sun, 2017-04-09 at 13:12 -0600, Erich E. Hoover wrote: >> +static int open_database( struct msidb_state *state ) >> +{ >> + LPCWSTR db_mode = state->create_database ? MSIDBOPEN_CREATEDIRECT : MSIDBOPEN_DIRECT; >> + UINT ret; >> + >> + ret = MsiOpenDatabaseW( state->database_file, db_mode, &state->database_handle ); > > Databases opened in direct mode should always be committed. You should either do that > in following patches, or open them in transacted mode. Thanks for the review! MsiDatabaseCommit is called by the close_database routine (called as long as we actually open a database). I did things this way so that if any of the requested operations (in the next patches) fail then the database does not get committed (to prevent creating "partially updated" databases). Is there something that I can do to improve this or make that clearer? Best, Erich

April 27, 2017