Sorry, accidentally hit send before I was done writing. Continuing:
> + DWORD bytesSent = 0;
Not to bikeshed style, but being consistent within the same code would at least be appreciated. (And in new code we tend to avoid camel case.)
> + s = socket(AF_INET, SOCK_DGRAM, 0);
Let's explicitly use IPPROTO_UDP here, please. It's also possible to create raw or ICMP DGRAM sockets, for which sending to port zero actually does something.
> + todo_wine ok(bytesSent == sizeof(buf), "Failed to send full data(%lu) only sent(%lu)\n", (unsigned long) sizeof(buf), (unsigned long) bytesSent);
No need to cast; you can use %Iu for expressions of type size_t, and %lu for DWORD.
From patch 2/2:
> +static int is_port0( const union unix_sockaddr *uaddr)
This returns BOOL.
> +{
> + switch (uaddr->addr.sa_family)
> + {
> + case AF_INET:
> + {
> + return uaddr->in.sin_port == 0;
> + }
> +
> + case AF_INET6:
> + {
> + return uaddr->in6.sin6_port == 0;
> + }
> +
No need for these braces.
> + #ifdef HAS_IPX
> + case AF_IPX:
> + {
> + return uaddr->ipx.sipx_port == 0;
> + }
> + #endif
Is this right? Unless we can confirm it's correct I'd leave this out.
> +
> + #ifdef HAS_IRDA
> + case AF_IRDA:
> + {
> + return FALSE;
> + }
> + #endif
> +
> + case AF_UNSPEC:
> + return FALSE;
There's no need for these; the default case already covers this.
> +
> + default:
> + return FALSE;
> + }
> +}
> @@ -1018,6 +1053,7 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async )
> hdr.msg_iov = async->iov + async->iov_cursor;
> hdr.msg_iovlen = async->count - async->iov_cursor;
>
> +
> while ((ret = sendmsg( fd, &hdr, async->unix_flags )) == -1)
> {
> if (errno == EISCONN)
Spurious whitespace change here.
> @@ -1028,7 +1064,24 @@ static NTSTATUS try_send( int fd, struct async_send_ioctl *async )
> else if (errno != EINTR)
> {
> if (errno != EWOULDBLOCK) WARN( "sendmsg: %s\n", strerror( errno ) );
> - return sock_errno_to_status( errno );
> +
> + if(errno == EINVAL && is_port0(&unix_addr)){
> +
> + /*
> + * Some Windows applications(Spellforce 3 is known to) send to port 0.
> + * This causes 'sendmsg' to throw a EINVAL error on Windows this does nothing but consume the data.
> + * This workaround says we successfully sent data even though we didnt send anything.
> + * Matching the Windows behaviour, making the program work(in theory).
> + */
Comment spacing is mangled here.
> + ret = 0;
> + for(ssize_t i = 0; i < hdr.msg_iovlen; i++){
> + ret += hdr.msg_iov[i].iov_len;
> + }
> + WARN("Attempting to send to port 0, skipping over %zd bytes\n", ret);
> + break;
> + }else{
> + return sock_errno_to_status( errno );
> + }
> }
This works, but I'm not sure it's the place we want to handle this. For one thing, I'm not sure all platforms are going to return EINVAL here.
Probably better is to either handle it in try_send() before sending, or handle it in sock_send() in about the same place. (I think it'd need to be after the server call, though I haven't properly checked whether the server needs to do anything important here.) Note that you'll also explicitly want to check the protocol here in that case, so we only catch UDP sockets with this logic.
Also, please try to match the surrounding code style wrt things like brace placement and spacing.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/2100#note_23505
The prefix of patch 1/2 should be changed to "ws2_32/tests". The subject is not quite a grammatical sentence; I'd just write "Check if sending to port 0 succeeds."
> + char buf[12] = "hello world";
No reason to specify the buffer size; you can just write "char buf[]".
> + DWORD bytesSent = 0;
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/2100#note_23489
QueryInterface should set *out to NULL on failure, as it seems to do in many different places.
This seems to be a regression from !888.
One old game started to crash on startup with this merge request.
I bisected it and it pointed to the commit 131ada052a2dbec66df695ce536ca048a2bd9174.
--
v4: wmvcore/tests: check out value for NULL in check_interface
https://gitlab.winehq.org/wine/wine/-/merge_requests/2130
On Wed Feb 8 19:01:54 2023 +0000, **** wrote:
> Paul Gofman replied on the mailing list:
> ```
> On 2/8/23 12:55, Rémi Bernon (@rbernon) wrote:
> >
> > USR1 / USR2 / INT for instance could be blocked in non-Wine threads,
> raising them again until they reach a Wine thread.
> I think USR1 may only be received by a non-Wine thread if it was
> originated by some native library and not by wineserver which sends that
> only to threads it knows? In that case, not sure how that can be handled
> better in a Wine thread?
> Also, is there a good way to block signals in threads which are created
> by native libraries, I think we don't know anything about those threads?
> ```
Yes, what Paul said. If a non-Wine thread gets USR1 or USR2 it didn't come from us, and handing it to a Wine thread is not going to do the right thing.
Note that signal masks are per-thread, but according to my understanding signal *actions* aren't.
QUIT is supposed to kill the entire process by default, not just a thread, so just killing that thread doesn't seem right. I suppose we *could* exit() in that case, but QUIT also dumps core by default, so it doesn't seem quite right to do a clean exit either. At least not right enough to be worth the extra complexity.
Rethrowing INT to a Wine thread is... nontrivial, but possible, I guess? I'd rather leave it for a follow-up patch if it's really worth doing.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_23484
On Wed Feb 8 18:55:31 2023 +0000, Zebediah Figura wrote:
> > Should we really crash on any unexpected signal?
> All of the signals we catch are signals for which the default action
> specified by POSIX is to terminate. In the case of TRAP, SEGV, ILL, BUS
> we have no choice. That might be true for FPE too but I can't remember;
> but either way, either the program was expecting an exception and won't
> get it, or it was going to crash anyway.
> In the case of ABRT, QUIT, INT not terminating seems like the wrong
> thing to do (and we can't handle them the normal way because we don't
> have a TEB and can't contact the server).
> We could potentially ignore USR1 and USR2, but that doesn't seem great
> for stability. Whatever was sending them had a reason to do so, and
> isn't going to function correctly.
> Perhaps more saliently, we were already crashing on unexpected signals,
> simply by virtue of trying to access the TEB and then dereferencing it.
> This patch just makes it more obvious.
Well we currently crash indeed but I don't think it is intentional. Making it explicit would make it look more like it is.
I'd think that instead we should block the signals in the threads that cannot handle them, or handle them in a way that doesn't require a TEB.
USR1 / USR2 / INT for instance could be blocked in non-Wine threads, raising them again until they reach a Wine thread. I think QUIT should be handled properly by calling `pthread_cancel` or `pthread_exit` and letting the thread cleanup properly.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/2126#note_23475
Without this patch the process will silently die.
I managed to trigger this while trying to trace Child of Light with Renderdoc,
the latter of which crashed in its own "TargetControlServerThread".
This, confusingly, manifested in the game restarting itself without the Ubisoft
overlay; apparently the game or one of its launchers was capable of recognizing
when the process had died and restarting it, but would not try to inject the
overlay a second time. I have not investigated the cause of the crash; it is not
unlikely that it resulted from the overlay injection (despite the fact that that
should only directly affect PE code.)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/2126
While running in XCode's profiler, I noticed two memory leaks in `interp_redim_preserve`.
After looking at `interp_redim`, the `bounds` structure is freed.
I've updated `interp_redim_preserve` to free `bounds` when the array is NULL and not NULL.
--
v5: vbscript: Fix memory leak in Split()
https://gitlab.winehq.org/wine/wine/-/merge_requests/2132
While running in XCode's profiler, I noticed two memory leaks in `interp_redim_preserve`.
After looking at `interp_redim`, the `bounds` structure is freed.
I've updated `interp_redim_preserve` to free `bounds` when the array is NULL and not NULL.
--
v4: vbscript: Fix memory leak in interp_redim_preserve
https://gitlab.winehq.org/wine/wine/-/merge_requests/2132
The Rutoken driver installer tries to start this service, and fails if it
doesn't exist.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54396
--
v2: scardsvr: Add stub service.
wine.inf: Always use FLG_ADDREG_APPEND for SvcHost entries.
setupapi: Create the registry value if it doesn't exist in append_multi_sz_value().
setupapi: Fail installation when trying to append to a registry value of the wrong type.
setupapi/tests: Add tests for FLG_ADDREG_APPEND.
https://gitlab.winehq.org/wine/wine/-/merge_requests/2073
Because of the change introduced in f21693b2, SM1 scalars and vectors were not longer getting the correct writemask when they are allocated, so this is fixed.
Also, the mapping of sm1 src register swizzles is moved outside `write_sm1_instruction()` since there are some instructions that don't do this, remarkably dp2add. This is fixed.
Before the last patch we are writing the operation as:
```
dp2add r0.x, r1.x, r0.x, r2.x
```
and now it is:
```
dp2add r0.x, r1.xyxx, r0.xyxx, r2.x
```
dp2add now has its own function, `write_sm1_dp2add()`, since it seems to
be the only instruction with this structure.
Ideally we would be using the default swizzles for the first two src arguments:
```
dp2add r0.x, r1, r0, r2.x
```
since, according to native's documentation, these are supported for all sm < 4.
But using default swizzles whenever is possible -- along with following the conversion of repeating the
last component of the swizzle when fewer than 4 components are to be
specified -- has a higher scope. Probably would involve modifying
`hlsl_swizzle_from_writemask()` and `hlsl_map_swizzle()`.
--
v3: vkd3d-shader/hlsl: Fix SM1 dp2add swizzles.
vkd3d-shader/hlsl: Map SM1 src swizzles outside write_sm1_instruction().
vkd3d-shader/hlsl: Set writemasks correctly for SM1 scalar and vector types.
vkd3d-shader/hlsl: Expect component count in allocate_register().
vkd3d-shader/hlsl: Rename 'component_count' arguments to 'reg_size'.
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/81