March 2016 - Wine-Devel - list.winehq.org

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Paul Gofman

On 03/29/2016 02:16 AM, Matteo Bruni wrote: > >> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc) >> +{ >> + while ((*ptr & 0xffff) == 0xfffe) >> + { >> + if (*(ptr + 1) == fourcc) >> + return ptr + 2; >> + ptr += 1 + (*ptr >> 16); >> + } >> + return NULL; >> +} > This is probably paranoid but in theory a broken or malicious effect > could send you rummaging outside of the bytecode buffer so all the > offsets should be checked. I don't think it's critical in practice > i.e. I'll probably sleep just fine if you decide to ignore this > issue... I will pass through bytecode size parameter and add a size check. There is D3DXGetShaderConstantTable function used in another function taking the same bytecode reference and doing comment search in a similar way, and it does not have size parameter, so it will crash anyway on insane bytecode. The same is for d3d shader create functions. But those functions do not modify the memory at bytecode, and segfault on read access cannot be used alone to inject anything. At the same time parse_preshader writes to that pointer a 0xfffe0000 constant (restoring a memory a bit later) to make it suitable D3DXGetShaderConstantTable, so actually adding these checks does not seem much paranoid. > > > It depends on what is coming up next but maybe it would make sense to > add an enum for "PRES_REGTAB_IMMED + 1" (incidentally, > PRES_REGTAB_FIRST is never used in this patch series). PRES_REGTAB_FIRST was used in tables free function. I now removed _FIRST/_LAST, used 0/_COUNT instead, and introduced PRES_REGTAB_FIRST_SHADER to use it in parse_param_eval instead of 'PRES_REGTAB_IMMED + 1'. Please note that I introduced PRES_REGTAB_FIRST/_LAST in v3 patch series based on comments on v1, and should issue a possible endless loop warning :). I suggest that if there is still any concern regarding this enum maybe we could confirm its final form at this point? The current enum after my last edits looks like this: enum pres_reg_tables { PRES_REGTAB_IMMED, /* immediate double constants from CLIT */ PRES_REGTAB_CONST, PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_TEMP, PRES_REGTAB_COUNT, PRES_REGTAB_FIRST_SHADER = PRES_REGTAB_CONST, }; > > + if (FAILED(regstore_alloc_table(&peval->pres.regs, i))) > + { > + hr = E_OUTOFMEMORY; > + goto err_out; > + } > + } > + > + if (TRACE_ON(d3dx)) > + dump_bytecode(byte_code, byte_code_size); > + > + *peval_out = peval; > + TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out); > Same with this. This one in particular doesn't seem worth keeping. Maybe we can leave this in place? I changed this to TRACE("Created param_eval %p.\n", *peval_out). I saw similar traces many times in d3d code and they are extremely helpful when we need to find a creation log for object we see used somewhere.

March 29, 2016

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Matteo Bruni

2016-03-29 1:39 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > I intentionally added this TRACE with output pointer as otherwise I > cannot find init trace related to particular preshader. E. g., if I have > some weirdness or error message when param_eval is used much later > (among a hundred of other param evals), I want to find the log records > where this preshader was created which has disassembly and binary blob. > Without this TRACE I have no sure way to match the used preshader with > its parsing trace. So unless you are strictly opposed to this TRACE I > would keep it to make future bug hunting easier. I'm okay with printing the pointer or similar info but that kind of trace is pretty ugly. You could trace the pointer when you allocate the struct, or together with the struct state pointer / the state parameter in the caller. I guess you can also leave the trace there, if it says something like: TRACE("Created parameter evaluator %p.\n", *peval_out); BTW, notice that you are never checking the return value from d3dx_create_param_eval(). You should either do that or remove the return value from the function altogether.

March 29, 2016

Re: Need some advice on shell32

by Mathy Vanvoorden

> I think that shell32 in shfldr_unixfs.c should check this setting > instead. Since it's skipping ntdll and using the Unix file APIs > directly, it should report the same thing to applications that ntdll > does, namely that dotfiles are not hidden files. > I have been looking at this just now and would like some more advice. In ntdll a number of things are done: * The registry key is only read once (I presume for performance reasons) * The DIR_is_hidden_file is called for every file * Folders like /dev, /proc, wine config, ... are always hidden It seems strange to me to reimplement all this logic and I figure it is desired to have the same behavior in both components so I would like to abstract it to a common base, however: * What is the best place to do this? To me somewhere in the libs seemed logical but I noticed that nowhere in there registry keys are being read so am I wrong thinking this is undesired behavior there? Is there a dll I could put this common code? * ntdll and shell32 use different functions to access registry keys, would it be a problem if the common code would only use the RegOpenKey functions? br, Mathy

March 29, 2016

Re: [PATCH] d3drm: Partially Implement IDirect3DRM*::LoadTexture(v3).

by Henri Verbeet

On 28 March 2016 at 20:02, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > v3: Add bitmap verification, use InitFromFile and other minor improvements. > Implementing InitFromFile() should be a separate patch from implementing LoadTexture(). > if (FAILED(hr = d3drm_texture_create(&object))) > return hr; > > + if (FAILED(hr = IDirect3DRMTexture_InitFromFile(&object->IDirect3DRMTexture_iface, filename))) > + { > + d3drm_texture_destroy(object); > + return hr; > + } > + > + object->d3drm = iface; > + IDirect3DRM_AddRef(iface); It would be good to back this up with tests. It would seem more natural to pass the IDirect3DRM interface to d3drm_texture_create(). If that's how it's supposed to work, you can make that change in a separate patch. > @@ -39,6 +40,9 @@ struct d3drm_texture > IDirect3DRMTexture IDirect3DRMTexture_iface; > IDirect3DRMTexture2 IDirect3DRMTexture2_iface; > IDirect3DRMTexture3 IDirect3DRMTexture3_iface; > + IDirect3DRM *d3drm; > + D3DRMIMAGE *image; > + BOOL internal_image_created; You don't really need internal_image_created, you can probably just add a destroy callback. > +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, > + BOOL load_upside_down) DECLSPEC_HIDDEN; > + > +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down) DECLSPEC_HIDDEN; These are internal helper functions now. > @@ -4109,36 +4111,94 @@ static void test_load_texture(void) > > hr = IDirect3DRM_LoadTexture(d3drm1, filename, &texture1); > ok(SUCCEEDED(hr), "Test %u: Failed to load texture, hr %#x.\n", i, hr); > + ref2 = get_refcount((IUnknown *)d3drm1); > + ok(ref2 > ref1, "Test %u: expected ref2 > ref1, got ref1 = %u, ref2 = %u.\n", i, ref1, ref2); > + > + hr = IDirect3DRMTexture_InitFromFile(texture1, filename); > + ok(hr == D3DRMERR_BADOBJECT, "Test %u: Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", i, hr); > + /* It's odd, but InitFromFile seems to AddRef IDirect3DRM even if it fails. */ So a test for the refcounts would be appropriate here. > +HRESULT d3drm_texture_load(struct d3drm_texture *texture, IDirect3DRM *d3drm, const char *path, BOOL load_upside_down) > +{ This is an internal helper function now and should be static. You never use "d3drm". > +D3DRMIMAGE *d3drm_create_image(unsigned char *buffer, BITMAPINFO *info, BOOL palette, BOOL upside_down) > +{ > + D3DRMPALETTEENTRY *colors; > + D3DRMIMAGE *image; > + BOOL black_used = FALSE; > + LONG w = info->bmiHeader.biWidth; > + LONG h = abs(info->bmiHeader.biHeight); > + int i, j, k; > + unsigned int idx, buffer1_idx; > + unsigned char *buffer1; > + int bpp = info->bmiHeader.biBitCount == 24 ? 32 : info->bmiHeader.biBitCount; > + int bpl = palette ? w : ((w + 3) & ~3) * bpp / 8; > + int num_colors = 0; There's not reason to use signed types for any of these. > + struct colors_24bpp > + { > + BYTE red; > + BYTE green; > + BYTE blue; > + } *color; > + color = (struct colors_24bpp *)buffer; > + > + if (bpl * h > UINT_MAX) > + { > + return NULL; > + } This doesn't work. If "bpl * h" would be larger than UINT_MAX it would overflow before the comparison. In fact, strictly speaking things become implementation defined above INT_MAX already, since you're using signed types for "bpl" and "h". What you'd typically do is something like "if (h > UINT_MAX / bpl)". Note that the calculation of "bpl" itself can overflow as well. So you'd need something like "if (align(w, 4) > UINT_MAX / (bpp / 8))" there as well. > + if (!(colors = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*colors)))) > + { > + WARN("Not enough memory to allocate palette, returning NULL.\n"); > + return NULL; > + } > + > + > + if (!(image = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*image)))) > + { > + WARN("Not enough memory to allocate image struct, returning NULL.\n"); > + return NULL; > + } You probably don't need HEAP_ZERO_MEMORY for either of these. You certainly don't for "colors", and you should probably just initialise the few fields in "image" that you don't already. You also leak "colors" if the allocation for "image" fails. The indentation for the WARN is wrong. > + if (!(image->buffer1 = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, bpl * h))) > + { > + WARN("Not enough memory to allocate image buffer, returning NULL.\n"); > + return NULL; > + } You don't need HEAP_ZERO_MEMORY here if you're going to memset() it again below. You leak "colors" and "image" on failure. > + buffer1 = image->buffer1; > + memset(buffer1, 255, sizeof(unsigned char) * bpl * h); I think 0xff is clearer than 255. The "sizeof(unsigned char)" doesn't add much. > + if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, num_colors * sizeof(*image->palette)))) > + { > + WARN("Not enough memory to allocate image palette, returning NULL.\n"); > + return NULL; > + } Like above, you use HEAP_ZERO_MEMORY here, but immediately memcpy() over it. You leak on error. > + if (!(image->palette = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 256 * sizeof(*image->palette)))) > + { > + WARN("Not enough memory to allocate image palette, returning NULL.\n"); > + return NULL; > + } Leak on error. > static HRESULT WINAPI d3drm_texture1_InitFromFile(IDirect3DRMTexture *iface, const char *filename) > { > - FIXME("iface %p, filename %s stub!\n", iface, debugstr_a(filename)); > + struct d3drm_texture *object = impl_from_IDirect3DRMTexture(iface); I'd suggest "texture" instead of "object". > + if (object->d3drm) > + { > + /* InitFromFile seems to AddRef IDirect3DRM even if it fails. */ > + IDirect3DRM_AddRef(object->d3drm); > + } This is very suspicious, since you always assign "object->d3drm" after calling InitFromFile(). It's not backed up by tests either.

March 29, 2016

Re: [v2 PATCH 2/3] d2d1/tests: Basic test for ID2D1DCRenderTarget creation

by Nikolay Sivov

On 29.03.2016 12:15, Henri Verbeet wrote: > On 29 March 2016 at 01:21, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >> + static D2D1_PIXEL_FORMAT invalid_formats[] = >> + { >> + { DXGI_FORMAT_UNKNOWN, D2D1_ALPHA_MODE_PREMULTIPLIED }, >> + { DXGI_FORMAT_B8G8R8A8_UNORM, D2D1_ALPHA_MODE_UNKNOWN }, >> + { DXGI_FORMAT_R8G8B8A8_UNORM, D2D1_ALPHA_MODE_PREMULTIPLIED }, >> + }; > I should have noticed before, but this should be const. > > Thanks, I sent all 3 of them so testbot is not confused, only 2/3 was changed.

March 29, 2016

Re: [v2 PATCH 2/3] d2d1/tests: Basic test for ID2D1DCRenderTarget creation

by Henri Verbeet

On 29 March 2016 at 01:21, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > + static D2D1_PIXEL_FORMAT invalid_formats[] = > + { > + { DXGI_FORMAT_UNKNOWN, D2D1_ALPHA_MODE_PREMULTIPLIED }, > + { DXGI_FORMAT_B8G8R8A8_UNORM, D2D1_ALPHA_MODE_UNKNOWN }, > + { DXGI_FORMAT_R8G8B8A8_UNORM, D2D1_ALPHA_MODE_PREMULTIPLIED }, > + }; I should have noticed before, but this should be const.

March 29, 2016

Re: [PATCH 2/5] ddraw/tests: Add a test for color varyings clamping.

by Henri Verbeet

On 28 March 2016 at 23:03, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > + hr = IDirect3DDevice7_GetDirect3D(device, &d3d); > + ok(SUCCEEDED(hr), "Failed to get d3d interface, hr %#x.\n", hr); I suppose this is borrowed from test_shademode() where it's used to create the vertex buffers, but you're not using "d3d" anywhere here.

March 29, 2016

Re: [PATCH] msvcr120/tests: Add critical section test.

by Francois Gouget

On Tue, 22 Mar 2016, Piotr Caban wrote: > On 03/22/16 17:28, YongHao Hu wrote: > > How long is a test running acceptable? I want to know a appropriate > > standard. > There's no rule for that. In most cases 100ms wait is long enough to > make sure something works. Of course if the test is useful and it can't > be written to end quickly it can take much longer. I'm trying to avoid > tests that calls Sleep if possible. The TestBot and WineTest impose a 2 minute limit on individual tests (like msvcr120:msvcr120 or kernel32:process). Furthermore the TestBot imposes a 30 minute limit on the full test suite. At 585 tests (and growing all the time), this only allows an average of about 3 seconds per test. But of course some tests take much longer so the remainder must be much faster to bring the average down. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ If you think the whole world revolves around you, quit staring at the GPS display while driving.

March 29, 2016

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Paul Gofman

On 03/29/2016 02:16 AM, Matteo Bruni wrote: > >> + if (FAILED(regstore_alloc_table(&peval->pres.regs, i))) >> + { >> + hr = E_OUTOFMEMORY; >> + goto err_out; >> + } >> + } >> + >> + if (TRACE_ON(d3dx)) >> + dump_bytecode(byte_code, byte_code_size); >> + >> + *peval_out = peval; >> + TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out); > Same with this. This one in particular doesn't seem worth keeping. I intentionally added this TRACE with output pointer as otherwise I cannot find init trace related to particular preshader. E. g., if I have some weirdness or error message when param_eval is used much later (among a hundred of other param evals), I want to find the log records where this preshader was created which has disassembly and binary blob. Without this TRACE I have no sure way to match the used preshader with its parsing trace. So unless you are strictly opposed to this TRACE I would keep it to make future bug hunting easier. The TRACE on error return a few lines below is a leftover.

March 28, 2016

Re: [v7 2/7] d3dx9: Implement preshader parsing.

by Matteo Bruni

2016-03-28 9:20 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > Changes > - Changed opcode function to take double arguments and return double Good catch. > +static struct op_info pres_op_info[] = > +{ > + {0x000, "nop", 0, 0, NULL }, /* PRESHADER_OP_NOP */ > + {0x100, "mov", 1, 0, pres_mov}, /* PRESHADER_OP_MOV */ > +}; I think this can be const. > +struct d3dx_pres_ins > +{ > + enum pres_ops op; > + /* first input argument is scalar, > + scalar component is propagated */ > + BOOL scalar_op; > + unsigned int component_count; > + struct d3dx_pres_operand inputs[3]; The maximum number of input operands could use a #define. Not a huge deal though. > +static void regstore_free_tables(struct d3dx_regstore *rs) > +{ > + unsigned int i; > + > + for (i = PRES_REGTAB_FIRST; i <= PRES_REGTAB_LAST; ++i) > + { > + if (rs->tables[i]) > + HeapFree(GetProcessHeap(), 0, rs->tables[i]); > + if (rs->table_value_set[i]) > + HeapFree(GetProcessHeap(), 0, rs->table_value_set[i]); > + } > +} You don't need to check for non-NULL, HeapFree() handles NULL arguments just fine. > +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc) > +{ > + while ((*ptr & 0xffff) == 0xfffe) > + { > + if (*(ptr + 1) == fourcc) > + return ptr + 2; > + ptr += 1 + (*ptr >> 16); > + } > + return NULL; > +} This is probably paranoid but in theory a broken or malicious effect could send you rummaging outside of the bytecode buffer so all the offsets should be checked. I don't think it's critical in practice i.e. I'll probably sleep just fine if you decide to ignore this issue... > +static unsigned int *parse_pres_arg(unsigned int *ptr, struct d3dx_pres_operand *opr) > +{ > + static const enum pres_reg_tables reg_table[8] = > + { > + PRES_REGTAB_COUNT, PRES_REGTAB_IMMED, PRES_REGTAB_CONST, PRES_REGTAB_COUNT, > + PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_REG Matter of taste, so feel free to ignore this, but PRES_REGTAB_REG sounds a bit vague to me. I'd go with something like PRES_REGTAB_TEMP. > + if (input_count != pres_op_info[i].input_count) > + { > + FIXME("Actual input args %u, expected %u, instruction %s.\n", input_count, > + pres_op_info[i].input_count, pres_op_info[i].mnem); > + return NULL; > + } > + for (i = 0; i < input_count; ++i) > + { > + ptr = parse_pres_arg(ptr, &ins->inputs[i]); > + if (!ptr) > + return NULL; > + } Another pretty paranoid potential check, here you could verify that input_count is < ARRAY_SIZE(ins->inputs). Of course that could only fail if we encounter some instruction with more than 3 components AND we update pres_op_info BUT we forget to update struct d3dx_pres_ins, but I don't think it would hurt to be safe... > + for (i = 0; i < pres->ins_count; ++i) > + { > + unsigned int table; > + > + for (j = 0; j < pres_op_info[pres->ins[i].op].input_count; ++j) > + { > + table = pres->ins[i].inputs[j].table; > + max_offset = get_reg_offset(table, pres->ins[i].inputs[j].offset + pres->ins[i].component_count - 1); > + pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1); > + } > + table = pres->ins[i].output.table; > + max_offset = get_reg_offset(table, pres->ins[i].output.offset + pres->ins[i].component_count - 1); > + pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1); > + } > + update_table_sizes_consts(pres->regs.table_sizes, &pres->inputs); It probably makes sense to add a small update_table_size() helper (or similar), the "same" 3 lines are repeated in the loop for the input operands, for the output operand and in update_table_sizes_consts(). > + for (i = PRES_REGTAB_IMMED + 1; i <= PRES_REGTAB_LAST; ++i) It depends on what is coming up next but maybe it would make sense to add an enum for "PRES_REGTAB_IMMED + 1" (incidentally, PRES_REGTAB_FIRST is never used in this patch series). > + { > + TRACE("table_sizes[%u] %u.\n", i, peval->pres.regs.table_sizes[i]); That trace looks a bit like a debugging leftover. > + if (FAILED(regstore_alloc_table(&peval->pres.regs, i))) > + { > + hr = E_OUTOFMEMORY; > + goto err_out; > + } > + } > + > + if (TRACE_ON(d3dx)) > + dump_bytecode(byte_code, byte_code_size); > + > + *peval_out = peval; > + TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out); Same with this. This one in particular doesn't seem worth keeping. > + return D3D_OK; > + > +err_out: > + FIXME("Error creating parameter evaluator.\n"); > + d3dx_free_param_eval(peval); > *peval_out = NULL; > - return E_NOTIMPL; > + TRACE("retval %u, *peval_out %p.\n", hr, *peval_out); > + return hr; > +} You could print the HRESULT in the FIXME (e.g. FIXME("Error creating parameter evaluator, returning %#x.\n", hr)). Tracing the value of *peval_out doesn't seem particularly worthwhile. > +static void d3dx_free_const_tab(struct d3dx_const_tab *ctab) > +{ > + if (ctab->inputs) > + HeapFree(GetProcessHeap(), 0, ctab->inputs); > + if (ctab->inputs_param) > + HeapFree(GetProcessHeap(), 0, ctab->inputs_param); > + if (ctab->ctab) > + ID3DXConstantTable_Release(ctab->ctab); > +} > + > +static void d3dx_free_preshader(struct d3dx_preshader *pres) > +{ > + if (pres->ins) > + HeapFree(GetProcessHeap(), 0, pres->ins); > + > + regstore_free_tables(&pres->regs); > + d3dx_free_const_tab(&pres->inputs); > } You can drop all those ifs (I think there are a few more of them in the middle of the patch I haven't specifically mentioned).

March 28, 2016