January 2015 - Wine-Devel - list.winehq.org

Re: [resend 04/10] reg: Add sane_path function to do preliniary key validation
by Stefan Dösinger Jan. 6, 2015

Jan. 6, 2015

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, I was on holiday when you sent these patches, sorry for the late review. Am 2014-12-13 um 18:06 schrieb Jonathan Vollebregt: > +static LSTATUS sane_path(const WCHAR *key) > +{ > + if (key[0] == '\\' && key[1] == '\\' && key[2] != '\\') > + return ERROR_NO_REMOTE; > + > + return ERROR_SUCCESS; > +} This adds (or rather extends) a possible out of bound array access. Otherwise these patches look reasonable to me and worth review by others. A general note on the error handling code for other reviewers: Native reg.exe has very limited error reporting. With a few exceptions it just prints a generic message like "ERROR: INVALID SYNTAX" or something similar. I guess there are also some possible improvements on the output code as Bruno mentioned, but I think patch 3 is OK until (if ever) we have unified console output handling for all command line programs. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUrEB5AAoJEN0/YqbEcdMw8bUP/1dhlLDCE2kOZhLkhVXg0RVa BckpiGygdwfIIvGoyCaj6ObtgvrnYr7OMzZsruytK/X5UUvl+KsObctp8g1zjNY1 dFPQeBsra9TRtbtKTMX2pIdZS1wSNwWaOYrdZcKn5IkQfDWN0rwdKNujT8pAf+Ik 8dpFJUYtMjd6N0avPqphglRbW8rFhLhqYOlYuPjvzuEtBM11vGfy9mAp+sPA1ekX 8sRqGtn65w1JsT3cg054DvtY2xHNmRE5eTnrsbL7gqD/oljdPyU470eY6Vwofo3Z frAF8OKemJsU6MrxZLXe1US5GzyvvCRRpIBNcfrO6qzTTdio0/LBe77VDq9J0jAa GUWYFUtxIXw7Ccw1tNrxYp1JPo9bZnk5yONMMEAPfw2ij2iQCTvpv0lY9Kk58mqF kKzWrqzhVdi5aFGOGhnmrk7NZ4l8paA7Gk2S1c8IukWubww0hlOclrhrWDYpuEfJ ZKyGG8jFUvSjW3iTEES7NHMnCd+lWgaf+3TODrSGD5J/wdP6GQT9S78/HZ+SbM8m fxnvnDLylq9arQm3bh0wQy9ZoMCTMx4YtbTouQBpLZBDKAgZNK1BUC5sxP1NlO9P eP5JAZFHTzpGXCADkL+4OI4+ImG9mAMWGYytHGEQ3kwvAucNwTWP00twzMbSjHEX +8k+frQxyupijPZrL4vo =PJFk -----END PGP SIGNATURE-----

1 1

Re: [PATCH 2/5] winemac: Implement wglCreateContextAttribsARB.
by cdavis5x＠gmail.com Jan. 6, 2015

Jan. 6, 2015

> On Jan 5, 2015, at 9:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > > As an aside, reported WGL extensions don't depend on the specific > GL context (e.g. WGL_ARB_pbuffer is reported as supported even on core > profile contexts). Do real Windows drivers behave like this? > @@ -1272,6 +1273,175 @@ static BOOL init_gl_info(void) […] > +/********************************************************************** > + * create_context > + */ > +static BOOL create_context(struct wgl_context *context, CGLContextObj share, BOOL core) > +{ […] > + attribs[n++] = kCGLPFAAuxBuffers; > + attribs[n++] = pf->aux_buffers; You must reject any pixel format with >0 auxiliary buffers when creating a core profile context. CGL will specifically fail the ChoosePixelFormat (with error 10000, kCGLBadAttribute) if you specify both a GL version >= 3.2 and a non-zero number of auxiliary buffers. > + > + attribs[n++] = kCGLPFAColorSize; > + attribs[n++] = color_modes[pf->color_mode].color_bits; > + attribs[n++] = kCGLPFAAlphaSize; > + attribs[n++] = color_modes[pf->color_mode].alpha_bits; > + if (color_modes[pf->color_mode].is_float) > + attribs[n++] = kCGLPFAColorFloat; > + > + attribs[n++] = kCGLPFADepthSize; > + attribs[n++] = pf->depth_bits; > + > + attribs[n++] = kCGLPFAStencilSize; > + attribs[n++] = pf->stencil_bits; > + > + if (pf->stereo) > + attribs[n++] = kCGLPFAStereo; > + > + if (pf->accum_mode) > + { > + attribs[n++] = kCGLPFAAccumSize; > + attribs[n++] = color_modes[pf->accum_mode - 1].color_bits; > + } You must also reject any pixel format with an accumulation buffer when creating a core profile context, for the same reason. > + > + /* Explicitly requesting pbuffers in CGLChoosePixelFormat fails with core contexts. */ > + if (pf->pbuffer && !core) > + attribs[n++] = kCGLPFAPBuffer; > + > + if (pf->sample_buffers && pf->samples) > + { > + attribs[n++] = kCGLPFASampleBuffers; > + attribs[n++] = pf->sample_buffers; > + attribs[n++] = kCGLPFASamples; > + attribs[n++] = pf->samples; > + } > + > + if (pf->backing_store) > + attribs[n++] = kCGLPFABackingStore; > + > + if (core) > + { > + attribs[n++] = kCGLPFAOpenGLProfile; > + attribs[n++] = (int)kCGLOGLPVersion_3_2_Core; > + } There’s a constant for requesting a 4.x core context, too. (But it’s only defined in the 10.9 and 10.10 SDKs.) You might consider using it if the requested version is >= 4.0. That way, creation will fail if the system doesn’t support it. > + > + attribs[n] = 0; > + > + err = CGLChoosePixelFormat(attribs, &pix, &virtualScreens); > + if (err != kCGLNoError || !pix) > + { > + WARN("CGLChoosePixelFormat() failed with error %d %s\n", err, CGLErrorString(err)); > + SetLastError(ERROR_INVALID_OPERATION); This is somewhat nitpicking, but one thing you might consider is setting the last error based on what CGL returned. For example, if you get the error kCGLBadAlloc, you could set the last error to ERROR_NO_SYSTEM_RESOURCES. > + return FALSE; > + } > + > + err = CGLCreateContext(pix, share, &context->cglcontext); > + CGLReleasePixelFormat(pix); > + if (err != kCGLNoError || !context->cglcontext) > + { > + context->cglcontext = NULL; > + WARN("CGLCreateContext() failed with error %d %s\n", err, CGLErrorString(err)); > + SetLastError(ERROR_INVALID_OPERATION); Ditto. > @@ -2076,6 +2246,133 @@ cant_match: […] > +/*********************************************************************** > + * macdrv_wglCreateContextAttribsARB > + * > + * WGL_ARB_create_context: wglCreateContextAttribsARB > + */ > +static struct wgl_context *macdrv_wglCreateContextAttribsARB(HDC hdc, > + struct wgl_context *share_context, > + const int *attrib_list) > +{ […] > + if (major > 3 || (major == 3 && minor >= 2)) > + { > + if (!(flags & WGL_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB)) > + { > + WARN("OS X only supports forward-compatible 3.2+ contexts\n"); > + SetLastError(ERROR_INVALID_VERSION_ARB); > + return NULL; > + } Just so you know, a side effect of this is that our GL 3.x tests get skipped here, because they don’t specify the FOWARD_COMPATIBLE bit. Also, you should consider rejecting the DEBUG flag, if it’s set: OS X never returns that flag. (Or do you want to hook glGetInteger(3G) to return the debug flag if it’s set?) > + if (profile != WGL_CONTEXT_CORE_PROFILE_BIT_ARB) > + { > + WARN("Compatibility profiles for GL version >= 3.2 not supported\n"); > + SetLastError(ERROR_INVALID_PROFILE_ARB); > + return NULL; > + } > + core = TRUE; > + } > + else if (major == 3) > + { > + WARN("OS X doesn't support 3.0 or 3.1 contexts\n"); > + SetLastError(ERROR_INVALID_VERSION_ARB); > + return NULL; > + } I think we can support requests for 3.1 contexts, if the FORWARD_COMPATIBLE bit is set; we just won’t advertise GL_ARB_compatibility. Chip

3 5

Re: loader: More generic pre_exec for Linux (try 2)
by André Hentschel Jan. 6, 2015

Jan. 6, 2015

Am 29.12.2014 um 23:03 schrieb André Hentschel: > --- > loader/main.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) [After a small IRC discussion] My main goal is to also get the 2G/2G vmsplit warning on ARM, too. (and potential other platforms) There are still many ARM kernels out there with a 2G split. The problem is that Wine can't start on 2G/2G, it seems there are too much assumptions in the code for a 3G split. e.g. for shared user data. We can't allocate 0x7ffe0000-0x7fff0000 on 2G split, most likely something else is already there, idk

2 3

Re: dpnet: Implement IDirectPlay8Address GetComponentByIndex (try 4)
by Stefan Dösinger Jan. 6, 2015

Jan. 6, 2015

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, You have style inconsistencies in the patch. I won't point them out individually and just focus on suggestions on the code itself. Overall: Is there a way to remove components? I don't see a method that supports that, and AddComponent with a NULL pointer returns an error, so I guess components can't be removed. Am 2015-01-06 um 08:59 schrieb Alistair Leslie-Hughes:> +static void add_component(IDirectPlay8AddressImpl *This, struct component *item) > +{ > + if(!This->comp_count) > + This->components = HeapAlloc( GetProcessHeap(), 0, sizeof(struct component *) * This->comp_count+1 ); > + else > + This->components = HeapReAlloc( GetProcessHeap(), 0, This->components, sizeof(struct component *) * This->comp_count+1 ); How many components are usually added? If it is just a small number (say, < 10) this is OK. Otherwise it would be more efficient to grow the array by a factor (e.g. duplicate the array size each time you grow it), so you make growing an O(log(n)) instead of O(n). > + int i; > + struct component *entry; > > - LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &This->components, struct component, entry) > + for(i=0; i < This->comp_count; i++) Mismatching data type. This->comp_count is a DWORD, i is an int. Please use DWORD consistently. (Using DWORD for the component is mandated by the parameters to GetComponentByIndex and return value of GetNumComponents) > + switch (entry->type) > + { > + case DPNA_DATATYPE_DWORD: > + memcpy(pvBuffer, &entry->data.value, sizeof(DWORD)); > + break; > + case DPNA_DATATYPE_GUID: > + memcpy(pvBuffer, &entry->data.guid, sizeof(GUID)); > + break; > + case DPNA_DATATYPE_STRING: > + memcpy(pvBuffer, &entry->data.string, entry->size); > + break; > + case DPNA_DATATYPE_STRING_ANSI: > + memcpy(pvBuffer, &entry->data.ansi, entry->size); > + break; > + case DPNA_DATATYPE_BINARY: > + memcpy(pvBuffer, &entry->data.binary, entry->size); > + break; > + } You can use assignments for DPNA_DATATYPE_DWORD and DPNA_DATATYPE_GUID. The same applies to the existing code in GetComponentByName. AddComponent already uses assignments. > --- a/dlls/dpnet/dpnet_private.h > +++ b/dlls/dpnet/dpnet_private.h > @@ -94,7 +94,8 @@ struct IDirectPlay8AddressImpl > GUID SP_guid; > BOOL init; > > - struct list components; > + struct component **components; > + DWORD comp_count; Do you need the extra indirection? If the number of components is small you can work without it I think. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUq/bOAAoJEN0/YqbEcdMwOnMP/RnnAWVfyTuK2ABnRqDLW9/1 DydTHhjeOpm1FDKJTy7ebzqU76HDL7YWJOqwSNYPhtr8e3omXt5nApptwVjOBst0 lpRxDju4sPGo9GYePy5w39TI/w78gPLEGLVHpIiV5GlMz+YRpgxKI05Q90jkGtfo whPpQ9ns1xiXz+d07iJnPn8GnEjWeTkIN2NnY6Nj+ARQ8rmUNSn3YR4ZGtZfuzrt nKUurOt04FUAqQzDNYNbdtpybYQq7SSVQr8vWpJAJOpedq7oNyCEdjC8Zl5Yitl5 bg6a4oTvl7PgGmYOSvNVzsh6L0dYv0HpjLZsfLUo97ULXRsCFgfX8yNogrCSq8o0 2KoZSWEhehn1RV+WCywMbxHxly5ZvkyEr9g9/h+8+7cpPrdg8TeoaMUjtjUxrqra fLDfLtGnMGs4DGiFmleRR3lBFmr8UMVkP4/YLEJ1HPKNdAAwXfQso6/ODmdvC78W 5sRfY/pSlLEn93szGfx+hunpUX30c2XkGmuBElLJQsQgVeoijmsNhUNOLUdhTx9k wPhslaQ+HCtR76KVghvQzN3iBHPuO+O0dfH4PWz5/7Grq5/pIhfbWcMCVJJM3d9u CNjKAOkw+NzPPsdLyJZsmMWWvv+BJ039l15xjMTmWxG2uH/QgaiBtvDyD2/u3KxJ MgsfOYAWEYondC/Jllaz =uIjr -----END PGP SIGNATURE-----

1 0

Re: windowscodecs: DibDecoder_CreateInstance() is unused so remove it.
by Vincent Povirk Jan. 6, 2015

Jan. 6, 2015

The original intent was that this could be used by d3dx9 to implement reading of the DIB format, but I didn't have the knowledge of d3dx9 to test that this really does what is needed.

2 1

Re: [PATCH] xaudio2_7: add stub DLL
by Alexandre Julliard Jan. 6, 2015

Jan. 6, 2015

Mark Harmstone <hellas(a)burntcomma.com> writes: > diff --git a/dlls/xaudio2_7/xaudio2_7.spec b/dlls/xaudio2_7/xaudio2_7.spec > new file mode 100644 > index 0000000..e69de29 Please list stubs for all the exported functions. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [PATCH 3/5] opengl32: Fix extension checks on OpenGL core profile contexts.
by Henri Verbeet Jan. 6, 2015

Jan. 6, 2015

On 5 January 2015 at 17:17, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > static BOOL has_extension( const char *list, const char *ext, size_t len ) > { > + if (!list) > + { > + const struct opengl_funcs *funcs = NtCurrentTeb()->glTable; > + const char *gl_ext; > + unsigned int i; > + GLint extensions_count; > + > + glGetIntegerv(GL_NUM_EXTENSIONS, &extensions_count); > + for (i = 0; i < extensions_count; ++i) > + { > + gl_ext = (const char *)funcs->ext.p_glGetStringi(GL_EXTENSIONS, i); > + if (!strncmp(gl_ext, ext, len) && !gl_ext[len]) > + return TRUE; > + } > + return FALSE; > + } > + If I'm reading this correctly, this effectively ignores DisabledExtensions for anything newer than GL 3.0. (And at least as far as wglGetProcAddress() is concerned it affects both compatibility and core contexts.) > + gl_version = (const char *)glGetString(GL_VERSION); > + sscanf(gl_version, "%u", &major); > + if (major >= 3) > + { It seems tempting to just flag the context in wglCreateContextAttribsARB() under the appropriate conditions, perhaps in a similar way to wgl_handle_type. Not sure if it would really make it better though. As an aside, note that winex11.drv also uses "glGetString(GL_EXTENSIONS);" in X11DRV_WineGL_InitOpenglInfo().

3 4

Re: [PATCH 2/5] winemac: Implement wglCreateContextAttribsARB.
by Ken Thomases Jan. 5, 2015

Jan. 5, 2015

On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > +#if !defined(MAC_OS_X_VERSION_10_7) || MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7 > + if (core) > + { > + WARN("OS X version >= 10.7 is required to be able to create core contexts\n", > + err, CGLErrorString(err)); The "err, CGLErrorString(err)" arguments are superfluous. > + return FALSE; > + } > + if (core) > + { > + attribs[n++] = kCGLPFAOpenGLProfile; > + attribs[n++] = (int)kCGLOGLPVersion_3_2_Core; > + } These lines won't compile when the SDK is earlier than 10.7. So, they also need to be guarded with the appropriate #if. > + case WGL_CONTEXT_PROFILE_MASK_ARB: > + if ((value & ~VALID_PROFILE_BITS_MASK) || !value || > + (value & VALID_PROFILE_BITS_MASK) == VALID_PROFILE_BITS_MASK) I think it would be better to simply do: if (value != WGL_CONTEXT_CORE_PROFILE_BIT_ARB && value != WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB) Or did I miss something about the logic you used? > + { > + WARN("WGL_CONTEXT_PROFILE_MASK_ARB bits %#x invalid\n", value); > + SetLastError(ERROR_INVALID_PROFILE_ARB); > + return NULL; > + } > + profile = value; > + break; > + register_extension("WGL_ARB_create_context"); > + register_extension("WGL_ARB_create_context_profile"); > + opengl_funcs.ext.p_wglCreateContextAttribsARB = macdrv_wglCreateContextAttribsARB; These should only be advertised when building against the 10.7 SDK or later. So, this should be guarded by the SDK #if directive. Also, given that it's possible to build against a 10.7+ SDK but target deployment back to 10.6, I'm tempted to say we should determine if core profiles are actually available and only advertise those extensions if so. Basically, call CGLChoosePixelFormat() with the simplest set of attributes that specify the core profile and see if it succeeds. -Ken

2 2

Re: [PATCH 5/5] wined3d: Prefer the core glActiveTexture function.
by Henri Verbeet Jan. 5, 2015

Jan. 5, 2015

On 5 January 2015 at 17:17, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > +#define MAP_GL_FUNCTION(core_func, ext_func) \ > + { \ > + if (!gl_info->gl_ops.ext.p_##core_func) \ > + gl_info->gl_ops.ext.p_##core_func = gl_info->gl_ops.ext.p_##ext_func; \ > + } > + > + MAP_GL_FUNCTION(glActiveTexture, glActiveTextureARB); > +#undef MAP_GL_FUNCTION > + This kind of thing doesn't exactly fill me with joy, but I guess the alternatives wouldn't be much better. Any reason this can't be part of load_gl_funcs()? (And while we're touching load_gl_funcs(), I don't see much of a reason we couldn't just inline GL_EXT_FUNCS_GEN these days.) > + /* Newer core functions */ \ > + USE_GL_FUNC(glActiveTexture) \ It would probably be nice to note the exact GL version where the function entered core GL.

2 3

Re: [PATCH 1/5] winemac: Make the implementation of clearToBlackIfNeeded compatible with core contexts.
by Ken Thomases Jan. 5, 2015

Jan. 5, 2015

On Jan 5, 2015, at 10:17 AM, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > --- > dlls/winemac.drv/cocoa_opengl.m | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) Looks OK to me. -Ken

1 0