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

Re: [4/4] dlls/ole32: Add more traces to the storage methods. Take 2.

by Vincent Povirk

> To avoid any confusion I have added a TRACE() for those cases in dmusic. > And tests. E.g.: > > /* IPersistStream methods not implemented in native */ > HRESULT WINAPI unimpl_IPersistStream_GetClassID(IPersistStream *iface, > CLSID *class) > { > TRACE("(%p, %p): method not implemented\n", iface, class); > return E_NOTIMPL; > } Well, they're internal, so we can't test their return codes. I have no way of knowing if the native implementation of these objects even have a similar method. As long as the Commit implementations have traces, I don't think we need them here, but right now we severely lack traces so I'm OK with adding a bunch of them and not thinking too hard about which ones we'll need.

Nov. 2, 2015

Re: [4/4] dlls/ole32: Add more traces to the storage methods. Take 2.

by Michael Stefaniuc

On 11/02/2015 06:13 PM, Vincent Povirk wrote: >> static HRESULT TransactedSnapshotImpl_LockTransaction(StorageBaseImpl *base, BOOL write) >> { >> + FIXME("stub\n"); >> return E_NOTIMPL; >> } >> >> static HRESULT TransactedSnapshotImpl_UnlockTransaction(StorageBaseImpl *base, BOOL write) >> { >> + FIXME("stub\n"); >> return E_NOTIMPL; >> } > > It's the same situation with these two. They are internal functions, > and they're not supposed to be implemented. To avoid any confusion I have added a TRACE() for those cases in dmusic. And tests. E.g.: /* IPersistStream methods not implemented in native */ HRESULT WINAPI unimpl_IPersistStream_GetClassID(IPersistStream *iface, CLSID *class) { TRACE("(%p, %p): method not implemented\n", iface, class); return E_NOTIMPL; } bye michael

Nov. 2, 2015

Re: [4/4] dlls/ole32: Add more traces to the storage methods. Take 2.

by Vincent Povirk

> static HRESULT TransactedSnapshotImpl_LockTransaction(StorageBaseImpl *base, BOOL write) > { > + FIXME("stub\n"); > return E_NOTIMPL; > } > > static HRESULT TransactedSnapshotImpl_UnlockTransaction(StorageBaseImpl *base, BOOL write) > { > + FIXME("stub\n"); > return E_NOTIMPL; > } It's the same situation with these two. They are internal functions, and they're not supposed to be implemented.

Nov. 2, 2015

Re: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.

by Vincent Povirk

>> It seems like you're doing two unrelated things here? I'd expect the >> process of building the string to send and sending it to be >> independent. > > I don't think the other code we have that interacts with X11 is > similar enough to this to justify creating a helper code and moving > some of this code there. Also, I'm not trying to redesign the entire > startup notification feature, I'm just trying to fix a potential > buffer overflow while making this function a bit more efficient and > understandable. I don't think you need to restructure the code, just saying maybe it'd make sense to split this. >>> if ((src = strstr( id, "_TIME" ))) update_user_time( atol( src + 5 )); >>> >>> - pos = snprintf(message, sizeof(message), "remove: ID="); >>> - message[pos++] = '"'; >>> - for (i = 0; id[i] && pos < sizeof(message) - 2; i++) >>> + pos = sprintf(message, "remove: ID=\""); >>> + for (i = 0; id[i]; i++) >>> { >>> if (id[i] == '"' || id[i] == '\\') >>> + { >>> + if (pos == sizeof(message) - 3) break; >>> message[pos++] = '\\'; >>> + } >>> + if (pos == sizeof(message) - 2) break; >>> message[pos++] = id[i]; >>> } >>> message[pos++] = '"'; >>> message[pos++] = '\0'; >> >> Your use of == instead of > or >= in these checks makes me >> uncomfortable. Given that a single iteration can increment pos twice, >> why can't we have a situation where (pos > sizeof(message) - 3), >> inside the if statement? > > Because I'm checking pos every time that it increments by one. You're checking it, but not for the same value. Why can't we have this happen: * id[i] is 'a', and pos == sizeof(message) - 3 * We check whether pos == sizeof(message) - 2, it isn't so we keep going. * We write 'a' into message and increment pos, now it's sizeof(message) - 2 * We increment i and continue with the loop, now id[i] is '"'. * We check whether pos == sizeof(message) - 3, it isn't, we write a \ and increment pos to sizeof(message) - 1 * There's nothing to stop the loop from continuing when we run out of buffer. A simpler approach might be to change the buffer sizes so that we can't overrun the second buffer. > So, does all that make sense? Would you sign off on this patch if I > resubmit it keeping the memset? No, because I'm not convinced the loop is correct.

Nov. 2, 2015

Re: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.

by Alex Henrie

2015-10-31 13:03 GMT-06:00 Vincent Povirk <vincent(a)codeweavers.com>: > It seems like you're doing two unrelated things here? I'd expect the > process of building the string to send and sending it to be > independent. I don't think the other code we have that interacts with X11 is similar enough to this to justify creating a helper code and moving some of this code there. Also, I'm not trying to redesign the entire startup notification feature, I'm just trying to fix a potential buffer overflow while making this function a bit more efficient and understandable. >> if ((src = strstr( id, "_TIME" ))) update_user_time( atol( src + 5 )); >> >> - pos = snprintf(message, sizeof(message), "remove: ID="); >> - message[pos++] = '"'; >> - for (i = 0; id[i] && pos < sizeof(message) - 2; i++) >> + pos = sprintf(message, "remove: ID=\""); >> + for (i = 0; id[i]; i++) >> { >> if (id[i] == '"' || id[i] == '\\') >> + { >> + if (pos == sizeof(message) - 3) break; >> message[pos++] = '\\'; >> + } >> + if (pos == sizeof(message) - 2) break; >> message[pos++] = id[i]; >> } >> message[pos++] = '"'; >> message[pos++] = '\0'; > > Your use of == instead of > or >= in these checks makes me > uncomfortable. Given that a single iteration can increment pos twice, > why can't we have a situation where (pos > sizeof(message) - 3), > inside the if statement? Because I'm checking pos every time that it increments by one. > As for the memset, I didn't examine that as thoroughly, but it's a > good policy to not send uninitialized data over the wire. Still, we > could move the memset outside the loop and know that won't happen. That's a good point, and not just because we don't accidentally want to send sensitive information over the wire: If the program is running over X11 forwarding, zeroing out unused bytes improves performance by improving the compression ratio. In the end, I think it's best to leave the memset how it was before (inside the loop), especially given that it's unusual for this loop to execute more than once. So, does all that make sense? Would you sign off on this patch if I resubmit it keeping the memset? -Alex

Nov. 2, 2015

Re: steam web browsing on OS X

by Erich E. Hoover

On Mon, Nov 2, 2015 at 5:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > > ... > I'm not talking about other platforms, but at a minimum wow64 needs to > be supported. We can't require people to use 32-bit prefixes forever. > As far as I can tell from the sandbox source, Windows 8 and Windows 10 > versions would also have to be handled differently. I have support for xp and xp-WoW64 in my prototype, the difficulty is that the xp-WoW64 doesn't work with Steam because of the 64-bit "helper". https://dl.dropboxusercontent.com/u/195059/wine/ntdll-Syscall_Wrappers/inde… The most interesting patches are 5 and 6, 7 is trying to figure out what is going on with the 64-bit version (it fails to even read the function memory at all). Michael has a brilliant scheme for getting the sysenter to work, but I'll leave that up to him to discuss. Best, Erich

Nov. 2, 2015

Re: [v3 resend] d3d9: call wined3d_device_copy_sub_resource_region instead of wined3d_device_update_surface in d3d9_device_U

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=17903 Your paranoid android. === build (build) === Patch failed to apply

Nov. 2, 2015

Re: [PATCH 1/5] winepulse: Add initial stub for pulseaudio support

by Andrew Eikum

On Tue, Nov 03, 2015 at 12:22:09AM +0900, Alexandre Julliard wrote: > Andrew Eikum <aeikum(a)codeweavers.com> writes: > > > I didn't really test it in-progress like that because lots of critical > > functionality is missing in these early patches. I'm not sure how well > > our winmm, dsound, and tests will handle missing IAudioRenderClient, > > for example. It's split up for review purposes, because the full > > driver is over 3000 lines. > > > > I could try reproducing those issues and fixing them if you want, but > > I'm not sure it's worth the time if the client DLLs and applications > > won't handle it well anyway. > > If it's just because things are missing, then that's OK. But you better > hope that once the whole series is in, it no longer crashes here ;-) After those five, there are three more patches to implement missing critical interfaces (volume, session, and marshalling). All three should be in the next wave. After those, I expect the audio tests to pass. Then there are a couple of other patches for small improvements and consistency with other drivers, split out to retain authorship. Andrew

Nov. 2, 2015

Re: [PATCH 1/5] winepulse: Add initial stub for pulseaudio support

by Alexandre Julliard

Andrew Eikum <aeikum(a)codeweavers.com> writes: > I didn't really test it in-progress like that because lots of critical > functionality is missing in these early patches. I'm not sure how well > our winmm, dsound, and tests will handle missing IAudioRenderClient, > for example. It's split up for review purposes, because the full > driver is over 3000 lines. > > I could try reproducing those issues and fixing them if you want, but > I'm not sure it's worth the time if the client DLLs and applications > won't handle it well anyway. If it's just because things are missing, then that's OK. But you better hope that once the whole series is in, it no longer crashes here ;-) -- Alexandre Julliard julliard(a)winehq.org

Nov. 2, 2015

Re: [PATCH 1/7] wined3d: Introduce wined3d_device_map_sub_resource().

by Józef Kucia

On Mon, Nov 2, 2015 at 12:24 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > I'm not sure how easy it would be at this point, but we'll probably > just want resource ops on sub-resources to print an ERR(). E.g. > ideally we'd never call resource_unload() on a surface, but only ever > on a texture. But that then means not adding sub-resources to the > device's resource list, etc. > I'll add this to my todo list.

Nov. 2, 2015