Re: wmc: Fix symbol was not declared and using plain integer as NULL pointer sparse warnings.
2010/1/21 Jacek Caban : > Hi Rob, > > On 1/21/10 12:57 PM, Rob Shearman wrote: >> >> --- >> tools/wmc/wmc.c | 8 +--- >> tools/wmc/wmc.h | 2 +- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> > > --- a/tools/wmc/wmc.h > +++ b/tools/wmc/wmc.h > @@ -65,7 +65,7 @@ extern node_t *nodehead; > extern lan_blk_t *lanblockhead; > > int mcy_lex(void); > -FILE *yyin; > +extern FILE *yyin; > > > It looks like it worked before your patch only because it's used only in > wmc.c, which means that it probably should be static. It's used in mcl.c too: mcl.c: cptr = fgets(xlatebuffer, INPUTBUFFER_SIZE, yyin); mcl.c: if(!cptr && ferror(yyin)) mcl.c: n = fread(inputbuffer, 1, 8, yyin); mcl.c: if(!n && ferror(yyin)) mcl.c: t = fread(&inputbuffer[i], 2, 1, yyin); mcl.c: if(!t && ferror(yyin)) -- Rob Shearman
Re: Nikolay Sivov : advapi32: Free descriptor if it isn' t returned from GetSecurityInfo().
2010/1/11 Alexandre Julliard : > Module: wine > Branch: master > Commit: dbd76575ef8353484afa0b0d2da95760c26f34fe > URL: > http://source.winehq.org/git/wine.git/?a=commit;h=dbd76575ef8353484afa0b0d2da95760c26f34fe > > Author: Nikolay Sivov > Date: Fri Jan 8 21:05:21 2010 +0300 > > advapi32: Free descriptor if it isn't returned from GetSecurityInfo(). > > --- > > dlls/advapi32/security.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/dlls/advapi32/security.c b/dlls/advapi32/security.c > index 629a74a..babe468 100644 > --- a/dlls/advapi32/security.c > +++ b/dlls/advapi32/security.c > @@ -3118,6 +3118,8 @@ DWORD WINAPI GetSecurityInfo( > } > if (ppSecurityDescriptor) > *ppSecurityDescriptor = sd; > + else > + LocalFree(sd); > > return ERROR_SUCCESS; > } This is not correct as per the discussion on wine-devel, so it needs to be reverted. It causes these new Valgrind warnings: http://kegel.com/wine/valgrind/logs/2010-01-11-14.17/diff-advapi32_security.txt -- Rob Shearman
Re: Patch feedback requested for OleCreatePropertyFrame()
2010/1/8 Wolfram Sang : > Geoffrey Hausheer wrote: > >> Would someone mind reviewing this for style/content? I still have no > > Can't say much about the content, but stylewise the pointer handling > could be improved. For example, > > + if ( (iface==0) || (ppvObject==0) ) > > should at least use NULL instead of 0 as both are pointers, but !ptr is > even better here. Accordingly iface shouldn't generally be checked for NULL unless a program depends on it and a testcase shows this behaviour in native. -- Rob Shearman
Re: [3/4] jscript: Fix various memory and reference count leaks.
2009/12/30 Jacek Caban : > Hi Rob, > > Thanks for your work on jscript. Unfortunately fixing Valgrind on jscript is > not going to be easy. Most of your fixes are good, except clear_global part > and "jscript: Fix a circular reference caused by object and function objects > being able to have properties that are objects and functions." patch. I've > sent a test showing that Windows doesn't do that. I've resent the memory leak patch without the clear_global part. > To fix it properly we need > to implement a garbage collector. I've designed it in the very early days of > jscript, but I never had time to implement it. That sounds like an excellent piece of work for someone interested in algorithms as opposed to the usual bug fixing. I don't think I have the time to follow through on this myself at the moment. I can see how this would work for solving the general problem of circular reference counts in jscript. However, I don't see how this helps for managing the lifetime of global objects. It seems that either the global constructors have state in which case they need to be around for as long as the script context is, while not holding a reference to the script context to avoid preventing it from dying, or they don't have any state in which case they can be constructed lazily and destroyed when the last reference is removed. Thanks, -- Rob Shearman
Re: qedit: Implement partially the Sample Grabber filter
2009/12/23 Paul Chitescu : > Changelog: > qedit: Implement partially the Sample Grabber filter > > The IMediaPosition, IMediaSeeking and IQualityControl interfaces are currently > not implemented at all. No programs I've tested tried to query them though. > > Buffering of samples is also not implemented yet, applications usually install > an ISampleGrabberCB callback. Thread-safety is non-existent in your newly written code. DirectShow uses multiple threads so I would say thread-safety is a requirement, not a nice-to-have. -- Rob Shearman
Re: Rob Shearman : msvcrt: Free memory allocated in TLS slot on module unload as well as thread exit.
2009/12/14 Nikolay Sivov : > On 12/14/2009 18:51, Alexandre Julliard wrote: >> >> Module: wine >> Branch: master >> Commit: c20868e0a2f7af909cf8af2877ae8b024fa6d11e >> URL: >> http://source.winehq.org/git/wine.git/?a=commit;h=c20868e0a2f7af909cf8af2877ae8b024fa6d11e >> >> Author: Rob Shearman >> Date: Mon Dec 14 14:13:57 2009 + >> >> msvcrt: Free memory allocated in TLS slot on module unload as well as >> thread exit. >> >> --- >> >> dlls/msvcrt/main.c | 27 +++ >> 1 files changed, 15 insertions(+), 12 deletions(-) >> > > It's a partial solution I suppose? I works for a one thread only that > unloads module. The real motive for the patch was to free the memory on process exit to silence valgrind warnings. In reality, I don't believe msvcrt would be loaded dynamically by an app, but if we did want to cope with this the code could be changed to maintain a linked list of the objects allocated for this purpose or to use a private heap. -- Rob Shearman
Re: [10/10] rpcrt4: Add tests for RPC calls with authentication.
2009/12/14 Hans Leidekker : > On Monday 14 December 2009 10:41:12 Paul Vriens wrote: > >> I took the liberty of running these tests on winetestbot. It shows two >> errors for W2K and above: >> >> https://winetestbot.geldorp.nl/JobDetails.pl?Key=115 > > I wrote those tests originally, so I'll fix it :) Those failures only occur because of modifications I made to them, but thanks for fixing it anyway! -- Rob Shearman
Re: kernel32.dll: Reduce Registry access to KEY_READ or MAXIMUM_ALLOWED wherever possible (try 2)
2009/11/27 Paul Chitescu : > --- ./dlls/kernel32/locale.c.orig 2009-11-02 05:45:10.0 +0200 > +++ ./dlls/kernel32/locale.c 2009-11-26 19:35:39.0 +0200 > @@ -3017,7 +3017,7 @@ > RtlInitUnicodeString( &keyName, szKeyName ); > InitializeObjectAttributes(&attr, &keyName, 0, hRootKey, NULL); > > -if (NtOpenKey( &hkey, KEY_ALL_ACCESS, &attr ) != STATUS_SUCCESS) > +if (NtOpenKey( &hkey, MAXIMUM_ALLOWED, &attr ) != STATUS_SUCCESS) > hkey = 0; > > return hkey; This is ugly. The function should be changed to allow the required access rights to be specified. -- Rob Shearman
Re: services.exe: Reduce Registry access to KEY_READ or MAXIMUM_ALLOWED wherever possible
2009/11/27 Paul Chitescu :
> @@ -376,7 +376,7 @@
> InitializeCriticalSection(&(*db)->cs);
>
> err = RegCreateKeyExW(HKEY_LOCAL_MACHINE, SZ_SERVICES_KEY, 0, NULL,
> - REG_OPTION_NON_VOLATILE, KEY_ALL_ACCESS, NULL,
> + REG_OPTION_NON_VOLATILE, MAXIMUM_ALLOWED, NULL,
>&(*db)->root_key, NULL);
> if (err != ERROR_SUCCESS)
> HeapFree(GetProcessHeap(), 0, *db);
I'm not happy with this change because the code was written with the
assumption that data it will always have permission to write data back
to keys under root_key. The return value from save_service_config
isn't always checked and thus this could lead to silent data loss. In
any event, the use of MAXIMUM_ALLOWED should not be encouraged where
at all possible [1] and instead the code should try the most
permissions it needs and fall back to less as appropriate.
> @@ -417,7 +417,7 @@
> break;
>
> WINE_TRACE("Loading service %s\n", wine_dbgstr_w(szName));
> -err = RegOpenKeyExW(db->root_key, szName, 0, KEY_READ | KEY_WRITE,
> &hServiceKey);
> +err = RegOpenKeyExW(db->root_key, szName, 0, KEY_READ, &hServiceKey);
> if (err == ERROR_SUCCESS)
> {
> err = load_service_config(hServiceKey, entry);
This change is fine since hServiceKey is only every used with
operations that require KEY_READ at the most.
--
Rob Shearman
[1] http://blogs.msdn.com/larryosterman/archive/2004/09/14/229658.aspx
Re: advapi32.dll: Reduce Registry access to MAXIMUM_ALLOWED wherever possible
2009/11/27 Paul Chitescu : > Changelog: > advapi32.dll: Reduce Registry access to MAXIMUM_ALLOWED wherever > possible > > Makes read operation succeed on read/only Registry keys. Needs tests to show that this behaviour matches that on Windows. You should be able to achieve this by setting an appropriate security descriptor on the registry key before calling the function you are changing. However, the change to get_special_root_hkey looks fine to me. The rest of your patch isn't easy to review because you haven't generated the diff with the "-p" option. -- Rob Shearman
Re: ole32: Fix circular reference count in default handler objects.
2009/11/24 Rob Shearman : > This is caused by caching a pointer and reference to the data cache's > IPersistStorage interface without managing reference counts > appropriately. > --- > dlls/ole32/defaulthandler.c | 22 ++ > 1 files changed, 22 insertions(+), 0 deletions(-) > > Fixes memory leaks in defaulthandler tests. May also fix bugs 12419 & 14863. I should also note that the rules for aggregating objects implemented by this patch are listed here: http://msdn.microsoft.com/en-us/library/ms686558(VS.85).aspx -- Rob Shearman
Re: mapistub: add stubbed dll mapistub.dll
2009/11/23 Louis Lenders : > This fixes http://bugs.winehq.org/show_bug.cgi?id=20050 > > The dll is present on my XP system This DLL is already partially implemented in a DLL called mapi32, so there should be no need to add this completely stubbed out. -- Rob Shearman
Re: [PATCH] ole32: Some missing error checking in FileMonikerImpl_CommonPrefixWith (Coverity)
2009/11/23 Marcus Meissner :
> if(mkSys==MKSYS_FILEMONIKER){
> HRESULT ret;
>
> - CreateBindCtx(0,&pbind);
> + ret = CreateBindCtx(0,&pbind);
> + if (FAILED(ret))
> + return ret;
>
> /* create a string based on common part of the two paths */
>
> - IMoniker_GetDisplayName(iface,pbind,NULL,&pathThis);
> - IMoniker_GetDisplayName(pmkOther,pbind,NULL,&pathOther);
> + ret = IMoniker_GetDisplayName(iface,pbind,NULL,&pathThis);
> + if (FAILED(ret))
> + return ret;
pbind is leaked here.
> + ret = IMoniker_GetDisplayName(pmkOther,pbind,NULL,&pathOther);
> + if (FAILED(ret))
> + return ret;
pbind and pathThis are leaked here.
>
> nb1=FileMonikerImpl_DecomposePath(pathThis,&stringTable1);
> + if (FAILED(nb1))
> + return nb1;
Ditto plus pathOther.
> nb2=FileMonikerImpl_DecomposePath(pathOther,&stringTable2);
> + if (FAILED(nb2))
> + return nb2;
Ditto plus stringTable1.
>
> if (nb1==0 || nb2==0)
> return MK_E_NOPREFIX;
>
>
> commonPath=HeapAlloc(GetProcessHeap(),0,sizeof(WCHAR)*(min(lstrlenW(pathThis),lstrlenW(pathOther))+1));
> + if (!commonPath)
> + return E_OUTOFMEMORY;
Ditto plus stringTable2.
>
> *commonPath=0;
--
Rob Shearman
Re: [RFC] Handle process token groups in server/file.c::sd_to_mode
2009/11/16 Ben Peddell : > This proposed patch (which I believe will contribute toward solving bugs > 17672, 19588 and 20643, and any others where the permissions are set too > restrictive) exposes the token_sid_present call in token.c, > and uses it to check the SIDs in the security descriptor against those > in the process token. > > Are there any changes anyone can think of before I submit it to > wine-patches? > > Is there a better (already exposed) way of checking a SID against the > process token's group list? Hi Ben, While I agree that there is a problem that needs to be fixed, I'm not sure this is the right approach. I think you need to take a step back and consider the meanings of the different SIDs in a token by default and how they map wine running inside the Unix permissions model. For example, maybe these mappings make sense: security_local_sid -> user + group + others security_interactive_sid -> user + group + others alias_users_sid -> user + group + others? Now it's likely that the bugs you are trying to fix are trying to set the SD for a file to alias_admins_sid or alias_users_sid. The mapping for alias_admins_sid is less clear - one could argue that all Wine users on a given system would present themselves as admins to apps, but then again the apps may be restricting permissions on a file because it contains sensitive data and should only be shared with other admins (which would be trusted as such, unlike other users on a system). -- Rob Shearman
Re: (try 2) user32: Add argument check for PTITLEBARINFO in GetTitleBarInfo().
2009/11/12 Rico Schüller : > Hi, > > this patch adds a check for PTITLEBARINFO in GetTitleBarInfo() to avoid a > crash. This fixes bug 20249 . > > Try 2 improvement: > - do not print the null pointer, it's useless > > Cheers > Rico > > --- > dlls/user32/nonclient.c | 6 ++ > 1 files changed, 6 insertions(+), 0 deletions(-) You could easily add a testcase for this behaviour. -- Rob Shearman
Re: Question about GetTokenInformation and memory leaks
2009/11/11 Dan Kegel : > On Mon, Nov 9, 2009 at 2:12 PM, Rob Shearman wrote: >> 2009/11/9 Dan Kegel : >>> 16 bytes in 1 blocks are definitely lost in loss record 123 of 728 >>> at RtlAllocateHeap (heap.c:1423) >>> by RtlAllocateAndInitializeSid (sec.c:156) >>> by NtQueryInformationToken (nt.c:379) >>> by GetTokenInformation (security.c:676) >> >> The bug is in the Wine code. Try the attached patch, which should fix >> it as a side-effect. > > Works great, thanks. Is that submitted / going to be submitted soon? Given the possibility of these changes breaking installers I plan to submit it after the next release of Wine. -- Rob Shearman
Re: Still a crash in the rpcrt4 tests
2009/11/12 Paul Vriens : > Hi Rob, > > There is still a crash in the rpcrt4 tests on NT4+. > > These 2 tests both crash actually: > > 886 x = echo_ranged_int(0); > 887 ok(x == 0, "echo_ranged_int() returned %d instead of 0\n", x); > 888 x = echo_ranged_int(100); > 889 ok(x == 100, "echo_ranged_int() returned %d instead of 100\n", x); > > but only in the "ncalrpc_basic" case. I have a fix in widl for these which I'll send shortly. -- Rob Shearman
Re: What's the use of volatile in Wine code?
2009/11/11 : > Hi, > > AJ wrote in > http://www.winehq.org/pipermail/wine-devel/2009-November/079575.html >>If there is proper synchronization you don't need >>volatile, and if there isn't volatile won't magically fix it. > > However, mcimidi has in its code since pre 1999: > :/* it seems that in case of multi-threading, gcc is optimizing just a little > bit > : * too much. Tell gcc not to optimize status value using volatile. > : */ > :while (((volatile WINE_MCIMIDI*)wmm)->dwStatus == MCI_MODE_PAUSE); > > The comment is right. Any C compiler is allowed to optimize this into > an endless loop without volatile. But I'd rather see the following: > :volatile dwStatus = NOT_READY; /* one central declaration */ > :... > :while (wmm->dwStatus == PAUSE) ; /* what, busy wait!?! */ > > So what's the use of volatile? When is it appropriate in Wine? I haven't looked in detail at the context of your patch, but this looks appropriate to it: http://lkml.indiana.edu/hypermail/linux/kernel/0607.0/1566.html Particularly the part about volatile storage vs. access. -- Rob Shearman
Re: Fun new valgrind warnings in rpc code?
2009/11/11 Dan Kegel : > As of today, six tests: > > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-rpcrt4_server.txt > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-ole32_marshal.txt > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-ole32_moniker.txt > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-rpcrt4_ndr_marshall.txt > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-ole32_moniker.txt > http://kegel.com/wine/valgrind/logs/2009-11-11-07.36/diff-rpcrt4_server.txt > > all report the new valgrind warning > > Syscall param socketcall.send(msg) points to uninitialised byte(s) > at send (socket.S:100) > by rpcrt4_conn_write (rpc_binding.h:170) > by RPCRT4_SendWithAuth (rpc_message.c:841) > by RPCRT4_Send (rpc_message.c:983) > by process_bind_packet (rpc_server.c:281) > by RPCRT4_process_packet (rpc_server.c:412) > by RPCRT4_worker_thread (rpc_server.c:435) > by worker_thread_proc (threadpool.c:114) > by ??? (signal_i386.c:2279) > by call_thread_entry_point (signal_i386.c:2306) > by start_thread (thread.c:469) > by start_thread (pthread_create.c:297) > by clone (clone.S:130) > Address 0x7f037e28 is 40 bytes inside a block of size 60 alloc'd > at notify_alloc (heap.c:214) > by RtlAllocateHeap (heap.c:1421) > by RPCRT4_SendWithAuth (rpc_message.c:800) > by RPCRT4_Send (rpc_message.c:983) > by process_bind_packet (rpc_server.c:281) > by RPCRT4_process_packet (rpc_server.c:412) > by RPCRT4_worker_thread (rpc_server.c:435) > by worker_thread_proc (threadpool.c:114) > by ??? (signal_i386.c:2279) > by call_thread_entry_point (signal_i386.c:2306) > by start_thread (thread.c:469) > by start_thread (pthread_create.c:297) > by clone (clone.S:130) > Uninitialised value was created by a client request > at mark_block_uninitialized (heap.c:187) > by RtlAllocateHeap (heap.c:1429) > by I_RpcAllocate (rpcrt4_main.c:574) > by RPCRT4_ReceiveWithAuth (rpc_message.c:1160) > by RPCRT4_Receive (rpc_message.c:1298) > by RPCRT4_io_thread (rpc_server.c:453) > by ??? (signal_i386.c:2279) > by call_thread_entry_point (signal_i386.c:2306) > by start_thread (thread.c:469) > by start_thread (pthread_create.c:297) > by clone (clone.S:130) > > I don't think that happened before today. > > Rob/Hans, could you have a look? Probably caused by: http://source.winehq.org/git/wine.git/?a=commitdiff;h=59ba6d2573532c15e2487bbc86f6bb93022c1d38 I'll take a look. -- Rob Shearman
Re: Question about GetTokenInformation and memory leaks
2009/11/9 Dan Kegel :
> I've never used the security apis, so I'm pretty unfamiliar with them.
> Valgrinding chromium's sandbox_unittests.exe shows the leak
>
> 16 bytes in 1 blocks are definitely lost in loss record 123 of 728
> at RtlAllocateHeap (heap.c:1423)
> by RtlAllocateAndInitializeSid (sec.c:156)
> by NtQueryInformationToken (nt.c:379)
> by GetTokenInformation (security.c:676)
> by ATL::CAccessToken::GetInfoConvert
> (atlsecurity.h:754)
> by ATL::CAccessToken::GetPrimaryGroup (atlsecurity.inl:3623)
> by sandbox::RestrictedTokenTest_CustomInit_Test::TestBody
> (restricted_token_unittest.cc:92)
>
> The test in question is on line 92 of
> http://src.chromium.org/viewvc/chrome/trunk/src/sandbox/src/restricted_token_unittest.cc?revision=11651
>
> Can someone familiar with this stuff have a look?
> I suspect it's a problem with the test, but
> can't figure out what's going on. I would
> have thought the destructors for ATL::CAccessToken
> and ATL::CSid would have freed everything.
> (I think the code leaks token_handle, but fixing
> that doesn't get rid of the reported leak.)
Hi Dan,
The bug is in the Wine code. Try the attached patch, which should fix
it as a side-effect.
Thanks,
--
Rob Shearman
From 766547300875c677ea9cfa6b3a10a3fb717c68ee Mon Sep 17 00:00:00 2001
From: Robert Shearman
Date: Mon, 27 Apr 2009 18:53:15 +0100
Subject: [PATCH] server: Extend get_token_user server call to also retrieve SIDs for the token's owner or primary group.
To: wine-patches
---
dlls/ntdll/nt.c | 51 ---
server/protocol.def |9 +
server/token.c | 45 +
3 files changed, 70 insertions(+), 35 deletions(-)
diff --git a/dlls/ntdll/nt.c b/dlls/ntdll/nt.c
index b29a8ae..bc01a4f 100644
--- a/dlls/ntdll/nt.c
+++ b/dlls/ntdll/nt.c
@@ -257,12 +257,6 @@ NTSTATUS WINAPI NtQueryInformationToken(
switch (tokeninfoclass)
{
-case TokenOwner:
-len = sizeof(TOKEN_OWNER) + sizeof(SID);
-break;
-case TokenPrimaryGroup:
-len = sizeof(TOKEN_PRIMARY_GROUP);
-break;
case TokenSource:
len = sizeof(TOKEN_SOURCE);
break;
@@ -287,16 +281,17 @@ NTSTATUS WINAPI NtQueryInformationToken(
switch (tokeninfoclass)
{
case TokenUser:
-SERVER_START_REQ( get_token_user )
+SERVER_START_REQ( get_token_sid )
{
TOKEN_USER * tuser = tokeninfo;
PSID sid = tuser + 1;
DWORD sid_len = tokeninfolength < sizeof(TOKEN_USER) ? 0 : tokeninfolength - sizeof(TOKEN_USER);
req->handle = wine_server_obj_handle( token );
+req->which_sid = tokeninfoclass;
wine_server_set_reply( req, sid, sid_len );
status = wine_server_call( req );
-if (retlen) *retlen = reply->user_len + sizeof(TOKEN_USER);
+if (retlen) *retlen = reply->sid_len + sizeof(TOKEN_USER);
if (status == STATUS_SUCCESS)
{
tuser->User.Sid = sid;
@@ -372,17 +367,21 @@ NTSTATUS WINAPI NtQueryInformationToken(
break;
}
case TokenPrimaryGroup:
-if (tokeninfo)
+SERVER_START_REQ( get_token_sid )
{
TOKEN_PRIMARY_GROUP *tgroup = tokeninfo;
-SID_IDENTIFIER_AUTHORITY sid = {SECURITY_NT_AUTHORITY};
-RtlAllocateAndInitializeSid( &sid,
- 2,
- SECURITY_BUILTIN_DOMAIN_RID,
- DOMAIN_ALIAS_RID_ADMINS,
- 0, 0, 0, 0, 0, 0,
- &(tgroup->PrimaryGroup));
+PSID sid = tgroup + 1;
+DWORD sid_len = tokeninfolength < sizeof(TOKEN_PRIMARY_GROUP) ? 0 : tokeninfolength - sizeof(TOKEN_PRIMARY_GROUP);
+
+req->handle = wine_server_obj_handle( token );
+req->which_sid = tokeninfoclass;
+wine_server_set_reply( req, sid, sid_len );
+status = wine_server_call( req );
+if (retlen) *retlen = reply->sid_len + sizeof(TOKEN_PRIMARY_GROUP);
+if (status == STATUS_SUCCESS)
+tgroup->PrimaryGroup = sid;
}
+SERVER_END_REQ;
break;
case TokenPrivileges:
SERVER_START_REQ( get_token_privileges )
@@ -398,15 +397,21 @@ NTSTATUS WINAPI NtQueryInformationToken(
SERVER_END_REQ;
break;
case TokenOwner:
-if (tokeninfo)
+SERVER_START_REQ( get_token_sid )
{
-TOKEN_OWNER *owner = tokeninfo;
-PSID sid = owner + 1;
-SID_IDENTIFIER_AUTHORITY localSidAuthority = {SECURITY_NT_AUTHORITY};
-
Re: [PATCH] rpcrt4: compare networkoptions correctly (Coverity) (adjusted)
2009/11/8 Marcus Meissner :
> @@ -84,6 +84,17 @@ static RPC_STATUS RpcAssoc_Alloc(LPCSTR Protseq, LPCSTR
> NetworkAddr,
> return RPC_S_OK;
> }
>
> +static BOOL compare_networkoptions(LPCWSTR opts1, LPCWSTR opts2)
> +{
> + if ((opts1 == NULL) && (opts2 == NULL))
> + return TRUE;
> + if ((opts1 == NULL) && (opts2 != NULL))
> + return FALSE;
> + if ((opts1 != NULL) && (opts2 == NULL))
> + return FALSE;
> + return !strcmpW(opts1, opts2);
> +}
> +
> RPC_STATUS RPCRT4_GetAssociation(LPCSTR Protseq, LPCSTR NetworkAddr,
> LPCSTR Endpoint, LPCWSTR NetworkOptions,
> RpcAssoc **assoc_out)
> @@ -97,8 +108,8 @@ RPC_STATUS RPCRT4_GetAssociation(LPCSTR Protseq, LPCSTR
> NetworkAddr,
> if (!strcmp(Protseq, assoc->Protseq) &&
> !strcmp(NetworkAddr, assoc->NetworkAddr) &&
> !strcmp(Endpoint, assoc->Endpoint) &&
> - ((!assoc->NetworkOptions && !NetworkOptions) ||
> !strcmpW(NetworkOptions, assoc->NetworkOptions)))
> - {
> + compare_networkoptions(NetworkOptions, assoc->NetworkOptions)
> + ) {
Looks good functionally, but please don't change the style.
> assoc->refs++;
> *assoc_out = assoc;
> LeaveCriticalSection(&assoc_list_cs);
Thanks,
--
Rob Shearman
Re: [PATCH] rpcrt4: Fixed incorrect NULL check condition (Coverity)
2009/10/17 Marcus Meissner : > Hi, > > Coverity spotted that this could lead to NULL dereference, > and yes, the && should be ||. This is not what was intended. The code inside the if block shouldn't execute if assoc->NetworkOptions is NULL and NetworkOptions isn't and vice-versa. The comparison should probably be split into a separate function. > @@ -97,7 +97,7 @@ RPC_STATUS RPCRT4_GetAssociation(LPCSTR Protseq, LPCSTR > NetworkAddr, > if (!strcmp(Protseq, assoc->Protseq) && > !strcmp(NetworkAddr, assoc->NetworkAddr) && > !strcmp(Endpoint, assoc->Endpoint) && > - ((!assoc->NetworkOptions && !NetworkOptions) || > !strcmpW(NetworkOptions, assoc->NetworkOptions))) > + (!assoc->NetworkOptions || !NetworkOptions || > !strcmpW(NetworkOptions, assoc->NetworkOptions))) Thanks, -- Rob Shearman
Re: libsmbclient and wine?
2009/10/23 Dan Kegel : > Hi folks, > it's often been said that wine can't use libsmbclient because > of license problems. I'd like to look at that assumption a bit. > > - What would wine like to use libsmbclient for, if it could? > > - Would it suffice to just link it in to wineserver, or would > it need to be invoked directly by all sorts of dlls? > > - Would we just use the system's libsmbclient (I hope so) > rather than trying to import its source? > > Also, fwiw, I think the Samba guys are willing to multiplex > their listening daemon so Wine could handle incoming > named pipe requests if we want to. libsmbclient would need to be linked into wineserver to properly implement handles to named pipes that can be inherited by child processes (and to be allow the handle to be accepted by all of the functions that take a handle without large modifications). However, having 3rd party code linked into the wineserver probably gives many people the heebie-jeebies, given how critical the wineserver is to allowing wine processes to continue to run and the special rules of coding in the wineserver - not blocking, for one. -- Rob Shearman
Re: advapi32: SetEntriesInAclW() should accept account name "CURRENT_USER".
2009/8/16 Rein Klazes :
> @@ -3510,7 +3511,8 @@ DWORD WINAPI SetEntriesInAclW( ULONG count,
> PEXPLICIT_ACCESSW pEntries,
> DWORD sid_size = FIELD_OFFSET(SID,
> SubAuthority[SID_MAX_SUB_AUTHORITIES]);
> DWORD domain_size = MAX_COMPUTERNAME_LENGTH + 1;
> SID_NAME_USE use;
> - if (!LookupAccountNameW(NULL, pEntries[i].Trustee.ptstrName,
> ppsid[i], &sid_size, NULL, &domain_size, &use))
> + if ( strcmpW( pEntries[i].Trustee.ptstrName, CURRENT_USER ) &&
What will the memory pointed to by ppsid[i] be set to in this case?
Hint: whatever the memory was last used for, not the SID of the
current user which is what is desired.
> + !LookupAccountNameW(NULL, pEntries[i].Trustee.ptstrName,
> ppsid[i], &sid_size, NULL, &domain_size, &use))
> {
> WARN("bad user name %s for trustee %d\n",
> debugstr_w(pEntries[i].Trustee.ptstrName), i);
> ret = ERROR_INVALID_PARAMETER;
--
Rob Shearman
Re: tools/widl: Split expr_int_const off from expr
1
> ); }
> | expr_list_int_const ',' expr_int_const { $$ = append_expr(
> $1, $3 ); }
> ;
>
> -expr_int_const: expr { $$ = $1;
> - if (!$$->is_const)
> - error_loc("expression
> is not an integer constant\n");
> - }
> +expr_int_const: aNUM { $$ = make_exprl(EXPR_NUM,
> $1); }
> + | aHEXNUM { $$ =
> make_exprl(EXPR_HEXNUM, $1); }
> + | tNULL { $$ = make_exprl(EXPR_NUM,
> 0); }
> + | aIDENTIFIER { $$ =
> make_exprs(EXPR_IDENTIFIER, $1); }
aIDENTIFIER isn't necessarily a constant expression.
> + | '~' expr_int_const { $$ = make_expr1(EXPR_NOT,
> $2); }
> + | '+' expr_int_const %prec POS { $$ = make_expr1(EXPR_POS,
> $2); }
> + | '-' expr_int_const %prec NEG { $$ = make_expr1(EXPR_NEG,
> $2); }
> + | expr_int_const SHL expr_int_const { $$ = make_expr2(EXPR_SHL,
> $1, $3); }
> + | expr_int_const SHR expr_int_const { $$ = make_expr2(EXPR_SHR,
> $1, $3); }
> + | expr_int_const '+' expr_int_const { $$ = make_expr2(EXPR_ADD,
> $1, $3); }
> + | expr_int_const '-' expr_int_const { $$ = make_expr2(EXPR_SUB,
> $1, $3); }
> + | expr_int_const '%' expr_int_const { $$ = make_expr2(EXPR_MOD,
> $1, $3); }
> + | expr_int_const '*' expr_int_const { $$ = make_expr2(EXPR_MUL,
> $1, $3); }
> + | expr_int_const '/' expr_int_const { $$ = make_expr2(EXPR_DIV,
> $1, $3); }
> + | expr_int_const '|' expr_int_const { $$ = make_expr2(EXPR_OR ,
> $1, $3); }
> + | expr_int_const '^' expr_int_const { $$ = make_expr2(EXPR_XOR,
> $1, $3); }
> + | expr_int_const '&' expr_int_const { $$ = make_expr2(EXPR_AND,
> $1, $3); }
Using "expr_int_const" instead of "expr" here prevents a many forms of
expressions from being parsed.
> + | tSIZEOF '(' type ')' { $$ =
> make_exprt(EXPR_SIZEOF, $3, NULL); }
> + | '(' expr ')' { $$ = $2; }
> ;
>
> expr_const: expr { $$ = $1;
It looks like you'll have to find another way of fixing the issue you
are trying to fix.
--
Rob Shearman
Re: Minor fix for include/bits1_5.idl
2009/6/28 Gerald Pfeifer : > Strike that, I must have misread the documentation. > > Only thing I am wondering is do we really need the (unsigned long) here? Yes, as conformance/variance is restricted by NDR to 32-bit quantities. The IDL compiler should warn/error without the cast as it would be silently truncating the value used for computing the memory allocated for storing the array. > If anyone has a pointer to clear documentation that would be nice; what I > found so far leaves some questions open... The DCE/RPC specification would be a good start, but there is probably some MSDN documentation on the limitations of size_is expressions in IDL. -- Rob Shearman
Re: [2/3] ole32: Implement CoGetContextToken.
2009/6/23 Hans Leidekker : > @@ -228,6 +232,7 @@ static void COM_TlsDestroy(void) > if (info->errorinfo) IErrorInfo_Release(info->errorinfo); > if (info->state) IUnknown_Release(info->state); > if (info->spy) IUnknown_Release(info->spy); > + if (info->context_token) IUnknown_Release((IUnknown > *)info->context_token); > HeapFree(GetProcessHeap(), 0, info); > NtCurrentTeb()->ReservedForOle = NULL; > } Why not change the type of info->context_token to be IObjContext* or IUnknown* instead of ULONG_PTR? It shouldn't change the memory layout of the oletls structure and it will eliminate the need of these types of casts. -- Rob Shearman
Re: ntdll: add missing valgrind hook in RtlAllocateHeap
2009/4/15 Dan Kegel : > Two chromium unit tests reported phony errors > when run on wine on valgrind because > RtlAllocateHeap sometimes returned an > allocation without notifying valgrind about it. RtlReAllocateHeap is also missing valgrind notifications for the large block paths. It also appears that the function leaks memory in the realloc small -> large path. -- Rob Shearman
Re: ntdll: Improve stubs for NtGet/SetInformationToken(TokenDefaultDacl).
2009/4/15 Hans Leidekker :
> @@ -441,6 +441,23 @@ NTSTATUS WINAPI NtQueryInformationToken(
> }
> SERVER_END_REQ;
> break;
> + case TokenDefaultDacl:
> + if (tokeninfo)
> + {
> + TOKEN_DEFAULT_DACL *token = (TOKEN_DEFAULT_DACL *)tokeninfo;
> + ACL *acl;
> +
> + FIXME("returning empty DACL\n");
> +
> + acl = token->DefaultDacl = (ACL *)token +
> sizeof(TOKEN_DEFAULT_DACL);
> +
> + acl->AclRevision = ACL_REVISION;
> + acl->Sbz1 = 0;
> + acl->AclSize = sizeof(ACL);
> + acl->AceCount = 0;
> + acl->Sbz2 = 0;
> + }
> + break;
> default:
> {
> ERR("Unhandled Token Information class %d!\n", tokeninfoclass);
The server already tracks the default DACL of the token in the token
object, so implementing these functions properly shouldn't be much
more work than writing the code to marshall and unmarshall the ACL to
and from the server, with two new trivial functions in the server for
getting and setting the default_dacl field.
--
Rob Shearman
Re: Fixing the last remaining failure on W2K (rpcrt4:server)
2009/4/15 Paul Vriens : > In an effort to fix that last failure on my box I found the culprit. I'm not > comfortable enough with just excluding (yet another) that test just for the > sake of having zero failures though. What exception is being raised? I think perhaps s_context_handle_test should be removed and replaced with another that uses widl-generated code for testing context handles rather than low-level functions. -- Rob Shearman
Re: Sources of noise on test.winehq.org?
2009/4/9 Dan Kegel : > (This was last discussed in February, e.g. > http://www.winehq.org/pipermail/wine-devel/2009-February/073060.html ) > > The results on test.winehq.org seem more variable than one > would expect, which makes it harder to gauge wine's progress. > > I can think of two sources of noise: > 1) 32 and 64 bit results are mixed together > 2) we don't have a stable stable (sic) of machines > running the tests > > Removing these two sources of noise might be as simple as > 1) omit 64 bit results, and Not a bad idea, but I would suggest classifying these machines as a different category (I would suggest "other" for the moment) rather than omitting the results. > 2) omit computers for which results are not > consistently available throughout the time range > being displayed > > Shouldn't be too hard for someone to whip together an > alternate report that did that. Wish I had the time to... -- Rob Shearman
Re: Wine registry question
2009/4/2 Damjan Jovanovic : > Hi > > I've noticed that Wine doesn't merge HKCU\Classes\Root into HKCR > (http://bugs.winehq.org/show_bug.cgi?id=17019). > > Is this for compatibility with older Windows versions, or is it a Wine bug? It's not a bug, it's just a missing feature and one that isn't trivial to implement. For example, in which component should this be done - in the server, in advapi32 code or when loading the user's registry hive (should be userenv, but actually done in the server)? What happens if there are two keys or values with the same name in both HKCU\Software\Classes and HKLM\Software\Classes? Does a delete from HKCR delete the key/value in the underlying view? If you add a key/value to a key that exists in HKCR and you don't have permission because it is from HKLM\Software\Classes if it added instead to HKCU\Software\Classes? How do you amalgamate change notification from both HKCU\Software\Classes and HKLM\Software\Classes? -- Rob Shearman
Re: Microsoft Office 2007 error
2009/4/2 Chris Howe : > 2009/4/2 Robert Lunnon >> >> I Get the following unimplemented call in Wine 1.1.16 when using the word >> 2007 file-selector >> wine: Call from 7fbb155b to unimplemented function >> shdocvw.dll.IEILIsEqual, aborting >> Hints anyone > > From the name, and the DLL it resides in, something to do with Image Lists? No: http://msdn.microsoft.com/en-us/library/bb773321(VS.85).aspx -- Rob Shearman
Re: [1/5] resend include: bring in6_addr into line with the MS definition
2009/4/1 Jeff Latimer :
> Alexandre Julliard wrote:
>>
>> Jeff Latimer writes:
>>
>>
>>>
>>> diff --git a/include/ws2tcpip.h b/include/ws2tcpip.h
>>> index a38ccda..9ddb3d1 100644
>>> --- a/include/ws2tcpip.h
>>> +++ b/include/ws2tcpip.h
>>> @@ -85,26 +85,41 @@ struct WS(ip_msfilter) {
>>> struct WS(in_addr) imsf_slist[1];
>>> };
>>> -typedef struct WS(in_addr6)
>>> -{
>>> - WS(u_char) s6_addr[16]; /* IPv6 address */
>>> +struct WS(in6_addr) {
>>> + union {
>>> + WS(u_char) Byte[16];
>>> + WS(u_short) Word[8];
>>> + } u;
>>> } IN6_ADDR, *PIN6_ADDR, *LPIN6_ADDR;
>>> +#define in_addr6 WS(in6_addr)
>>> +
>>> +#define _S6_un u
>>> +#define _S6_u8 Byte
>>> +#ifndef USE_WS_PREFIX
>>> +#define s6_addr _S6_un._S6_u8
>>> +#else
>>> +#define WS_s6_addr _S6_un._S6_u8
>>> +#endif
>>> +
>>> +#define s6_bytes u.Byte
>>> +#define s6_words u.Word
>>>
>>
>> This doesn't seem to match the PSDK, I don't see anything like this in
>> ws2tcpip.h.
>>
>
> What is the policy for selecting the Platform SDK? Do we always use the
> latest (in this case Vista) PSDK? There does not appear to any guidance on
> this matter on the various Wine sites.
It's rare for things to change much from one PSDK version to the next,
so there have not been many cases like this before. However, generally
newer is better.
>
> The following is from WS2tcpip.h in the XP PSDK that I have. I think
> it looks similar to what I submitted. Have I got the wrong PSDK?
>
> /* IPv6 definitions */
>
> #ifndef s6_addr
>
> struct in6_addr {
> union {
> u_char Byte[16];
> u_short Word[8];
> } u;
> };
>
> #define in_addr6 in6_addr
>
> /*
> ** Defines to match RFC 2553.
> */
> #define _S6_un u
> #define _S6_u8 Byte
> #define s6_addr _S6_un._S6_u8
>
> /*
> ** Defines for our implementation.
> */
> #define s6_bytes u.Byte
> #define s6_words u.Word
>
> #endif
This looks right, but in recent PSDKs it has moved from ws2tcpip.h to in6addr.h.
--
Rob Shearman
Re: rpcrt4: Compile in RPC over HTTP support even if HAVE_SOCKETPAIR is not defined.
2009/3/30 Alexandre Julliard : > Rob Shearman writes: > >> socketpair is only needed for the server functions for ncacn_ip_tcp. >> The tower implementation that ncacn_http and ncacn_ip_tcp share >> doesn't depend on it. > > It doesn't need socketpair, but it needs getaddrinfo and friends so it > still won't build on Mingw, that's why I disabled the whole thing. We > could put that part inside a #ifdef HAVE_GETADDRINFO instead, but the > end result would be the same. Ok, thanks for the clarification. I'll test building with Mingw before I submit any further patches along the same theme. -- Rob Shearman
Re: Unicode error
2009/3/22 Robert Lunnon : > I get this error under solaris building wine. > Any clues would be welcome (Things have probably changed a lot since I last > hacked on Wine) > > make[2]: Entering directory `/export/home/src/wine2004/wine/dlls/kernel32' > ../../tools/wrc/wrc --nostdinc -I. -I. -I../../include -I../../include > -D__WINESRC__ -D_KERNEL32_ -fokernel. res kernel.rc > Source: ¤Q¤ë a4 51 a4 eb > Unicode: 5341 6708 > Back: ¢Ì¤ë a2 cc a4 eb > nls/cht.nls:84:16: Error: String ¤Q¤ë does not convert identically to > Unicode and back in codepage 950. Try us ing a Unicode string > instead There is a problem with converting to/from codepages in wrc on your system, but the code that does should not be system-dependent (the character data is all in libwine). The Unicode string dumped above looks correct, but the first two bytes of "Back" are wrong. wrc uses libs/wine/wctomb.c:wine_cp_wcstombs to do the conversion from UCS-16 -> cp950 and in this case I believe it will specifically use the wcstombs_dbcs function. Can you change this function to dump out *src, uni2cp_high[*src >> 8] and res inside the loop and run wrc again on the file above? Also, what compiler did you use to compile wrc and libwine? -- Rob Shearman
Re: comctl32: correct handling of toolbar separators' size
2009/3/18 Igor Tarasov : > 2009/3/18 Rob Shearman : >> 2009/3/18 Igor Tarasov : >>>> Please ensure that there are no regressions in the display of IE6's >>>> toolbars after this patch. This is the main user of undocumented >>>> toolbar features. > > Installed IE6, tested both version (with my patch and without it). > Image is pixel-perfect, no bugs or glitches (though separators here > are 6 px only, which differs from wine's default 8). Is that enough > for regression testing? > > P.S: I was testing it with both my patches applied. Sounds good. Thanks for testing. -- Rob Shearman
Re: comctl32: correct handling of toolbar separators' size
2009/3/18 Igor Tarasov : >> Please ensure that there are no regressions in the display of IE6's >> toolbars after this patch. This is the main user of undocumented >> toolbar features. > > How can I do that? Follow the how-to here: http://appdb.winehq.org/objectManager.php?sClass=version&iId=469 However, I don't know if IE6 currently installs or works in Wine. -- Rob Shearman
Re: comctl32: CCS_VERT flips toolbar separators' orientation
2009/3/17 Igor Tarasov :
> Currently wine flips orientation of toolbar separators on
> BTNS_DROPDOWN, which is odd, since even horizontal toolbar separators
> may have this flag turned on. Tests with control spy show that native
> comctl flips orientation on CCS_VERT toolbar style, which is logical.
>
...
> @@ -855,7 +855,7 @@ TOOLBAR_DrawButton (HWND hwnd, TBUTTON_INFO *btnPtr, HDC
> hdc, DWORD dwBaseCus
> /* empirical tests show that iBitmap can/will be non-zero*/
> /* when drawing the vertical bar... */
> if ((dwStyle & TBSTYLE_FLAT) /* && (btnPtr->iBitmap == 0) */) {
> - if (btnPtr->fsStyle & BTNS_DROPDOWN)
> + if (dwStyle & CCS_VERT)
> TOOLBAR_DrawDDFlatSeparator (&rc, hdc, infoPtr);
> else
> TOOLBAR_DrawFlatSeparator (&rc, hdc, infoPtr);
You should fix the name of the TOOLBAR_DrawDDFlatSeparator function
and the comments above it if you think it should be called as a result
of the CCS_VERT window style and not the BTNS_DROPDOWN button style.
--
Rob Shearman
Re: comctl32: correct handling of toolbar separators' size
2009/3/17 Igor Tarasov : > Currently, wine uses iBitmap property for determining toolbar > separators width all the time, stating it's an undocumented feature. > This is not correct. > > According to MSDN, iBitmap is used only on inserting separator (since > there is no cx field in TBBUTTON structure). > http://msdn.microsoft.com/en-us/library/bb760476(VS.85).aspx > > In other cases, cx property that can be updated via get/set buttoninfo > is used, as tests show. Also, tests show that this property differs > from zero only if application specifically sets. Setting it to 0 makes > separator display in default width. All this is implemented in this > patch. More tests results in bugtracker (link below). Please ensure that there are no regressions in the display of IE6's toolbars after this patch. This is the main user of undocumented toolbar features. > @@ -4428,7 +4431,7 @@ TOOLBAR_SetButtonInfoA (HWND hwnd, WPARAM wParam, > LPARAM lParam) > if (lptbbi->dwMask & TBIF_LPARAM) > btnPtr->dwData = lptbbi->lParam; > if (lptbbi->dwMask & TBIF_SIZE) > - btnPtr->cx = lptbbi->cx; > +btnPtr->cx = lptbbi->cx; > if (lptbbi->dwMask & TBIF_STATE) > btnPtr->fsState = lptbbi->fsState; > if (lptbbi->dwMask & TBIF_STYLE) Whitespace only changes shouldn't be included in patches with other changes. -- Rob Shearman
Re: LookupAccountSidW returns "unexpected" username
2009/3/10 Andreas Rosenberg : > While I did some experimenting with the proposals from Vitaliy Margolen > to get an accepted patch for GetUserProfileDirectoryW, I found this > behavior: > > OpenProcessToken(hProcess,TOKEN_QUERY,&hToken); > ... > GetTokenInformation( hToken, TokenUser, ptiUser, cbti, &cbti ) > ... > LookupAccountSidW( NULL, ptiUser->User.Sid, lpUserName, lpchSize,lpDomain, > &cbDomain, &snu ); > > In Windows XP SP3 the LookupAccountSidW fills lpUserName with the > name of the current user (same as GetUserName) > > Running this code in Wine results in the username: 'INTERACTIVE'. See server/token.c:security_unix_uid_to_sid. This should return a SID with a pattern that LookupAccountSidW can recognise and then extract the uid and look it up and return the UNIX user name. > Seems like the LookupAccountSidW does something not compatible with > Windows. Im thinking about writing a test code, but simply checking > if the above code returns the same as GetUserNameW seems not > correct, if test code should run under a different account as the actual > login user. > > Is this a requirement for running the wine tests? -- Rob Shearman
Re: qmgr test failures
2009/3/9 Austin English : > Howdy Rob, > > Looks like one of your recent qmgr patches added failure to qmgr: > http://test.winehq.org/data/1b9a6fb4e9f5a76f1ca352bef121689df02d9289/#group_Wine > > file.c:121: Test failed: GetRemoteName failed: 800706c6 > file.c:124: Tests skipped: Unable to get remote name of test_file. > file.c:138: Test failed: GetLocalName failed: 800706c6 > file.c:141: Tests skipped: Unable to get local name of test_file. > file: 6 tests executed (0 marked as todo, 2 failures), 2 skipped. > > qmgr: 1 tests executed (0 marked as todo, 0 failures), 0 skipped. > qmgr.c:217: Test failed: Adding a job in another process failed > qmgr: 11 tests executed (0 marked as todo, 1 failure), 0 skipped. > > Could you take a look? I can't reproduce it, so someone who can needs to give me a +seh,+tid,+ole,+qmgr log of it happening. -- Rob Shearman
Re: String expansion (%n %t)
2009/3/8 Rick Jones : > Is there a Windows string function that expands escape sequences such as %n, > %t, etc, and which is replicated by Wine? FormatMessageA/W? > I ask because I have a Windows app that is screwing up because of something > like this. It has a private .ini settings file, and some settings are stored > in that kind of format, but when it runs on Wine they are wrongly retrieved > (this is a 3rd party app, I don't have the source). > > It looks horribly like %n is being expanded to NL instead of CR-NL. If > there's a Windows/Wine function for this then I might be able to use a > native DLL to get round it, but I've no idea where it might reside! AFAIK > GetPrivateProfileString() doesn't do this expansion inherently. > > Also if it is the case that the behaviour's different, then I would say it's > a Wine bug. > > Any ideas appreciated. -- Rob Shearman
Re: [PATCH 03/10] widl: Implement a more abstract way of representing basic types.
2009/3/5 Alexandre Julliard :
> Rob Shearman writes:
>
>> ---
>> tools/widl/expr.c | 18
>> tools/widl/header.c | 34 ++--
>> tools/widl/parser.y | 103
>> ++-
>> tools/widl/typelib.c | 60
>> tools/widl/typetree.c | 31 ++
>> tools/widl/typetree.h | 37 +-
>> tools/widl/widltypes.h | 29 +-
>> 7 files changed, 200 insertions(+), 112 deletions(-)
>>
>> Shouldn't change any generated files.
>
> It crashes on 64-bit:
>
> ../tools/widl/widl -I../../wine64/include -I. -I../../wine64/include
> -I../include -h -H activaut.h ../../wine64/include/activaut.idl
> make[1]: *** [activaut.h] Segmentation fault (core dumped)
> make[1]: Leaving directory `/home/julliard/wine/build/obj-elf64/include'
> make: *** [include] Error 2
> wine:~/wine/build/obj-elf64-$ gdb tools/widl/widl include/core
> GNU gdb 6.8-debian
> Copyright (C) 2008 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law. Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu"...
>
> warning: Can't read pathname for load map: Input/output error.
> Reading symbols from /lib/libc.so.6...done.
> Loaded symbols for /lib/libc.so.6
> Reading symbols from /lib/ld-linux-x86-64.so.2...done.
> Loaded symbols for /lib64/ld-linux-x86-64.so.2
> Core was generated by `../tools/widl/widl -I../../wine64/include -I.
> -I../../wine64/include -I../inclu'.
> Program terminated with signal 11, Segmentation fault.
> [New process 19396]
> #0 0x0042bd2d in set_type (v=0x21b4b20, decl_spec=0x21b4ae0,
> decl=0x21b0720, top=0) at ../../../wine64/tools/widl/parser.y:1369
> 1369 if (is_attr(type->attrs, ATTR_INLINE))
> (gdb) bt
> #0 0x0042bd2d in set_type (v=0x21b4b20, decl_spec=0x21b4ae0,
> decl=0x21b0720, top=0) at ../../../wine64/tools/widl/parser.y:1369
> #1 0x0042c697 in set_var_types (attrs=0x21b4aa0,
> decl_spec=0x21b4ae0, decls=0x21b4c10) at
> ../../../wine64/tools/widl/parser.y:1563
> #2 0x00429736 in parser_parse () at
> ../../../wine64/tools/widl/parser.y:705
> #3 0x0041fdd0 in main (argc=9, argv=0x7fff0037bc28) at
> ../../../wine64/tools/widl/widl.c:632
> (gdb) p type
> $1 = (type_t *) 0x1
> (gdb) p *decl_spec
> $2 = {type = 0x1, attrs = 0x0, stgclass = STG_NONE}
> (gdb)
Thanks for the info. Does the attached patch fix it?
--
Rob Shearman
diff --git a/tools/widl/typetree.c b/tools/widl/typetree.c
index 57a5736..961a3f1 100644
--- a/tools/widl/typetree.c
+++ b/tools/widl/typetree.c
@@ -102,7 +102,7 @@ type_t *type_new_basic(enum type_basic_type basic_type)
type_t *type_new_int(enum type_basic_type basic_type, int sign)
{
-static type_t *int_types[TYPE_BASIC_INT_MAX][3];
+static type_t *int_types[TYPE_BASIC_INT_MAX][3] = { { NULL } };
assert(basic_type <= TYPE_BASIC_INT_MAX);
Re: msacm32: acmFormatEnum - implementing ACM_FORMATENUMF_SUGGEST (2nd try)
2009/3/3 Henri Verbeet : > 2009/3/3 James McKenzie : >> Can you please use standard compression programs with standard extensions. >> >> Your file extension, .bin will not work here. >> > Although application/octet-stream might not be the best content type, > it's just text. In order for patches to be easily viewed in most email clients and hence reviewed, they should be sent with a content type of text/* (e.g. text/plain), regardless of whether the content is text and compressed text. -- Rob Shearman
Re: ole32/tests: Add a few CoGetMalloc tests.
2009/3/3 Huw Davies :
> diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c
> index a332d27..9720e52 100644
> --- a/dlls/ole32/tests/compobj.c
> +++ b/dlls/ole32/tests/compobj.c
> @@ -1162,6 +1162,28 @@ static void test_CoInitializeEx(void)
> OleUninitialize();
> }
>
> +static void test_CoGetMalloc(void)
> +{
> +IMalloc *malloc;
> +HRESULT hr;
> +void *ptr;
> +DWORD i, size;
> +
> +OleInitialize(NULL);
> +hr = CoGetMalloc(1, &malloc);
> +ok(hr == S_OK, "got %x\n", hr);
> +for(i = 0; i < 10; i++)
> +{
> +ptr = IMalloc_Alloc(malloc, i);
> +ok(ptr != NULL, "got NULL for size %d\n", i);
> +size = IMalloc_GetSize(malloc, ptr);
> +ok(size == i, "got %d expected %d\n", size, i);
Has this been tested on Win9x? Since HeapSize on those versions of
Windows doesn't return the same size as allocated, IMalloc_GetSize
might not either.
> +IMalloc_Free(malloc, ptr);
> +}
> +IMalloc_Release(malloc);
> +OleUninitialize();
> +}
--
Rob Shearman
Re: ole32: change a pointer cast from DWORD to DWORD_PTR
2009/2/22 Austin English : > diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c > index b8ddba8..7360f66 100644 > --- a/dlls/ole32/compobj.c > +++ b/dlls/ole32/compobj.c > @@ -2007,7 +2007,7 @@ HRESULT WINAPI CoRegisterClassObject( > * Use the address of the chain node as the cookie since we are sure it's > * unique. FIXME: not on 64-bit platforms. > */ > - newClass->dwCookie= (DWORD)newClass; > + newClass->dwCookie= (DWORD_PTR)newClass; > >/* > * Since we're making a copy of the object pointer, we have to increase its Since dwCookie is of type DWORD this is just hiding the problem. The correct solution is to store a process-wide index and then to increase this every time a class object is registered and use it for the cookie. CoRevokeClassObject already looks through the list of registered classes to find the cookie so this doesn't need to be changed. -- Rob Shearman
Re: ntdll: Replace malloc() with RtlAllocateHeap()
2009/2/17 Andrew Talbot : > Changelog: >ntdll: Replace malloc() with RtlAllocateHeap(). > > diff --git a/dlls/ntdll/server.c b/dlls/ntdll/server.c > index 145540f..0cade3f 100644 > --- a/dlls/ntdll/server.c > +++ b/dlls/ntdll/server.c > @@ -743,13 +743,13 @@ static void setup_config_dir(void) > struct stat st; > char *tmp_dir; > > -if (!(tmp_dir = malloc( p + 1 - config_dir ))) fatal_error( "out > of memory\n" ); > +if (!(tmp_dir = RtlAllocateHeap( GetProcessHeap(), 0, p + 1 - > config_dir ))) fatal_error( "out of memory\n" ); > memcpy( tmp_dir, config_dir, p - config_dir ); > tmp_dir[p - config_dir] = 0; > if (!stat( tmp_dir, &st ) && st.st_uid != getuid()) > fatal_error( "'%s' is not owned by you, refusing to create a > configuration directory there\n", > tmp_dir ); > -free( tmp_dir ); > +RtlFreeHeap( GetProcessHeap(), 0, tmp_dir ); > } I'd be surprised if this worked, since setup_config_dir is called very early in the ntdll execution sequence, and RtlAllocateHeap itself will depend on server calls (NtAllocateVirtualMemory, NtCreateMutex, etc.) that depend on the server having started (i.e. after this function having already executed). -- Rob Shearman
Re: winex11.drv: Map Super_L, Super_R, and Menu keys correctly.
2009/2/13 GerbilSoft : > --- keyboard.c.orig 2009-02-12 11:50:28.110457529 -0500 > +++ keyboard.c2009-02-12 11:52:06.061206978 -0500 You need to generate the patch from the top of the wine directory. You also need to use your real name for copyright-tracking purposes. -- Rob Shearman
Re: wininet: parse cookie information from cookie value
2009/2/13 Aric Stewart : > +if (CompareStringW(GetThreadLocale(), NORM_IGNORECASE, ptr, 6, > + szDomain, 6) == 2) LOCALE_INVARIANT should be used when comparing with a constant string. See here for the reasons why: http://blogs.msdn.com/michkap/archive/2004/12/29/344136.aspx http://blogs.msdn.com/larryosterman/archive/2004/07/01/171078.aspx http://msdn.microsoft.com/en-us/library/dd373795(VS.85).aspx It doesn't look like it's implemented in Wine, so I guess you should just use MAKELCID(LANG_ENGLISH, SUBLANG_DEFAULT) with a FIXME to use LOCALE_INVARIANT when it's implemented. And, yes, there's probably lots of other places where we do case insensitive comparisons incorrectly like this, but we should stop introducing new ones. -- Rob Shearman
Re: RFC: patch to advapi32's cred tests
2009/2/13 Juan Lang :
> Hi all, I've written a patch which should fix the failing advapi32
> cred tests that mirrors the skips in the existing tests:
>
> diff --git a/dlls/advapi32/tests/cred.c b/dlls/advapi32/tests/cred.c
> index 58103c6..bb6935b 100644
> --- a/dlls/advapi32/tests/cred.c
> +++ b/dlls/advapi32/tests/cred.c
> @@ -238,7 +238,13 @@ static void test_generic(void)
> new_cred.UserName = (char *)"winetest";
>
> ret = pCredWriteA(&new_cred, 0);
> -ok(ret, "CredWriteA failed with error %d\n", GetLastError());
> +ok(ret || broken(GetLastError() == ERROR_NO_SUCH_LOGON_SESSION),
> + "CredWriteA failed with error %d\n", GetLastError());
> +if (!ret)
> +{
> +skip("couldn't write generic credentials, skipping tests\n");
> +return;
> +}
>
> ret = pCredEnumerateA(NULL, 0, &count, &creds);
> ok(ret, "CredEnumerateA failed with error %d\n", GetLastError());
>
> I'm wondering whether a better patch would be to change the persist
> type from CRED_PERSIST_ENTERPRISE to CRED_PERSIST_LOCAL_MACHINE. Rob,
> this is particularly aimed at you since you wrote the original tests:
> is there a good reason to use CRED_PERSIST_ENTERPRISE here?
No, CRED_PERSIST_LOCAL_MACHINE should be fine but does this change
make any difference? I believe the ERROR_NO_SUCH_LOGON_SESSION is
returned when the tests are being run as a user that doesn't have a
profile, and therefore the credentials manager doesn't have anywhere
to store the credentials. Changing the persist type shouldn't affect
this, so I don't see why it would make these calls start working.
--
Rob Shearman
Re: How to get more info on timed out/crashed tests?
2009/2/11 Juan Lang :
> Hi all,
>
> there are a number of tests at test.winehq.org that indicate that
> they're timed out or crashed. The difficulty is, I have no access to
> machines running the version of Windows on which the test is timing
> out or failing, and the data about the crash are not helpful in
> pinpointing the failing test.
>
> How can we get more information about these timeouts/crashes?
Something like the attached patch?
It needs changes in winetest along with the web dissector to integrate
properly, which I haven't got around to doing yet.
--
Rob Shearman
diff --git a/include/wine/test.h b/include/wine/test.h
index caa3f35..3664cf5 100644
--- a/include/wine/test.h
+++ b/include/wine/test.h
@@ -473,6 +473,24 @@ static void usage( const char *argv0 )
}
+static LONG CALLBACK winetest_unhandled_exception_filter(PEXCEPTION_POINTERS epointers)
+{
+tls_data* data=get_tls_data();
+
+fprintf( stdout, "%s: unhandled exception 0x%08x at address %p",
+ current_test->name,
+ epointers->ExceptionRecord->ExceptionCode,
+ epointers->ExceptionRecord->ExceptionAddress );
+if (data->current_file)
+fprintf( stdout, " after %s:%d\n", data->current_file, data->current_line );
+else
+fprintf( stdout, " before first test\n" );
+
+ExitProcess( epointers->ExceptionRecord->ExceptionCode );
+/* never reached */
+}
+
+
/* main function */
int main( int argc, char **argv )
{
@@ -499,6 +517,13 @@ int main( int argc, char **argv )
list_tests();
return 0;
}
+
+/* developers sometimes don't have access to the Windows machines on
+ * which tests crash, so print a bit more information upon crashing to
+ * aid in debugging */
+if (strcmp(winetest_platform, "windows") == 0)
+SetUnhandledExceptionFilter(winetest_unhandled_exception_filter);
+
return run_test(argv[1]);
}
Re: Proposed goal for website: happily usable at 800x600?
2009/2/13 Ben Klein : > 2009/2/13 Francois Gouget : >> On Thu, 12 Feb 2009, Dan Kegel wrote: >> >>> What with netbooks with small screens abounding, it >>> might make sense to review our web site and make sure >>> its most important parts are usable in an 800x600 screen >>> (easy to simulate, just resize your browser until xwininfo >>> says it's 800x550, those gnome top and bottom bars >>> chew 25 pixels each). >> >> For what it's worth, most netbooks have a 1024x600 resolution (all the >> 9" and 10" ones as far as I know, with the exception of the HP one which >> has a highjer resolution). The old 7" models might have had an 800x600 >> resolution but I think these can be ignored now (no 7" netbook is >> shipping anymore). >> > I think it's still valuable as an accessibility thing. Remember, not > everyone has excellent eyesight! Turn the DPI up, not the resolution down. -- Rob Shearman
Re: wintrust(3/3): Fix a couple tests on a variety of systems
2009/2/12 Juan Lang : > Hi Paul, > >> This is of course on boxes where people dont' run winetest as a >> administrator. I'm not sure if ERROR_ACCESS_DENIED is thus actually an >> acceptable failure. In other tests we have used skip() for this. > > Of course that's the reason, but not running as administrator is an > acceptable configuration, is it not? I don't think it should be. There are many variables that can be changed that will cause test failures and this is one of them. We are already putting a lot of effort into fixing tests that fail just because of different versions of DLLs. We don't have the resources to spend ensuring that the tests work in every possible configuration and if many tests are skipped because of them being not being run as Administrator then that isn't going to generate that much of a useful test report. Of course, we should find some way of preventing such reports being filed and one of the ways could be to add a check to the winetest program. -- Rob Shearman
Re: oleaut32: Fix SAFEARRAY marshalling on 64-bit platforms.
2009/2/9 Alexandre Julliard : > Rob Shearman writes: > >> @@ -874,8 +874,8 @@ unsigned char * WINAPI LPSAFEARRAY_UserMarshal(ULONG >> *pFlags, unsigned char *Buf >> >> *(ULONG *)Buffer = ulCellCount; >> Buffer += sizeof(ULONG); >> -*(ULONG_PTR *)Buffer = (ULONG_PTR)psa->pvData; >> -Buffer += sizeof(ULONG_PTR); >> +*(ULONG *)Buffer = (ULONG)(ULONG_PTR)psa->pvData; >> +Buffer += sizeof(ULONG); > > Wouldn't this break if the pointer happens to be 4Gb-aligned? It would write 0 into memory, yes, but the value written here isn't used during unmarshalling so it is fine and I believe this matches what native does. If we wanted to relax the tests, we could change this to "*(ULONG *)Buffer = psa->pvData ? 0x2 : 0x0" and check the value during unmarshalling to be totally correct with the NDR-formatted data that we are writing. -- Rob Shearman
Re: widl: initialize _RetVal to avoid compiler warnings
2009/2/5 Christoph von Wittich :
> --- a/tools/widl/proxy.c
> +++ b/tools/widl/proxy.c
> @@ -327,7 +327,12 @@ static void gen_proxy(type_t *iface, const var_t
> *func, int idx,
> if (has_ret) {
> print_proxy( "" );
> write_type_decl_left(proxy, type_function_get_rettype(func->type));
> -print_proxy( " _RetVal;\n");
> +
> +/* Initialize _RetVal properly in order to avoid compiler warnings */
> +if (is_ptr(type_function_get_rettype(func->type)) ||
> is_array(type_function_get_rettype(func->type)))
> + print_proxy(" _RetVal = NULL;\n");
> +else
> + print_proxy(" _RetVal = 0;\n");
> }
> print_proxy( "RPC_MESSAGE _RpcMessage;\n" );
> if (has_ret) {
I think this is just hiding the problem. Can you give an example of
IDL which causes widl to generate code that causes compiler warnings?
--
Rob Shearman
Re: rpcrt4: Check for null endpoint in RpcServerUseProtseqEpExW
2009/2/6 Nikolay Sivov :
> Rob Shearman wrote:
>> 2009/2/5 Nikolay Sivov :
>>
>>> Changelog:
>>>- Check for null endpoint in RpcServerUseProtseqEpExW.
>>> Installed IE8 RC1 crashes on this call, parameter set is specified in
>>> test case.
>>>
>>> >From 89f889b2c7a754302b599884577b4eba22b3d235 Mon Sep 17 00:00:00 2001
>>> From: Nikolay Sivov
>>> Date: Thu, 5 Feb 2009 22:03:48 +0300
>>> Subject: Check for null endpoint in RpcServerUseProtseqEpExW
>>>
>>> ---
>>> dlls/rpcrt4/rpc_server.c |6 --
>>> dlls/rpcrt4/tests/rpc.c | 15 +++
>>> 2 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/dlls/rpcrt4/rpc_server.c b/dlls/rpcrt4/rpc_server.c
>>> index b6e058a..e9bed42 100644
>>> --- a/dlls/rpcrt4/rpc_server.c
>>> +++ b/dlls/rpcrt4/rpc_server.c
>>> @@ -775,8 +775,10 @@ RPC_STATUS WINAPI RpcServerUseProtseqEpExW( RPC_WSTR
>>> Protseq, UINT MaxCalls, RPC
>>> return status;
>>>
>>> EndpointA = RPCRT4_strdupWtoA(Endpoint);
>>> - status = RPCRT4_use_protseq(ps, EndpointA);
>>> - RPCRT4_strfree(EndpointA);
>>> + if (EndpointA) {
>>> + status = RPCRT4_use_protseq(ps, EndpointA);
>>> + RPCRT4_strfree(EndpointA);
>>> + }
>>>
>>
>> This isn't correct, I'm afraid. The protseq open function should
>> generate an endpoint name to use. For example, the ncacn_ip_tcp code
>> should select a random available TCP port.
>>
>>
>>> return status;
>>> }
>>>
>>> diff --git a/dlls/rpcrt4/tests/rpc.c b/dlls/rpcrt4/tests/rpc.c
>>> index bf538e4..b815acc 100644
>>> --- a/dlls/rpcrt4/tests/rpc.c
>>> +++ b/dlls/rpcrt4/tests/rpc.c
>>> @@ -830,6 +830,20 @@ static void test_UuidCreate(void)
>>> }
>>> }
>>>
>>> +static void test_RpcServerUseProtseqEpEx(void)
>>> +{
>>> +RPC_STATUS status;
>>> +RPC_POLICY policy;
>>> +static WCHAR protW[] = {'n','c','a','l','r','p','c',0};
>>> +
>>> +policy.Length= sizeof( policy );
>>> +policy.EndpointFlags = 0;
>>> +policy.NICFlags = 0;
>>> +
>>> +status = RpcServerUseProtseqEpExW(protW, 10, NULL, NULL, &policy);
>>> +ok(status == RPC_S_OK, "Expected RPC_S_OK, got %d\n", status);
>>> +}
>>>
>>
>> This is a good start, but you should also test the bindings created as
>> a result of doing this and then you will probably discover that the
>> fix to RpcServerUseProtseqEpExW isn't quite correct.
>>
>>
>>
>>> START_TEST( rpc )
>>> {
>>> static unsigned char ncacn_np[] = "ncacn_np";
>>> @@ -849,4 +863,5 @@ START_TEST( rpc )
>>> test_endpoint_mapper(ncalrpc, NULL, lrpc_endpoint);
>>> test_RpcStringBindingFromBinding();
>>> test_UuidCreate();
>>> +test_RpcServerUseProtseqEpEx();
>>> }
>>>
>>
>>
> Ok, I'll try to find out.
> Rob, could you suggest a good documentation source on RPC? I'm not
> experienced in it.
Sure, although this particular aspect isn't well documented. I did
find a couple of references, but you have to fill in the blanks to get
to the conclusion that RpcServerUseProtseqEpEx with a NULL endpoint
should generate an otherwise unused endpoint string:
http://www.opengroup.org/onlinepubs/9629399/chap2.htm#tagcjh_05_02_03
http://msdn.microsoft.com/en-us/library/aa378452(VS.85).aspx - "The
endpoint name is generated by the RPC run time or the operating
system."
The following may also be useful in future RPC issues, but not in this case:
http://msdn.microsoft.com/en-us/library/cc243560(PROT.10).aspx
--
Rob Shearman
Re: rpcrt4: Check for null endpoint in RpcServerUseProtseqEpExW
2009/2/5 Nikolay Sivov :
> Changelog:
>- Check for null endpoint in RpcServerUseProtseqEpExW.
> Installed IE8 RC1 crashes on this call, parameter set is specified in
> test case.
>
> >From 89f889b2c7a754302b599884577b4eba22b3d235 Mon Sep 17 00:00:00 2001
> From: Nikolay Sivov
> Date: Thu, 5 Feb 2009 22:03:48 +0300
> Subject: Check for null endpoint in RpcServerUseProtseqEpExW
>
> ---
> dlls/rpcrt4/rpc_server.c |6 --
> dlls/rpcrt4/tests/rpc.c | 15 +++
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/rpcrt4/rpc_server.c b/dlls/rpcrt4/rpc_server.c
> index b6e058a..e9bed42 100644
> --- a/dlls/rpcrt4/rpc_server.c
> +++ b/dlls/rpcrt4/rpc_server.c
> @@ -775,8 +775,10 @@ RPC_STATUS WINAPI RpcServerUseProtseqEpExW( RPC_WSTR
> Protseq, UINT MaxCalls, RPC
> return status;
>
> EndpointA = RPCRT4_strdupWtoA(Endpoint);
> - status = RPCRT4_use_protseq(ps, EndpointA);
> - RPCRT4_strfree(EndpointA);
> + if (EndpointA) {
> + status = RPCRT4_use_protseq(ps, EndpointA);
> + RPCRT4_strfree(EndpointA);
> + }
This isn't correct, I'm afraid. The protseq open function should
generate an endpoint name to use. For example, the ncacn_ip_tcp code
should select a random available TCP port.
> return status;
> }
>
> diff --git a/dlls/rpcrt4/tests/rpc.c b/dlls/rpcrt4/tests/rpc.c
> index bf538e4..b815acc 100644
> --- a/dlls/rpcrt4/tests/rpc.c
> +++ b/dlls/rpcrt4/tests/rpc.c
> @@ -830,6 +830,20 @@ static void test_UuidCreate(void)
> }
> }
>
> +static void test_RpcServerUseProtseqEpEx(void)
> +{
> +RPC_STATUS status;
> +RPC_POLICY policy;
> +static WCHAR protW[] = {'n','c','a','l','r','p','c',0};
> +
> +policy.Length= sizeof( policy );
> +policy.EndpointFlags = 0;
> +policy.NICFlags = 0;
> +
> +status = RpcServerUseProtseqEpExW(protW, 10, NULL, NULL, &policy);
> +ok(status == RPC_S_OK, "Expected RPC_S_OK, got %d\n", status);
> +}
This is a good start, but you should also test the bindings created as
a result of doing this and then you will probably discover that the
fix to RpcServerUseProtseqEpExW isn't quite correct.
> START_TEST( rpc )
> {
> static unsigned char ncacn_np[] = "ncacn_np";
> @@ -849,4 +863,5 @@ START_TEST( rpc )
> test_endpoint_mapper(ncalrpc, NULL, lrpc_endpoint);
> test_RpcStringBindingFromBinding();
> test_UuidCreate();
> +test_RpcServerUseProtseqEpEx();
> }
--
Rob Shearman
Re: widl: filter_func isn't used when USE_COMPILER_EXCEPTIONS is defined
2009/2/6 Christoph von Wittich :
> diff --git a/tools/widl/proxy.c b/tools/widl/proxy.c
> index 0dcb737..88c0e53 100644
> --- a/tools/widl/proxy.c
> +++ b/tools/widl/proxy.c
> @@ -122,10 +122,12 @@ static void init_proxy(const statement_list_t *stmts)
> print_proxy( "void *This;\n");
> print_proxy( "};\n");
> print_proxy( "\n");
> + print_proxy("#ifndef USE_COMPILER_EXCEPTIONS\n");
> print_proxy("static int __proxy_filter( struct __proxy_frame *__frame
> )\n");
> print_proxy( "{\n");
> print_proxy( "return (__frame->_StubMsg.dwStubPhase !=
> PROXY_SENDRECEIVE);\n");
> print_proxy( "}\n");
> + print_proxy("#endif /* USE_COMPILER_EXCEPTIONS */\n");
> print_proxy( "\n");
> }
I think a better solution to this would be to use proxy_filter as the
expression in the RpcExcept statements.
Alexandre, what do you think?
--
Rob Shearman
Re: Stub DDE interface for Progman.exe
2009/2/5 Jeremiah Flerchinger : > On Thu, 2009-02-05 at 18:58 +0100, Rolf Kalbermatter wrote: >> "Jeremiah Flerchinger" wrote: >> >> > The shell32.dll DDE callback isn't always initialized & handling >> > Progman calls for all apps. I've seen Progman calls pass >> > through the DDE_client and DDE_server of user32.dll instead >> > and never go through shell32.dll. >> >> That would IMHO only be possible if the shell executable, which normally is >> just explorer >> creating the actual desktop view had been replaced by a different executable >> that does >> not initialize the shell DDE interface properly. >> >> Rolf Kalbermatter >> > DdeConnect, DDeInitialize, and DDeDisconnect can be called directly by > including Windows.h and importing the calls from user32 > (http://msdn.microsoft.com/en-us/library/ms648745(VS.85).aspx). DDE > calls can then be made with SendMessage or PostMessage. > > I found a couple apps that appear to do this. Wine does not > automatically start explorer.exe or initialize the shell DDE interface, > nor does it route the DdeConnect routine in user32.dll to the DdeConnect > routine in the shell32.dll. The DDE interface in shell32 and user32 will > need to call the same set of routines from either method for full > functionality of DDE calls, including those to Progman. http://en.wikipedia.org/wiki/Library_(computing)#Dynamic_linking -- Rob Shearman
Re: Stub DDE interface for Progman.exe
2009/2/5 Jeremiah Flerchinger : > On Thu, 2009-02-05 at 18:12 +0000, Rob Shearman wrote: >> 2009/2/3 Dmitry Timoshkov : >> > "Jeremiah Flerchinger" wrote: >> > >> >> Stubs basic DDE interface of Progman.exe. Similar to Progman stub in >> >> shell32.dll. Both will need to be extended and a 3rd progman interface >> >> will need to be added in user32. >> > >> > Progman DDE interface should be implemented by shell32.dll, nowhere else. >> >> And it already is: ShellDDEInit. >> >> All it needs is some always running process to call it. It looks like >> explorer would be a good choice. >> > Is explorer always running in Wine? I know "wine explorer" does not run > ShellDDEInit when it starts. I thought most people were against > additional processes, but I want to see the Progman DDE interface > working for the different ways of calling it. Try running a simple winelib app with WINEDEBUG=+process set and see for yourself. > ShellDDEInit didn't exist before Win 2000 and was in Shdocw.dll and not > shell32.dll. Do we want to call ShellDDEInit in shell32 when Wine starts > (and ends) then? I'm not sure if there is a desire to allow native shell32 to be used in Wine. However, if there is it would be simple to use GetProcAddress with appropriate checks on the shell32 module that has been loaded. > It is supposed to be called only by the process that is > acting as the shell when it registers itself as a DDE host for the > shell. I don't think anything else would ever call it then. I can't work out what you mean by this. Perhaps you can rephrase it or describe in more detail what you mean. -- Rob Shearman
Re: [patch] rpcrt4_conn_np_read needs to expect and accept ERROR_MORE_DATA
2009/2/5 Luke Kenneth Casson Leighton : > http://bugs.winehq.org/show_bug.cgi?id=17263 The patch looks acceptable in theory. Please email the patch to wine-patches (or wine-devel) and I'll review the implementation of it. -- Rob Shearman
Re: Stub DDE interface for Progman.exe
2009/2/3 Dmitry Timoshkov : > "Jeremiah Flerchinger" wrote: > >> Stubs basic DDE interface of Progman.exe. Similar to Progman stub in >> shell32.dll. Both will need to be extended and a 3rd progman interface >> will need to be added in user32. > > Progman DDE interface should be implemented by shell32.dll, nowhere else. And it already is: ShellDDEInit. All it needs is some always running process to call it. It looks like explorer would be a good choice. -- Rob Shearman
Re: [PATCH] mshtml: Fixed last argument to MBtoWC
2009/2/4 Marcus Meissner :
> @@ -442,7 +442,7 @@ static LPWSTR get_url(void)
>
> if(size > sizeof(httpW) && !memcmp(url, httpW, sizeof(httpW))) {
> strcatW(url, v_formatW);
> -MultiByteToWideChar(CP_ACP, 0, GECKO_VERSION, -1, url+strlenW(url),
> -1);
> +MultiByteToWideChar(CP_ACP, 0, GECKO_VERSION, -1, url+strlenW(url),
> (size-strlenW(url))/sizeof(WCHAR));
Dividing a string length (which returns character count) by
sizeof(WCHAR) doesn't look right.
--
Rob Shearman
Re: A proposal for increased security in wine - respecting previously expressed needs
2009/1/29 Alex Villacís Lasso :
> Guillaume SH escribió:
>> Hi wine community,
>>
>> I took some time for reflexion following the thread "A step in the wrong
>> direction, in an ocean of steps in the right direction" and to the
>> explanations some of you kindly exposed to me.
>>
>> As a follow-up I am making a proposal.
>>
>> A - The proposal
>>
>> A1 - The core proposal
>>
>> All function callable from outside wine should implement, as the first
>> task they perform(1), a sanity parameter check. This check
>> hasn't to be systematic, only really used parameters should be checked
>> and only checks assuring those parameters can be safely used
>> the way they are in the function implementation (or in a function
>> called by it). Other check are superfluous and must be discarded.
>>
>> So to decline the proposal operationally, I will take an example(2) :
>>
>> BOOL WINAPI GetOverlappedResult(HANDLE hFile, LPOVERLAPPED
>> lpOverlapped, [...])
>>
>> if ( lpOverlapped == NULL )
>> {
>> #Call the function ExitWineCleanlyAndAdvertiseUser
>> }
>>
>>
>> This call being justified by the statement, following some lines later :
>> status = lpOverlapped->Internal;
>>
>>
>> The function ExitWineCleanlyAndAdvertiseUser being something like that :
>>
>> ExitWineCleanlyAndAdvertiseUser > be determined>
>>
>>#1 - Advertise user outputting a message, for example : "The
>> application you used present a defect. Using it directly expose you to some
>> security issue and indirectly expose others users to some other
>> security issue."
>>
>>#2 - Cleanly release all still allocated resources
>>
>>#3 - Cleanly exit wine
>>
>>
>> A2 - offering flexibility
>>
>> As I have understood that wine community is willing to be able to run
>> all applications written for a Windows platform,
>> even those relying on the worse behaviours of windows, I will propose
>> too to add a registry key for the purpose of enabling / disabling
>> wine "safe" mode.
>> In this case it would make sense that "safe" mode is the default, with
>> possibility to fall-back to "unsafe" mode when needed.
>>
>> B - Advantages / Drawbacks to the proposal
>>
>> Drawback of this solution I can think of are :
>> 1 - It is contrary to the current consensus
>> 2 - It implies a lot of work (even if this can be done bit by bit over a
>> long period time, direction is what matters here)
>> 3 - Detailed implementation of what I presented may very well not be as
>> simple as I imagine, or even impossible
>> 4 - Maybe all the reasons have not been expressed in the previous
>> thread, thus not considered here
>> 5 - It can go against the interest of the author of those apps relying
>> on Windows's bad behaviors (large firms for example)
>> 6 - It doesn't cover all security issue in wine and it doesn't cover at
>> all security issue in the calling app
>> 7 - Performance drop-down may be expected (0,01%, 0,1%, 1%, more ? I
>> don't know how to evaluate)
>>
>> Advantage of this solution I can think of are :
>> 1 - Top-notch level of service to user : I can be informed when I use an
>> unsafe software !
>> 2 - Encouragement to software industry : I must provide some clean and
>> safe software or wine can judge me "unsafe" (=promoting "best-practices")
>> 3 - Wine is better from it, so people will have a better opinion of wine
>> 4 - Goal reached by wine are far beyond "Windows behaviours on free-OS
>> platform"
>> 5 - Wine is safer so more people will want to use it and to promote its
>> usage
>>
>>
>> I think I will go no further than this proposal, so I leave the rest to you,
>> from simply ignoring it to demonstrate me than I'm wrong or applying a
>> variation,
>> Guillaume
>>
>> (1) TRACE, variable declaration or other task may come before though
>> (2) Please don't argue about the coding style, I am not a technical expert
>> (unlike you) and it would be off-topic
>>
>>
> I think I remember a discussion about a particular bug in which some
> version of an installer (InstallShield?) refused to work correctly
> because the wine version of one API call was checking a pointer
> parameter against NULL and returning an error code instead of crashing.
> If I recall correctly, it turned out that the installer *expected* the
> crash, and depended on the fault handler executing some code for its
> correct operation. So the NULL check was removed, the API now crashes
> with the invalid pointer (exactly like native) and that installer now
> works correctly. Maybe somebody who was involved in the actual fix can
> dig up a pointer to the relevant thread.
http://www.winehq.org/pipermail/wine-devel/2006-July/049830.html
http://bugs.winehq.org/show_bug.cgi?id=5384
--
Rob Shearman
Re: Ge van Geldorp : ole32/tests: Some Windows versions need the class to be registered.
2009/1/26 Alexandre Julliard :
> hr = StringFromCLSID(&CLSID_WineTest, &pszClsid);
> ok_ole_success(hr, "StringFromCLSID");
> -strcpy(buffer, "CLSID\\");
> +strcpy(buffer, "Software\\Classes\\CLSID\\");
> WideCharToMultiByte(CP_ACP, 0, pszClsid, -1, buffer + strlen(buffer),
> sizeof(buffer) - strlen(buffer), NULL, NULL);
> CoTaskMemFree(pszClsid);
> strcat(buffer, "\\InprocHandler32");
> -error = RegCreateKeyEx(HKEY_CLASSES_ROOT, buffer, 0, NULL, 0,
> KEY_SET_VALUE, NULL, &hkey, &dwDisposition);
> -ok(error == ERROR_SUCCESS, "RegCreateKeyEx failed with error %d\n",
> error);
> -error = RegSetValueEx(hkey, NULL, 0, REG_SZ, (const unsigned char
> *)"ole32.dll", strlen("ole32.dll") + 1);
> -ok(error == ERROR_SUCCESS, "RegSetValueEx failed with error %d\n",
> error);
> -RegCloseKey(hkey);
> +if (Register)
> +{
> +error = RegCreateKeyEx(HKEY_CURRENT_USER, buffer, 0, NULL, 0,
> KEY_SET_VALUE, NULL, &hkey, &dwDisposition);
> +ok(error == ERROR_SUCCESS, "RegCreateKeyEx failed with error %d\n",
> error);
> +error = RegSetValueEx(hkey, NULL, 0, REG_SZ, (const unsigned char
> *)"ole32.dll", strlen("ole32.dll") + 1);
> +ok(error == ERROR_SUCCESS, "RegSetValueEx failed with error %d\n",
> error);
> +RegCloseKey(hkey);
> +}
> +else
> +{
> +RegDeleteKey(HKEY_CURRENT_USER, buffer);
> +*strrchr(buffer, '\\') = '\0';
> +RegDeleteKey(HKEY_CURRENT_USER, buffer);
> +}
> +}
Ge,
This appears to have caused some regressions in running the tests on
older versions of Windows:
http://test.winehq.org/data/d0e77f8a47016c6f69e3a6b5a8bed8f620f1a8f1/nt4_cw-nt4sp6/ole32:marshal.html
versus before:
http://test.winehq.org/data/f63d950df75bdebbad3278790c6c9d11bfe1b226/nt4_cw-nt4sp6/ole32:marshal.html
While I can see how using HKEY_CURRENT_USER instead of
HKEY_CLASSES_ROOT can help developers using a standard user rather
than Admin on Windows, I don't see it as useful in general as a lot of
the tests emulate what installers do and hence depend on having full
access to the system.
--
Rob Shearman
Re: gdi32: Remove unneeded check. (Coverity)
2009/1/27 Kai Blin :
>
> lcdfilter = FT_LCD_FILTER_DEFAULT is hardcoded, so the check is always true.
>
> This fixes Coverity CID 816
>
> diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
> index 1ca6953..dfbcf2f 100644
> --- a/dlls/gdi32/freetype.c
> +++ b/dlls/gdi32/freetype.c
> @@ -4783,18 +4783,15 @@ DWORD WineEngGetGlyphOutline(GdiFont *incoming_font,
> UINT glyph, UINT format,
> (format == WINE_GGO_HRGB_BITMAP || format ==
> WINE_GGO_HBGR_BITMAP)?
> FT_RENDER_MODE_LCD: FT_RENDER_MODE_LCD_V;
>
> -if ( lcdfilter == FT_LCD_FILTER_DEFAULT || lcdfilter ==
> FT_LCD_FILTER_LIGHT )
> +if ( render_mode == FT_RENDER_MODE_LCD)
> {
> -if ( render_mode == FT_RENDER_MODE_LCD)
> -{
> -lpgm->gmBlackBoxX += 2;
> -lpgm->gmptGlyphOrigin.x -= 1;
> -}
> -else
> -{
> -lpgm->gmBlackBoxY += 2;
> -lpgm->gmptGlyphOrigin.y += 1;
> -}
> +lpgm->gmBlackBoxX += 2;
> +lpgm->gmptGlyphOrigin.x -= 1;
> +}
> +else
> +{
> +lpgm->gmBlackBoxY += 2;
> +lpgm->gmptGlyphOrigin.y += 1;
> }
>
> width = lpgm->gmBlackBoxX;
It doesn't make much sense to leave the lcdfilter variable in the
function if you're going to remove the if condition depending on it,
since it only has one other use.
However, It might have been the intention of the author of this code
that this is setting to be changed at compile time (or runtime through
a registry tweak) and have the code do the appropriate thing - in that
case it would have been better to put the variable at the top of the
file.
--
Rob Shearman
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
2009/1/26 Aurimas Fišeras :
> Saturn's error report:
> (INCONSISTENT USE) Possible null dereference of variable data+(count-1).
> This variable is checked for Null at lines: registry.c:1051
>
> Tested on Windows XP
>
> Changelog:
>advapi32: Fix potential NULL pointer dereference in RegSetValueExA
> [with test] (Saturn)
Excellent, this tool has spotted a corner-case that the code doesn't
handle correctly.
> From ea7773cc046992e327030fb99935afc5b25c1b4b Mon Sep 17 00:00:00 2001
> From: Aurimas Fischer
> Date: Mon, 26 Jan 2009 21:55:05 +0200
> Subject: advapi32: Fix potential NULL pointer dereference in RegSetValueExA
> [with test] (Saturn)
>
> ---
> dlls/advapi32/registry.c |1 +
> dlls/advapi32/tests/registry.c |4
> 2 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/dlls/advapi32/registry.c b/dlls/advapi32/registry.c
> index 52de6c5..88a89db 100644
> --- a/dlls/advapi32/registry.c
> +++ b/dlls/advapi32/registry.c
> @@ -1055,6 +1055,7 @@ LSTATUS WINAPI RegSetValueExA( HKEY hkey, LPCSTR name,
> DWORD reserved, DWORD typ
> else if (count && is_string(type))
> {
> /* if user forgot to count terminating null, add it (yes NT does
> this) */
> +if (!data) return ERROR_NOACCESS;
This should be moved before the comment to avoid the comment relating
to the wrong line.
> if (data[count-1] && !data[count]) count++;
> }
>
> diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c
> index b63b3e2..0e1b673 100644
> --- a/dlls/advapi32/tests/registry.c
> +++ b/dlls/advapi32/tests/registry.c
> @@ -383,6 +383,10 @@ static void test_set_value(void)
> test_hkey_main_Value_A(name2A, string2A, sizeof(string2A));
> test_hkey_main_Value_W(name2W, string2W, sizeof(string2W));
>
> +/* test RegSetValueExA with invalid parameters*/
> +ret = RegSetValueExA(hkey_main, name1A, 0, REG_SZ, NULL, 1);
> +ok(ret == ERROR_NOACCESS, "got %d (expected ERROR_NOACCESS)\n", ret);
From the source of RegSetValueExA, this appears to return
ERROR_INVALID_PARAMETER on Win9x, so this test case needs to be run on
Win9x and fixed if necessary to take this into account.
--
Rob Shearman
Re: [4/5] ddraw: Get rid of ICOM_INTERFACE.
2009/1/21 Henri Verbeet : > 2009/1/21 Stefan Dösinger : >>> I don't see the point. IDirect3DDevice7 is the primary vtable and >>> that's not going to change, ever. We also do things like this about >>> everywhere else, and I don't think ddraw should be special in that >>> regard. >> I don't see any 'primary' vtable in any ddraw object, but that's just my >> personal view. I also dislike casting between the iface and the impl, which >> is possible only because vtable is the first member of the impl struct. >> > Primary in the sense that it is the table that actually implements > most of the methods, including things like QueryInterface(), rather > than forwarding them to somewhere else. You're also relying on the IDirect3DDevice7 vtable field being the first field on the impl struct, so you should add a big comment to the impl struct warning that bad things will happen if the IDirect3DDevice7 vtable field isn't the first one. -- Rob Shearman
Re: rpcrt4: Add a stub implementation of NdrGetUserMarshalInfo.
2009/1/21 Hans Leidekker :
>
> diff --git a/dlls/rpcrt4/rpcrt4.spec b/dlls/rpcrt4/rpcrt4.spec
> index 2c86e68..56a55f6 100644
> --- a/dlls/rpcrt4/rpcrt4.spec
> +++ b/dlls/rpcrt4/rpcrt4.spec
> @@ -203,7 +203,7 @@
> @ stub NdrGetSimpleTypeBufferSize # wxp
> @ stub NdrGetSimpleTypeMemorySize # wxp
> @ stub NdrGetTypeFlags # wxp
> -@ stub NdrGetUserMarshallInfo
> +@ stdcall NdrGetUserMarshalInfo(ptr long ptr)
> @ stub NdrHardStructBufferSize #(ptr ptr ptr)
> @ stub NdrHardStructFree #(ptr ptr ptr)
> @ stub NdrHardStructMarshall #(ptr ptr ptr)
> diff --git a/dlls/rpcrt4/rpcrt4_main.c b/dlls/rpcrt4/rpcrt4_main.c
> index 537a005..1a6b61d 100644
> --- a/dlls/rpcrt4/rpcrt4_main.c
> +++ b/dlls/rpcrt4/rpcrt4_main.c
> @@ -1016,3 +1016,9 @@ RPC_STATUS RPC_ENTRY RpcCancelThreadEx(void*
> ThreadHandle, LONG Timeout)
> else
> return rpc_cancel_thread(target_tid);
> }
> +
> +RPC_STATUS RPC_ENTRY NdrGetUserMarshalInfo(ULONG *flags, ULONG level,
> NDR_USER_MARSHAL_INFO *mi)
> +{
> +FIXME("(%p, %u, %p)\n", flags, level, mi);
> +return RPC_X_INVALID_BUFFER;
> +}
Since this function has the prefix Ndr it should go in an ndr_*.c file
in rpcrt4, not one of the others (I would suggest ndr_marshall.c next
to the user marshal functions, which initialise the data structure
that it operates on). It also lacking a winapi comment.
--
Rob Shearman
Re: widl: fix a compiler error
2009/1/19 Austin English : > diff --git a/tools/widl/widltypes.h b/tools/widl/widltypes.h > index e6b0369..0851d2b 100644 > --- a/tools/widl/widltypes.h > +++ b/tools/widl/widltypes.h > @@ -539,6 +539,7 @@ static inline enum type_type > type_get_type_detect_alias(const type_t *type) > default: > assert(0); > } > +return 0; > } > > #define STATEMENTS_FOR_EACH_FUNC(stmt, stmts) \ Please put the return statement in the default case. -- Rob Shearman
Re: rpcrt4/tests: NdrVaryingArrayUnmarshall() is broken on Windows
2009/1/12 Alexandre Julliard : > "Ge van Geldorp" writes: > >> The failures in rpcrt4:server on Windows are caused by an access violation >> thrown in NdrVaryingArrayUnmarshall() called from get_5numbers(). I've >> created a minimum get_5numbers() test with Microsoft tools and that throws >> the same access violation. The access violation disappears when zeroing >> out the StubMsg before calling NdrClientInitializeNew(), so my guess is >> Windows NdrVaryingArrayUnmarshall() uses an uninitialized member. >> >> Changelog: >> NdrVaryingArrayUnmarshall() is broken on Windows > > If it's broken on Windows then there's no point in testing it, you can > simply remove it. There's value in the function working in Wine and testing the function stops it being inadvertently broken (probably by me). Microsoft may release a patch to fix it or fix it in a more recent version of Windows. -- Rob Shearman
Re: [PATCH 2/2] widl: Optimise the generated code by skipping calls calling the type function directly for some reference pointers.
2009/1/12 Alexandre Julliard :
> "Rob Shearman" writes:
>
>> Skip calling the Pointer marshalling/unmarshalling/buffer
>> sizing/freeing function in this case.
>>
>> Output code for calling union marshalling/unmarshalling/buffer
>> sizing/freeing functions.
>
> It doesn't work:
>
> ../../tools/widl/widl -I. -I. -I../../include -I../../include -D__WINESRC__
> -DREGISTER_PROXY_DLL -DPROXY_CLSID_IS="{ 0xb8da6310, 0xe19b, 0x11d0, { 0x93,
> 0x3c, 0x00, 0xa0, 0xc9, 0x0d, 0xca, 0xa9 } }" -p -P actxprxy_servprov_p.c
> actxprxy_servprov.idl
> widl: typetree.h:191: type_pointer_get_ref: Assertion `is_ptr(type)' failed.
> make[2]: *** [actxprxy_servprov_p.c] Aborted
> make[2]: *** Deleting file `actxprxy_servprov_p.c'
Sorry about that, sent the wrong version. Will send the updated version shortly.
--
Rob Shearman
Re: WIDL 'import' question
2009/1/11 Nikolay Sivov :
> Hi, I've a small question on using 'import' in .idl. Sorry if it's too
> dummy, it's my first try to use widl.
> Trying to get commoncontrols.idl port compile on Wine I've got the
> following problems -
> this file contains struct definitions already defined in commctrl.h.
>
> Adding
>
>import "commctrl.h"
>
> and removing after that all conflict definitions from commoncontrols.idl
> caused for me
>
> ../tools/widl/widl -I. -I. -I../include -I../include-h -H
> commoncontrols.h commoncontrols.idl
> ./prsht.h:39: error: syntax error, unexpected '{'
> make[1]: *** [commoncontrols.h] Error 1
> make[1]: Leaving directory `/home/mrlarch/wine/include'
> make: *** [include] Error 2
>
> showing that prsht.h isn't processed properly here
>
> 39static const WCHAR WC_PROPSHEETW[] = { 'S','y','s',
> 40 'P','r','o','p','e','r','t','y','S','h','e','e','t',0 };
>
> How could I get over that?
The IDL syntax that widl implements includes a subset of the C syntax,
however it is not practical to support the full C syntax (in
particular inline functions) and in this case widl doesn't like the
array initialiser. MIDL probably also suffers from similar limitations
and you'll see "#ifndef MIDL_PASS" in PSDK header files to work around
these sorts of issues. If you really have to treat commctrl.h as an
IDL file, you'll need to #ifdef out problem statements.
--
Rob Shearman
Re: [6/8] widl: Null-check pointers with [size_is]
2009/1/10 Michael Karcher :
> This matches midl behaviour. In
>
> void test1([out,size_is(x)] char str[], [in] int x);
> void test2([size_is(x), out] char* str, [in] int x);
>
> the parameter to test1 is not NULL-checked, but the one to test2 is.
> ---
> tools/widl/client.c|2 +-
> tools/widl/proxy.c | 15 ++-
> tools/widl/widltypes.h |1 +
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/tools/widl/client.c b/tools/widl/client.c
> index a23e298..0b10eb6 100644
> --- a/tools/widl/client.c
> +++ b/tools/widl/client.c
> @@ -59,7 +59,7 @@ static void check_pointers(const var_t *func)
>
> LIST_FOR_EACH_ENTRY( var, type_get_function_args(func->type), const
> var_t, entry )
> {
> -if (is_var_ptr(var) && cant_be_null(var))
> +if (is_var_declptr(var) && cant_be_null(var))
> {
> print_client("if (!%s)\n", var->name);
> print_client("{\n");
> diff --git a/tools/widl/proxy.c b/tools/widl/proxy.c
> index bb16b72..ce07ddc 100644
> --- a/tools/widl/proxy.c
> +++ b/tools/widl/proxy.c
> @@ -150,6 +150,11 @@ int is_var_ptr(const var_t *v)
> return is_ptr(v->type);
> }
>
> +int is_var_declptr(const var_t *v)
> +{
> + return is_declptr(v->type);
> +}
> +
Again, this function doesn't seem to improve readability of the code.
I would just move the checking of the type of v into cant_be_null.
> int cant_be_null(const var_t *v)
> {
> /* Search backwards for the most recent pointer attribute. */
> @@ -164,10 +169,10 @@ int cant_be_null(const var_t *v)
> if (is_user_type(type))
> return 0;
>
> - if (!attrs && is_ptr(type))
> + if (!attrs && is_declptr(type))
> {
> attrs = type->attrs;
> -type = type_pointer_get_ref(type);
> +type = get_deref_type(type);
> }
>
> while (attrs)
> @@ -180,7 +185,7 @@ int cant_be_null(const var_t *v)
> if (t == RPC_FC_RP)
> return 1;
>
> -if (is_ptr(type))
> +if (is_declptr(type))
> {
> if (type->type == RPC_FC_FP ||
> type->type == RPC_FC_OP ||
> @@ -188,7 +193,7 @@ int cant_be_null(const var_t *v)
> return 0;
>
> attrs = type->attrs;
> - type = type_pointer_get_ref(type);
> + type = get_deref_type(type);
> }
> else
> attrs = NULL;
It would be simpler if you just did:
if (is_ptr(type))
fc = type->type;
else
{
}
return fc == RPC_FC_RP;
Note: there appears to be a bug in the current code where it walks
pointers looking for the pointer attribute, instead of walking just
aliases, so that should probably be fixed too.
--
Rob Shearman
Re: [7/8] widl: move is_user_type from typegen to header
2009/1/10 Michael Karcher : > --- > tools/widl/header.c | 24 > tools/widl/header.h |2 ++ > tools/widl/typegen.c | 24 > tools/widl/typegen.h |1 - > 4 files changed, 26 insertions(+), 25 deletions(-) Although there are a lot of helper functions implemented in header.c & header.h, we shouldn't add any more. Ideally, there should be another file for these types of helper functions used for both .c and .h generation, but typegen.c is as good as any at the moment. It would be better just to move last_ptr and is_string_type into typegen.c/typegen.h, since that is where they are most used. -- Rob Shearman
Re: [8/8] widl: consider ptrs to user-marshalled types as pointers-to-nonpointers
2009/1/10 Michael Karcher :
> typdef [wire_marshal(wirefoo)] foo * pfoo;
> made widl incorrectly set the RPC_FC_P_DEREF bit for pfoo. But this
> declares a user-marshalled pointer, so the RPC runtime must not dereference
> it before passing it on to the UserFoo functions.
> ---
> tools/widl/header.h |3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/tools/widl/header.h b/tools/widl/header.h
> index 3f0eec1..f572440 100644
> --- a/tools/widl/header.h
> +++ b/tools/widl/header.h
> @@ -61,7 +61,8 @@ extern int is_const_decl(const var_t *var);
>
> static inline int last_ptr(const type_t *type)
> {
> -return is_ptr(type) && !is_declptr(type_pointer_get_ref(type));
> +const type_t * ref = type_pointer_get_ref(type);
> +return is_ptr(type) && (is_user_type(ref) || !is_declptr(ref));
> }
>
> static inline int last_array(const type_t *type)
Looks good.
--
Rob Shearman
Re: [5/8] widl: dereference operator in expr work on any declared pointer
2009/1/10 Michael Karcher : > If we take MIDL 6.00.0366 as reference, the following method type is > legal: > >void frobnicate([size_is(x),in,out] int * bar1, >[size_is(*bar1),out] int * bar2, >[in] int x); > > Without this patch, size_is(*bar1) would be rejected. > --- > tools/widl/expr.c |4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/widl/expr.c b/tools/widl/expr.c > index 7f0e26d..e943062 100644 > --- a/tools/widl/expr.c > +++ b/tools/widl/expr.c > @@ -468,8 +468,8 @@ static struct expression_type resolve_expression(const > struct expr_loc *expr_loc > break; > case EXPR_PPTR: > result = resolve_expression(expr_loc, cont_type, e->ref); > -if (result.type && is_ptr(result.type)) > -result.type = type_pointer_get_ref(result.type); > +if (result.type && is_declptr(result.type)) > +result.type = get_deref_type(result.type); > else > error_loc_info(&expr_loc->v->loc_info, "dereference operator > applied to non-pointer type in expression%s%s\n", >expr_loc->attr ? " for attribute " : "", Looks good, once you expand the dubious functions used here. Also, can you add a test for this in dlls/rpcrt4/tests/server.c? -- Rob Shearman
Re: [4/8] widl: add helper function get_deref_type
2009/1/10 Michael Karcher : > get_deref_type yields the type of array elements for arrays or the > pointed-to type of pointers. Most important, it works on anything > is_declptr returns true for. I don't believe that this type of helper function improves the readability of the code, given that it only replaces two or three lines of code and is only used in three places. -- Rob Shearman
Re: [3/8] widl: use base pointer type instead of default if not overwritten. (resend)
2009/1/10 Michael Karcher :
> ---
> tools/widl/proxy.c |5 +
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/tools/widl/proxy.c b/tools/widl/proxy.c
> index 4e69190..bb16b72 100644
> --- a/tools/widl/proxy.c
> +++ b/tools/widl/proxy.c
> @@ -182,6 +182,11 @@ int cant_be_null(const var_t *v)
>
> if (is_ptr(type))
> {
> + if (type->type == RPC_FC_FP ||
> + type->type == RPC_FC_OP ||
> + type->type == RPC_FC_UP)
> +return 0;
> +
> attrs = type->attrs;
> type = type_pointer_get_ref(type);
> }
Looks good. Eventually, I want to move this into
typegen.c:get_pointer_fc and in the future determine the pointer type
based on its attributes and whether it is a parameter or not.
Please send to wine-patches.
--
Rob Shearman
Re: [2/8] widl: Don't forget conformance info on [iid_is] void pointers. (resend)
2009/1/10 Michael Karcher : > --- > tools/widl/typegen.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/tools/widl/typegen.c b/tools/widl/typegen.c > index 2b0e414..7dfa0b2 100644 > --- a/tools/widl/typegen.c > +++ b/tools/widl/typegen.c > @@ -3046,7 +3046,7 @@ static void write_parameter_conf_or_var_exprs(FILE > *file, int indent, const char > } > break; > } > -else if (type->type == RPC_FC_IP) > +else if (type->type == RPC_FC_IP || type->type == 0) Please use is_void to make it clear what is being checked here. -- Rob Shearman
Re: [1/8] widl: fix handling of [local] functions without [call_as] (try 3)
2009/1/10 Michael Karcher : > Output NULL pointers into proxy table and STUB_FORWARDING_FUNCTION into > stub tables if a local function has no call_as function associated, just > as MIDL does. Looks fine to me. -- Rob Shearman
Re: WIDL and Windows rpcrt4
2009/1/7 Ge van Geldorp :
> Hi Rob,
>
> Is WIDL supposed to generate code that's compatible with Windows RPCRT4.DLL?
> Because it appears this is currently not the case. When you look at
> http://test.winehq.org you'll see that the rpcrt4:server test fails on
> pretty much any Windows version.
>
> I investigated a bit more, the failures are caused by an exception 0x6e6
> ("An internal error occurred in a remote procedure call (RPC)" thrown from
> NdrPointerBufferSize() in server_c.c function get_name(). The
> mingw-crosscompiled executable doesn't handle that exception which causes
> the process to crash. Since this is in WIDL-generated code, I decided to
> take a look at what MIDL generates for the same IDL file.
>
> It turns out that MIDL treats the name_t struct as a simple structure, while
> WIDL treats it as a complex structure. That made comparison a bit difficult.
> Since WIDL never seems to generate simple structures, I tried to convince
> MIDL to generate a complex structure too. After a bit of fiddling around
> (I'm no IDL expert...) I came up with the following type, a mix of name_t
> and sun_t:
>
> typedef struct
> {
> [string, size_is(size)] char *name;
> unsigned int size;
>
> [switch_is(s)] union
> {
>[case(SUN_I)] int i;
>[case(SUN_F1, SUN_F2)] float f;
>[case(SUN_PI)] int *pi;
> } u;
>
> int s;
> } complex_t;
>
> After staring at the WIDL-generated and MIDL-generated code for a while
> (which still differed quite a bit), I noticed something interesting: in the
> type format string, "unsigned int size" gets encoded as FC_ULONG by WIDL,
> but as FC_LONG by MIDL. I then went back to the server_c.c and server_s.c
> files generated by WIDL for the tests and changed the type format strings
> there (replaced the "unsigned int size" encoding from FC_ULONG by FC_LONG).
> With this change, the get_name() test passes :-). It then crashes later on
> with an access violation inside NdrVaryingArrayUnmarshall() in
> get_5numbers(), but let's take the problems one at a time...
>
> So it appears Windows RPCRT4.dll is not happy with FC_ULONG entries within a
> complex structure. I added a bunch of other primitive types to my complex_t
> structure to see how they are treated by MIDL. See attached complex.idl file
> and the MIDL-generated midl_complex_c.c file (also attached WIDL-generated
> widl_complex_c.c for completeness). It appears that MIDL simply drops the
> "unsigned" from any base type, at least within complex structures. "unsigned
> long" gets encoded as FC_LONG, "unsigned short" as FC_SHORT and "unsigned
> small" as FC_SMALL.
>
> Like I said, I'm no IDL expert. Does the stuff above make sense to you? Do
> you think it would be a good idea to change WIDL to generate the same
> encodings as MIDL?
Yes, and in fact Michael Karcher has already sent in a pair of patches
to do this:
http://www.winehq.org/pipermail/wine-patches/2009-January/067032.html
http://www.winehq.org/pipermail/wine-patches/2009-January/067031.html
However, they probably won't apply cleanly after my recent changes.
Michael, do you plan to rebase and resend these? If not, I can do
this.
--
Rob Shearman
Re: Unused function in quartz
2009/1/6 Francois Gouget : > In the following commit you added the OutputPin_DeliverNewSegment() > function to pin.c. However today it is unused. Is there any plan to use > it? Is it still relevant? Not, it looks like it has been superseded by InputPin_NewSegment. If Maarten agrees, then feel free to send a patch to remove it. > commit 0a6f11c88a8da49d80e4e7d8a0236ae4b36926b7 > Author: Robert Shearman > Date: Thu Sep 25 23:50:06 2003 + > > - A few cosmetic fixes. > - Various bug fixes. > - Add some OutputPin helpers. > - Add a new type of pin, PullPin. -- Rob Shearman
Re: [shell32/tests 1/2] ILFree() is only exported by ordinal on Win9x
2009/1/6 Paul Vriens : > Hi, > > For some reason this only shows up when I use my own cross compiled tests. > > Changelog > ILFree() is only exported by ordinal on Win9x ILFree is exported using -noname in Wine, so it should be imported by ordinal. You need to investigate further as to why it isn't working for you. I suggest to start by running "winedump dump -j import shell32_crosstest.exe". -- Rob Shearman
Re: widl: Make to interfaces RPC_FC_OP instead of RPC_FC_RP
2009/1/4 Michael Karcher : > This eliminates the wrong null pointer check for pointers to interfaces. This isn't correct. Object pointers are generated, I believe, in the case where we have an [in, out] unique pointer, since if the pointer has been set to NULL the previous memory needs to be freed (on the client side). The correct fix for the problem you see is to implement (yet another) check in cant_be_null. -- Rob Shearman
Re: widl: use base pointer type instead of default if not overwritten.
2009/1/4 Michael Karcher :
> diff --git a/tools/widl/proxy.c b/tools/widl/proxy.c
> index f0e3f1d..0e5cbaf 100644
> --- a/tools/widl/proxy.c
> +++ b/tools/widl/proxy.c
> @@ -185,6 +185,11 @@ int needs_null_check(const var_t *v)
>
> if (type)
> {
> + if (type->type == RPC_FC_FP ||
> + type->type == RPC_FC_OP ||
> + type->type == RPC_FC_UP)
> +return 0;
> +
>attrs = type->attrs;
>type = type->ref;
> }
Looks good, although we should always use the type set in the type
object, rather than trying to guess from the attributes. I plan to
clean this up in the near future so you don't need to change the patch
unless you want to.
--
Rob Shearman
Re: long in IDL files?
2009/1/4 Michael Stefaniuc : > Hello guys, > > i guess that we need to change long to LONG in IDL files too, right? > Or would that be something that widl could/should do automatically? long/unsigned long are explicitly defined by the DCE/RPC standard to be 32-bit types. In an ideal world, I would change widl to generate int16_t/uint16_t for short/unsigned short, int32_t/uint32_t for long/unsigned long and int64_t/uint64_t for hyper/unsigned hyper types, but the lack of stdint.h in the Windows world prevents this. Therefore, the best solution that I see at the moment is to change the "long" type in widl to output "int". This will allow the generated code to work correctly on both 32-bit and 64-bit Linux and Windows platforms (but not 16-bit platforms and possibly other platforms). -- Rob Shearman
Re: widl: fix handling of [local] functions without [call_as]
2009/1/3 Michael Karcher :
> @@ -548,8 +548,9 @@ static int write_proxy_methods(type_t *iface, int skip)
>if (iface->funcs) LIST_FOR_EACH_ENTRY( cur, iface->funcs, const func_t,
> entry ) {
> var_t *def = cur->def;
> if (!is_callas(def->attrs)) {
> + if (i != cur->idx ) error("widl internal error: method index
> mismatch");
>if (i) fprintf(proxy, ",\n");
> - if (skip) print_proxy( "0 /* %s_%s_Proxy */", iface->name,
> get_name(def));
> + if (skip || is_local(def->attrs)) print_proxy( "0 /* %s_%s_Proxy */",
> iface->name, get_name(def));
>else print_proxy( "%s_%s_Proxy", iface->name, get_name(def));
>i++;
> }
This will now cause methods with the [local] attribute and with a
matching [call_as] method to no longer be included in the function
table, which isn't what you want.
--
Rob Shearman
Re: kernel32: implement CPU detection for OpenBSD
2009/1/3 Austin English :
> diff --git a/dlls/kernel32/cpu.c b/dlls/kernel32/cpu.c
> index 63ca277..10a8e87 100644
> --- a/dlls/kernel32/cpu.c
> +++ b/dlls/kernel32/cpu.c
> @@ -544,7 +544,7 @@ VOID WINAPI GetSystemInfo(
> }
> fclose (f);
> }
> -#elif defined (__NetBSD__)
> +#elif defined (__NetBSD__) || defined(__OpenBSD__)
> {
> int mib[2];
> int value[2];
Why not just do something like the following:
#ifdef __OpenBSD___
size_t value[2];
#else
int value[2];
#endif
And then you wouldn't have to ifdef each sysctl call.
> @@ -557,31 +557,55 @@ VOID WINAPI GetSystemInfo(
> #ifdef CPU_FPU_PRESENT
> mib[1] = CPU_FPU_PRESENT;
> value[1] = sizeof(int);
> - if (sysctl(mib, 2, value, value+1, NULL, 0) >= 0)
> +#ifdef __OpenBSD__
> + if (sysctl(mib, 2, value, (size_t *)(value+1), NULL, 0) >= 0) {
> +#else
> + if (sysctl(mib, 2, value, value+1, NULL, 0) >= 0) {
> +#endif
...
--
Rob Shearman
Re: [shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.
2009/1/1 Michael Karcher : > Am Donnerstag, den 01.01.2009, 20:43 + schrieb Rob Shearman: >> No, the [string] attribute in this case in redundant and it should >> apply to the first pointer in the parameter. > Now, that makes sense. > >> I'm guessing by the >> change that you had to make that widl doesn't handle this too well... > Seems so. I don't want to hurt anyone, but I somehow get the impression > that the quality of widl is somewhere around works-for-me. The trouble in this case was a corner case that was only tested by some bad IDL. Corner cases are never easy to test for and the best way of fixing bugs is to eliminate them as much as possible. To that end I have some patches queued up that add more of an API to access the parse tree to try to make the generator code more robust. In particular, the C generator code could probably do with a function like type_get_ndr_type that deals with the nuances of detecting strings, user-types, context handles, interface pointers, etc and that would likely have fixed several of the issues you reported. In terms of testing, I want to find and fix these issues before users (i.e. other developers, like you) find them. To that end, I think a good test may be to run "widl -p" on as many of the files in include/ as possible and fix/log any issues that come up, and then to compile each file. There should be no errors in either case, since errors should be reported up front (i.e. even for just generating a header file or a syntax check). I also plan to write a fuzzer for widl someday - if anyone knows of a free fuzzer framework out there somewhere then that would be useful. -- Rob Shearman
Re: question on how to do language bindings to MSHTML's COM interface
2009/1/1 Luke Kenneth Casson Leighton : > folks, hi, > i have a rather intriguing issue i'd like to look at, which takes > quite a bit of explaining as to why i'd like to go about it - if > people are interested i can answer that, but for now i'll leave it at > this: > how, under linux, would i go about making an application that made > _use_ of MSHTML.DLL's functionality, via its COM / DCE/RPC / MSRPC > interface? > in other words, if you are familiar with gecko / XUL, how would i go > about making a gecko / XUL style of application, using MSHTML as the > basis, and, once that was achieved, would it stand a chance of > successfully running under vanilla windows, using MS's own version of > MSHTML? There are a number of parts to your question: 1. Is it OK for the application to be a winelib one (i.e. invoked via or linked to wine, rather than being "native")? 2. What language are you writing the application in? 3. Pretty much all interaction with mshtml happens through COM interfaces, so which interfaces are you interested in? > yes i _am_ fully aware that, underneath, according to the wiki page on > mshtml's implementation, mshtml underneath uses Gecko - but i don't > _want_ to use the Gecko / XUL interface (mostly because it already > exists! i don't like challenges that have already been done :) > specifically, i'd _really_ like to have python bindings to the COM > bindings to MSHTML - all under linux. > so there are some _really_ weird esoteric "sub-questions" involved, such as: > "what are the chances of porting pywin32 - specifically pywin32's COM > bindings - to linux, thanks to widl and friends"? > see http://sourceforge.net/projects/pywin32 > many thanks for any advice and information. We already build a typelib for mshtml using widl and pywin32 can use that to make bindings for use at runtime, so it "should" work, but I fail to see the need for it at the moment. At some point I'd like to see a Python output generator for widl so that it can directly generate Python code that will communicate to a remote process using DCE/RPC, but that comes second place to getting the C generator as close to perfect as possible at the moment. -- Rob Shearman
Re: [4/5] wined3d: Convert some BOOLs to bitfields in struct IWineD3DDeviceImpl.
2008/12/30 Henri Verbeet : > > > From f6e4f88407491db8bb53d22d526f69b9ff761aaf Mon Sep 17 00:00:00 2001 > From: Henri Verbeet > Date: Tue, 30 Dec 2008 14:56:49 +0100 > Subject: wined3d: Convert some BOOLs to bitfields in struct > IWineD3DDeviceImpl. > > Also fills a 3 byte hole. > --- > dlls/wined3d/device.c| 17 +-- > dlls/wined3d/nvidia_texture_shader.c |2 +- > dlls/wined3d/state.c |4 +- > dlls/wined3d/wined3d_private.h | 36 > - > 4 files changed, 26 insertions(+), 33 deletions(-) How many instances of this structure are likely to be in a process at any one time? It seems to me as though as any memory savings gained by making the BOOLs into bitfields will be taken up by increased code size. There is also the risk that there will be a small performance penalty for this and the other similar changes too. These kinds of optimisations need to be backed up by benchmarks, for both memory and performance. -- Rob Shearman
Re: ole32: Add HICON user marshalling stubs
2009/1/1 Michael Karcher : > diff --git a/dlls/ole32/ole32.spec b/dlls/ole32/ole32.spec > index 79376ac..f7db2b0 100644 > --- a/dlls/ole32/ole32.spec > +++ b/dlls/ole32/ole32.spec > @@ -122,6 +122,10 @@ > @ stdcall HBITMAP_UserMarshal(ptr ptr ptr) > @ stdcall HBITMAP_UserSize(ptr long ptr) > @ stdcall HBITMAP_UserUnmarshal(ptr ptr ptr) > +@ stdcall HICON_UserFree(ptr ptr) > +@ stdcall HICON_UserMarshal(ptr ptr ptr) > +@ stdcall HICON_UserSize(ptr long ptr) > +@ stdcall HICON_UserUnmarshal(ptr ptr ptr) > @ stub HBRUSH_UserFree > @ stub HBRUSH_UserMarshal > @ stub HBRUSH_UserSize The functions should be added in the correct place so that the list of functions remains sorted. -- Rob Shearman
Re: [shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.
2009/1/1 Michael Karcher : > Am Donnerstag, den 01.01.2009, 19:09 + schrieb Rob Shearman: >> 2009/1/1 Michael Karcher : >> > +[call_as(AddPropertySheetPages)] >> > +HRESULT RemoteAddPropertySheetPages(); > [...] >> > +[call_as(SendControlMsg)] >> > +HRESULT RemoteSendControlMsg(); >> > + > [...] >> > +[call_as(SetToolbarItems)] >> > +HRESULT RemoteSetToolbarItems(); >> These shouldn't be necessary. What is it that you're trying to fix? > Widl outputting vtable slot number mismatch errors when a local function > is present without a remote function being present also. But if I might > take a guess from my last experiences here: This is a widl bug that > should be fixed... I'm afraid so. >> > -HRESULT GetDropDownStatus( >> > +[local] HRESULT GetDropDownStatus( >> > [out] DWORD *pdwFlags, >> > -[out, string] LPWSTR *ppwszString); >> > +[out] LPWSTR *ppwszString); >> > + >> > +[call_as(GetDropDownStatus)] >> > +HRESULT RemoteGetDropDownStatus(); >> >> This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be >> remoted as is. > How does that work? ppwszString is a NULL-pointer-terminated array of > strings. MSDN says that this function actually just returns one string, not a NULL-pointer-terminated array of strings, so this should just work. > Does the [string] flag work for pointer arrays too? MSDN > doesn't say so. No, the [string] attribute in this case in redundant and it should apply to the first pointer in the parameter. I'm guessing by the change that you had to make that widl doesn't handle this too well... -- Rob Shearman
Re: [oleaut 1/3] include: add [unique] attributes to IPropertyBag::RemoteRead args
2009/1/1 Michael Karcher : > Am Donnerstag, den 01.01.2009, 18:53 + schrieb Rob Shearman: >> but MIDL ignores the pointer type for interface >> pointers and never outputs a NULL reference pointer check for them. So >> widl just needs to be fixed. > Is it OK for me to look at the output of MIDL? I believe so. > The proxy source code > files probably contain lots Microsoft boilerplate code, so I didn't dare > to do such a thing yet. -- Rob Shearman
Re: [oleaut 3/3] oleaut32: Implement IPropertyBag::Read proxying
2009/1/1 Michael Karcher : > Am Donnerstag, den 01.01.2009, 18:58 + schrieb Rob Shearman: >> 2009/1/1 Michael Karcher : >> > + hr = IPropertyBag_RemoteRead_Proxy(This, pszPropName, &outVariant, >> > pErrorLog, >> > + V_VT(pVar), pUnk); >> > + if(SUCCEEDED(hr)) >> > +hr = VariantCopy(pVar, &outVariant); >> > + >> > + return hr; >> >> You're leaking the memory in outVariant here, since VariantCopy does a >> deep copy of all of the data. Is there a reason you don't just pass >> pVar into IPropertyBag_RemoteRead_Proxy? > Yes. I want outVariant to be unmodified in the case of error. But that > might be wrong. Even if I pass pVar into IPropertyBag_RemoteRead_Proxy, > i have to take care of leaking, I think, as RemoteRead takes the variant > pointer as an out-only parameter, so I would have to VariantClear > outVariant before. But you are right that this might be the better > solution. According to NDR marshalling rules, even if a parameter is marked as [out]-only memory is still re-used so you don't have to clear the VARIANT before passing it into IPropertyBag_RemoteRead_Proxy. For this reason, if you don't decide to pass pVar straight into IPropertyBag_RemoteRead_Proxy you still have to do VariantInit on outVariant before passing it into IPropertyBag_RemoteRead_Proxy. -- Rob Shearman
Re: [shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.
2009/1/1 Michael Karcher :
> diff --git a/include/shobjidl.idl b/include/shobjidl.idl
> index a647798..c36f4ce 100644
> --- a/include/shobjidl.idl
> +++ b/include/shobjidl.idl
> @@ -452,6 +452,9 @@ interface IShellView : IOleWindow
> [in] LPFNSVADDPROPSHEETPAGE pfn,
> [in] LPARAM lparam);
>
> +[call_as(AddPropertySheetPages)]
> +HRESULT RemoteAddPropertySheetPages();
> +
> HRESULT SaveViewState();
> HRESULT SelectItem(
> [in] LPCITEMIDLIST pidlItem,
> @@ -582,6 +585,9 @@ cpp_quote("#endif")
> [in] LPARAM lParam,
> [in] LRESULT *pret);
>
> +[call_as(SendControlMsg)]
> +HRESULT RemoteSendControlMsg();
> +
> HRESULT QueryActiveShellView( [out] IShellView **ppshv );
> HRESULT OnViewWindowActive( [in] IShellView *pshv );
>
> @@ -590,6 +596,8 @@ cpp_quote("#endif")
> [in] LPTBBUTTONSB lpButtons,
> [in] UINT nButtons,
> [in] UINT uFlags);
> +[call_as(SetToolbarItems)]
> +HRESULT RemoteSetToolbarItems();
These shouldn't be necessary. What is it that you're trying to fix?
> }
>
>
> @@ -1441,9 +1449,12 @@ interface IAutoCompleteDropDown : IUnknown
> {
> cpp_quote("#define ACDD_VISIBLE 0x0001")
>
> -HRESULT GetDropDownStatus(
> +[local] HRESULT GetDropDownStatus(
> [out] DWORD *pdwFlags,
> -[out, string] LPWSTR *ppwszString);
> +[out] LPWSTR *ppwszString);
> +
> +[call_as(GetDropDownStatus)]
> + HRESULT RemoteGetDropDownStatus();
This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be
remoted as is.
>
> HRESULT ResetEnumerator();
> }
--
Rob Shearman
Re: [oleaut 3/3] oleaut32: Implement IPropertyBag::Read proxying
2009/1/1 Michael Karcher : > + hr = IPropertyBag_RemoteRead_Proxy(This, pszPropName, &outVariant, > pErrorLog, > + V_VT(pVar), pUnk); > + if(SUCCEEDED(hr)) > +hr = VariantCopy(pVar, &outVariant); > + > + return hr; You're leaking the memory in outVariant here, since VariantCopy does a deep copy of all of the data. Is there a reason you don't just pass pVar into IPropertyBag_RemoteRead_Proxy? -- Rob Shearman
Re: [oleaut 2/3] include: Add [unique] attribute to IPersistPropertyBag::Load arg
2009/1/1 Michael Karcher : > diff --git a/include/ocidl.idl b/include/ocidl.idl > index c40d155..d4979d7 100644 > --- a/include/ocidl.idl > +++ b/include/ocidl.idl > @@ -927,7 +927,7 @@ interface IPersistPropertyBag : IPersist > > HRESULT Load( > [in] IPropertyBag *pPropBag, > -[in] IErrorLog *pErrorLog); > +[in,unique] IErrorLog *pErrorLog); > > HRESULT Save( > [in] IPropertyBag *pPropBag, Same goes for this change. -- Rob Shearman
