- if (!pParameters) {
ERR("(%p) : Called with a NULL pParameters\n", This);- }
- *pParameters = This->createParms;
This is dereferencing a NULL pointer.
On 07/03/06, Ivan Gyurdiev ivg2@cornell.edu wrote:
- if (!pParameters) {
ERR("(%p) : Called with a NULL pParameters\n", This);- }
- *pParameters = This->createParms;
This is dereferencing a NULL pointer.
Yes. Native d3d does that as well. The NULL pointer check there is purely informational.
Am Dienstag, 7. März 2006 08:50 schrieb H. Verbeet:
On 07/03/06, Ivan Gyurdiev ivg2@cornell.edu wrote:
- if (!pParameters) {
ERR("(%p) : Called with a NULL pParameters\n", This);- }
- *pParameters = This->createParms;
This is dereferencing a NULL pointer.
Yes. Native d3d does that as well. The NULL pointer check there is purely informational.
Native D3D crashes if it's called with a NULL pointer? Are you sure that you shouldn't return something like D3DERR_INVALIDCALL if pParamters == NULL? Might be worth a test case if native really crashes.
On 07/03/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Native D3D crashes if it's called with a NULL pointer? Are you sure that you shouldn't return something like D3DERR_INVALIDCALL if pParamters == NULL?
Yes. I verified that with DirectX 9.0c on win2k.
H. Verbeet wrote:
On 07/03/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Native D3D crashes if it's called with a NULL pointer? Are you sure that you shouldn't return something like D3DERR_INVALIDCALL if pParamters == NULL?
Yes. I verified that with DirectX 9.0c on win2k.
This doesn't really make sense to me - the point of unspecified behavior if a parameter is NULL would be to speed up the function under contract with the caller. If you're already doing the check, why not handle it in some way other than crashing?
Anyway, I'm not familiar with wine development practices, so I wouldn't know whether this is the right thing to do - just wanted to point it out. I also see the same thing done in other wined3d patches.
On 07/03/06, Ivan Gyurdiev ivg2@cornell.edu wrote:
This doesn't really make sense to me - the point of unspecified behavior if a parameter is NULL would be to speed up the function under contract with the caller. If you're already doing the check, why not handle it in some way other than crashing?
Because we *want* to crash and get a nice backtrace. Since native d3d crashes when passed a NULL pointer, chances are that if we do get passed a NULL pParameters here, it is because we passed it to the application ourselves somewhere earlier. In the case of GetCreationParameters it's probably not very likely to get passed a NULL pointer anyway, but that's the general idea.
H. Verbeet schrieb:
On 07/03/06, Ivan Gyurdiev ivg2@cornell.edu wrote:
This doesn't really make sense to me - the point of unspecified behavior if a parameter is NULL would be to speed up the function under contract with the caller. If you're already doing the check, why not handle it in some way other than crashing?
Because we *want* to crash and get a nice backtrace.
hm maybe I'm missing something but afaik you will get a backtrace right back into the application code, which will help you exactly nothing (unless you have the source code of the game and the binary doesn't have its debugging information stripped which is very likely not the case). So you have to figure out where you might have returned something wrong by looking at the traces from _before_ GetCreationParameters. The backtrace itself won't give you _any_ clues.
Unless some app relies on this crashing behaviour(which I doubt), I don't think wine should implement bugs just because MS does it. Seems a bit obscure strategy to me to crash in case of an error instead of proper error checking/reporting.
Tuesday, March 7, 2006, 3:22:28 PM, Peter Beutner wrote:
H. Verbeet schrieb:
On 07/03/06, Ivan Gyurdiev ivg2@cornell.edu wrote:
This doesn't really make sense to me - the point of unspecified behavior if a parameter is NULL would be to speed up the function under contract with the caller. If you're already doing the check, why not handle it in some way other than crashing?
Because we *want* to crash and get a nice backtrace.
hm maybe I'm missing something but afaik you will get a backtrace right back into the application code, which will help you exactly nothing (unless you have the source code of the game and the binary doesn't have its debugging information stripped which is very likely not the case). So you have to figure out where you might have returned something wrong by looking at the traces from _before_ GetCreationParameters. The backtrace itself won't give you _any_ clues.
Unless some app relies on this crashing behaviour(which I doubt), I don't think wine should implement bugs just because MS does it. Seems a bit obscure strategy to me to crash in case of an error instead of proper error checking/reporting.
If anything Wine does crash in much more cases then native. Look at all the ntdll functions. Native never crash there (because all those are direct kernel calls). Wine on the other hand will crash whenever you have a bad pointer dereference.
So I don't see any reasons why we should check for NULL pointer here. It will still crash if pointer is bogus, but not NULL. Also having a crash in a debug trace helps to find the place where did it crash. If Wine just returns from this function, you never know where it will cause the real problem.
Vitaliy.
On Tue, 7 Mar 2006, Peter Beutner wrote: [...]
Unless some app relies on this crashing behaviour(which I doubt), I don't think wine should implement bugs just because MS does it. Seems a bit obscure strategy to me to crash in case of an error instead of proper error checking/reporting.
This has been discussed many times before (but maybe not in the recent past). There are two cases to consider:
* either some applications do pass NULL thus causing a segfault and then expect to catch that exception. As you said, this is pretty unlikely here but it has happened with other (non-DirectX) APIs. In this case, then Wine must segfault too otherwise the application will fail.
* or the application does not pass a NULL here because it would cause it to crash on Windows. But if we merely return an error the application will merrily go on, only to crash or report that DirectX does not work a while later. By then the source of the problem will be very hard to find due to all that happened since this function call. The point is that we need to fix the bug that causes the application to pass a NULL to Wine in the first place. And the best way to spot it, and thus fix it, is if Wine segfaults in the same way as Windows when given a NULL.
So segfaulting like Windows does is really the right thing to do.
It may warrant a comment in the code though, especially if there is a test with a message right before.
Francois Gouget schrieb:
- or the application does not pass a NULL here because it would cause
it to crash on Windows. But if we merely return an error the application will merrily go on, only to crash or report that DirectX does not work a while later. By then the source of the problem will be very hard to find due to all that happened since this function call. The point is that we need to fix the bug that causes the application to pass a NULL to Wine in the first place. And the best way to spot it, and thus fix it, is if Wine segfaults in the same way as Windows when given a NULL.
The whole point of my email was that a crash here doesn't help you to find the real problem. It only says you that the function was called with a NULL pointer as parameter. Well, grepping through the trace log where a DirectX function returned an error most probably will get you the same result. And with the ERR() message that was in the original patch it would have been even easier to spot. Anyway both cases do _not_ tell you where things went wrong in the first place. You have to work through the whole trace log before that call to check where something wrong could have been returned. It might be even a bug in the application itself (e.g. forgotten check after malloc), who knows ...
Or look at it from the view of a user developing a winelib application. I'm sure he will very much appreciate it when wine does a better parameter checking than MS and returns an error instead of a crash backtrace ;) Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
But as the patch already was commited, I think any further discussion is pointless.
Or look at it from the view of a user developing a winelib application. I'm sure he will very much appreciate it when wine does a better parameter checking than MS and returns an error instead of a crash backtrace ;)
Ooooh, don't I smell bait-and-switch here? :) Always good to learn from your adversaries, right?
Kuba
Hi,
On Wed, Mar 08, 2006 at 02:08:52PM +0100, Peter Beutner wrote:
Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
We're not a library. We're a very, very, very, very specific piece of software that is required to absolutely, positively fully emulate Windows to the closest extent possible (within practical limits, of course). As such debating whether to delay a crash by adding useless checks *that do not exist in Windows*(!) to *work around* strange behaviour most likely caused by our own non-conformant code is utterly pointless. If there's a problem that we need to be aware of, then we'd better get to know about it NOW, not 5 lines, not 3000 relay lines and not 10 minutes after it occurred and nobody ever remembers what the actual problem was.
Andreas Mohr
Hi Andreas Mohr schrieb:
On Wed, Mar 08, 2006 at 02:08:52PM +0100, Peter Beutner wrote:
Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
We're not a library.We're a very, very, very, very specific piece of software
Well, I remember I was told that wine is just another gui toolkit like gtk+ or qt *scnr*.
that is required to absolutely, positively fully emulate Windows to the closest extent possible (within practical limits, of course). As such debating whether to delay a crash by adding useless checks *that do not exist in Windows*(!) to *work around* strange behaviour most likely caused by our own non-conformant code is utterly pointless.
And I thought the whole idea behind error-checking was not just to delay the crash but to entirely prevent it and instead inform the user what and where something failed. I would prefer it if the application pops up a message "xyz failed" rather than a crash. (admittingly, in how far the application does this in a proper way is entirely out of our hands ;) )
Let's just hope the kernel devs don't adopt your "Better-crash-than-return-an-error" strategy :p
If there's a problem that we need to be aware of, then we'd better get to know about it NOW, not 5 lines, not 3000 relay lines and not 10 minutes after it occurred and nobody ever remembers what the actual problem was.
To repeat it, if that crash happens you are *already* "3000 relay lines" after the actually problem and you have *no* clue what the actual problem is.
Hi,
On Wed, Mar 08, 2006 at 03:49:55PM +0100, Peter Beutner wrote:
Hi Andreas Mohr schrieb:
We're not a library.We're a very, very, very, very specific piece of software
Well, I remember I was told that wine is just another gui toolkit like gtk+ or qt *scnr*.
Good memory ;) However I didn't state that, and I wouldn't want to ;)
that is required to absolutely, positively fully emulate Windows to the closest extent possible (within practical limits, of course). As such debating whether to delay a crash by adding useless checks *that do not exist in Windows*(!) to *work around* strange behaviour most likely caused by our own non-conformant code is utterly pointless.
And I thought the whole idea behind error-checking was not just to delay the crash but to entirely prevent it and instead inform the user what and where something failed. I would prefer it if the application pops up a message "xyz failed" rather than a crash. (admittingly, in how far the application does this in a proper way is entirely out of our hands ;) )
Can you mention an example of Windows popping up an error message "function foo baz called with invalid parameter ..., what do you want me to do?". See? Me neither...
And how do you want to """prevent""" anything?? Forget it...
Let's just hope the kernel devs don't adopt your "Better-crash-than-return-an-error" strategy :p
The kernel developers code according to their very own rules, they're the ones to design stuff properly and thus fully know error conditions and corner cases of the code they're designing from scratch. We *absolutely* don't - especially when considering MSDN "correctness" <expletive deleted>.
Any additional error checking in Wine that's *not done by Windows* to make (semi-)abnormal code flow keep running is A Very Bad Thing (tm).
If there's a problem that we need to be aware of, then we'd better get to know about it NOW, not 5 lines, not 3000 relay lines and not 10 minutes after it occurred and nobody ever remembers what the actual problem was.
To repeat it, if that crash happens you are *already* "3000 relay lines" after the actually problem and you have *no* clue what the actual problem is.
That may well be the case, but I sure as hell don't want to even increase my trouble by orders of magnitude by waiting a lot longer in this error case...
Andreas Mohr
Hi Andreas Mohr schrieb:
Hi, Can you mention an example of Windows popping up an error message "function foo baz called with invalid parameter ..., what do you want me to do?". See? Me neither...
heh, the message will more likely looks like "Failed to initialize {Direct3D/DirectDraw/OpenGL/Foo}" Which still leaves you puzzled what the hell actually went wrong, but at least shows that sometimes the return values are actually checked for errors ;)
Any additional error checking in Wine that's *not done by Windows* to make (semi-)abnormal code flow keep running is A Very Bad Thing (tm).
So if a windows function crashes with a given set of parameters the aim is to exactly replicate that behaviour? hmm seems I have to re-think my argument I usually tell people that bugs in windows are not neccessarily also present in wine. Let's hope there is never a exploitable bug that needs to be implemented in wine :p
If there's a problem that we need to be aware of, then we'd better get to know about it NOW, not 5 lines, not 3000 relay lines and not 10 minutes after it occurred and nobody ever remembers what the actual problem was.
To repeat it, if that crash happens you are *already* "3000 relay lines" after the actually problem and you have *no* clue what the actual problem is.
That may well be the case, but I sure as hell don't want to even increase my trouble by orders of magnitude by waiting a lot longer in this error case...
So let's remove the dozens of NULL pointer checks already in place ;) And better also the whole exception handling which gives applications the chance to carry on execution even in case of a segmentation fault, which highly increases the trouble to find the original source of the problem *scnr*.
Peter Beutner p.beutner@gmx.net writes:
Or look at it from the view of a user developing a winelib application. I'm sure he will very much appreciate it when wine does a better parameter checking than MS and returns an error instead of a crash backtrace ;) Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
It's just the opposite actually. A good library (which the MS ones certainly aren't...) should crash on bad pointers, so that bugs can be found and fixed. Hiding bugs and trying to stumble along is the MS way, this is what leads to idiocy like having an exception handler in lstrlen(). So if in this case Windows doesn't have a check we certainly don't want one either.
Alexandre Julliard schrieb:
Peter Beutner p.beutner@gmx.net writes:
Or look at it from the view of a user developing a winelib application. I'm sure he will very much appreciate it when wine does a better parameter checking than MS and returns an error instead of a crash backtrace ;) Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
It's just the opposite actually. A good library (which the MS ones certainly aren't...) should crash on bad pointers, so that bugs can be found and fixed. Hiding bugs and trying to stumble along is the MS way, this is what leads to idiocy like having an exception handler in lstrlen(). So if in this case Windows doesn't have a check we certainly don't want one either.
I fail to see in how far returning an error is hiding a bug. Imho if the library detects an error[*] it should pass the information to the application and let the application handle it. The library doesn't know if the application maybe can cope with it. Just ultimately crash is a bad idea imo.
Just take the example you have written something in Office and the given behaviour(passing a NULL pointer) happens when you click on the print button. If it crashes your work is lost, if it returns an error( and the app handles it in a sane way ) you just won't be able to print it, but may save it and print it somehow else. In that case my main concern is certainly not that a crash helps me to find a bug, but to don't loose my data ;)
[*] Obviously you can't check against every possible failure. And it certainly may be a bit too much to check against bad pointers in general, but a NULL pointer check is cheap, easy and certainly doable.
Peter Beutner p.beutner@gmx.net writes:
Just take the example you have written something in Office and the given behaviour(passing a NULL pointer) happens when you click on the print button. If it crashes your work is lost, if it returns an error( and the app handles it in a sane way ) you just won't be able to print it, but may save it and print it somehow else. In that case my main concern is certainly not that a crash helps me to find a bug, but to don't loose my data ;)
Your mistake is to assume that the app will handle the error in a sane way. That's extremely unlikely, especially for things like bad pointers since the app obviously didn't expect to be passing bad pointers. If the app really wants to handle that sort of error it can always add an exception handler around the call; so not trapping the error in the library doesn't prevent the app from catching the error, it just avoids forcing a lot of useless checks on well-written apps.
And it's not clear at all that crashing is more likely to lose data than not crashing. With the Windows way, the app will merrily continue working with corrupt data, which gives it a lot more opportunities to add garbage to your data without noticing, than if it crashed right away before having done too much damage.
Alexandre Julliard schrieb:
Your mistake is to assume that the app will handle the error in a sane way. That's extremely unlikely, especially for things like bad pointers since the app obviously didn't expect to be passing bad pointers. If the app really wants to handle that sort of error it can always add an exception handler around the call; so not trapping the error in the library doesn't prevent the app from catching the error, it just avoids forcing a lot of useless checks on well-written apps.
And it's not clear at all that crashing is more likely to lose data than not crashing. With the Windows way, the app will merrily continue working with corrupt data, which gives it a lot more opportunities to add garbage to your data without noticing, than if it crashed right away before having done too much damage.
I surrender, you convinced me ;). Sorry for the noise.
On Wed, 8 Mar 2006, Peter Beutner wrote:
Francois Gouget schrieb:
- or the application does not pass a NULL here because it would cause
it to crash on Windows. But if we merely return an error the application will merrily go on, only to crash or report that DirectX does not work a while later. By then the source of the problem will be very hard to find due to all that happened since this function call. The point is that we need to fix the bug that causes the application to pass a NULL to Wine in the first place. And the best way to spot it, and thus fix it, is if Wine segfaults in the same way as Windows when given a NULL.
The whole point of my email was that a crash here doesn't help you to find the real problem.
And you are wrong there. When an application crashes you have a very clear point where you know things went very wrong. This point is very clearly identified by the SEH traces and the winedbg backtrace. This information makes it *much* easier to find the original bug. And this is from experience.
When an application doesn't crash but doesn't do what you want either, it's hard to even know which part of the log is relevant to the problem.
Imo a library is supposed to validate given parameters as much as possible and rather return an error to the caller than to crash.
No. A library can say that a given function must not be caled with a NULL parameter in which case it is perfectly valid to crash if that rule is violated. There's no point for both the callee and the caller to check for NULL parameters.
Let's just hope the kernel devs don't adopt your "Better-crash-than-return-an-error" strategy :p
The design of kernels, servers, and other pieces of code that have to deal with untrusted data (e.g. browsers), is completely different from that of general purpose libraries. Trying to apply the rules of one to the other will only give bad results.
And, as was pointed out before, Wine is in yet a different situation since its number one requirement is compatibility with the Win32 API rather than breaking clean with it and 'doing things the right way' (whatever that is).
H. Verbeet wrote:
On 07/03/06, Stefan Dösinger stefandoesinger@gmx.at wrote:
Native D3D crashes if it's called with a NULL pointer? Are you sure that you shouldn't return something like D3DERR_INVALIDCALL if pParamters == NULL?
Yes. I verified that with DirectX 9.0c on win2k.
The Retail version of the dll might not check the parameters but the Debug version probably does.
-
Alexandre Julliard -
Andreas Mohr -
Francois Gouget -
H. Verbeet -
Ivan Gyurdiev -
Kuba Ober -
Peter Beutner -
Robert Reif -
Stefan Dösinger -
Vitaliy Margolen