bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

classic Classic list List threaded Threaded
6 messages Options
Adrien Ollier Adrien Ollier
Reply | Threaded
Open this post in threaded view
|

bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

Hello everybody,

working on bug #74702 led me to read file core/vcl/source/outdev/nativecontrols.cxx.

I think there is an issue here:



If mpGraphics == nullptr and AcquireGraphics() == false, then the second if does not return false and we execute the instruction of the return statement but this will lead to a crash (because mpGraphics is false in this scenario).

If the second if is correct, then the last instruction sould be:
​return mpGraphics ? mpGraphics->IsSupported(nType, nPart) : false;

What do you think about that ? Do you agree with me ?

Adrien Ollier

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Adrien Ollier Adrien Ollier
Reply | Threaded
Open this post in threaded view
|

RE: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

errata


De : Adrien Ollier
Envoyé : lundi 15 avril 2019 09:33
À : [hidden email]
Objet : bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)
 
Hello everybody,

working on bug #74702 led me to read file core/vcl/source/outdev/nativecontrols.cxx.

I think there is an issue here:



If mpGraphics == nullptr and AcquireGraphics() == false, then the second if does not return false and we execute the instruction of the return statement but this will lead to a crash (because mpGraphics is false nullptr in this scenario).

If the second if is correct, then the last instruction sould be:
​return mpGraphics ? mpGraphics->IsSupported(nType, nPart) : false;

What do you think about that ? Do you agree with me ?

Adrien Ollier

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

In reply to this post by Adrien Ollier
On 15.04.2019 10:33, Adrien Ollier wrote:

> Hello everybody,
>
> working on bug #74702
> <https://bugs.documentfoundation.org/show_bug.cgi?id=74702> led me to
> read file core/vcl/source/outdev/nativecontrols.cxx.
>
> I think there is an issue here:
>
>
>
> If mpGraphics == nullptr and AcquireGraphics() == false, then the second
> if does not return false and we execute the instruction of the return
> statement but this will lead to a crash (because mpGraphics is false in
> this scenario).

No, please check what AcquireGraphics() does. Namely, it is used here to
acquire mpGraphics when it's not yet available. In fact, lines 166-167
are equivalent to this: "if (!mpGraphics) AcquireGraphics(); if
(!mpGraphics) return false;"

--
Best regards,
Mike Kaganski
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

On 15.04.2019 10:44, Adrien Ollier wrote:
> OutputDevice::AcquireGraphics() is a pure virtual function, so we cannot
> know in the general case what it does.
> And what is written is not really equivalent to what you wrote because
> if AcquireGraphics() is false, there is not a second check for
> mpGraphics. That's why it is an issue.

AcquireGraphics() is not just "any function". Despite someone could of
course implement it to play poker, its purpose is to acquire mpGraphics
and return if it succeeded. Failing that is programmer's error breaking
contract. There's no use to introduce that kind of checks here. Please
see existing implementations, like VirtualDevice::AcquireGraphics() in
vcl/source/gdi/virdev.cxx.

--
Best regards,
Mike Kaganski
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Kaganski Mike Kaganski Mike
Reply | Threaded
Open this post in threaded view
|

Re: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

In reply to this post by Kaganski Mike
On 15.04.2019 10:44, Adrien Ollier wrote:
> And what is written is not really equivalent to what you wrote because
> if AcquireGraphics() is false, there is not a second check for mpGraphics

Also note that when AcquireGraphics() is false (telling it could not
acquire mpGraphics successfully), we return false. It returning true
means that now mpGraphics is non-nullptr, and then we proceed.

--
Best regards,
Mike Kaganski
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Jan-Marek Glogowski Jan-Marek Glogowski
Reply | Threaded
Open this post in threaded view
|

Re: bug 74702 - Issue with bool OutputDevice::IsNativeControlSupported(ControlType, ControlPart)

In reply to this post by Kaganski Mike
Chiming in a little late.

Am 15.04.19 um 09:50 schrieb Kaganski Mike:

> On 15.04.2019 10:44, Adrien Ollier wrote:
>> OutputDevice::AcquireGraphics() is a pure virtual function, so we cannot
>> know in the general case what it does.
>> And what is written is not really equivalent to what you wrote because
>> if AcquireGraphics() is false, there is not a second check for
>> mpGraphics. That's why it is an issue.
>
> AcquireGraphics() is not just "any function". Despite someone could of
> course implement it to play poker, its purpose is to acquire mpGraphics
> and return if it succeeded. Failing that is programmer's error breaking
> contract. There's no use to introduce that kind of checks here. Please
> see existing implementations, like VirtualDevice::AcquireGraphics() in
> vcl/source/gdi/virdev.cxx.
>

This is not the nicest API - probably the bool function should be renamed, or
also return the SalGraphics* for consistency; but then naming is hard...

$ git grep AcquireGraphics include/ vcl/inc/
include/vcl/outdev.hxx:    virtual bool AcquireGraphics() const = 0;
vcl/inc/salframe.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salprn.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salvd.hxx:    virtual SalGraphics* AcquireGraphics() = 0;

I was interested in the fact that the bool variant is const, which is a lie.
Looking at the code it's not true and in fact ~ half of OutputDevice's member
variables are mutable. That's probably done to do some lazy init for other const
functions, quite probably all the const font based ones. Don't know if it buys a
lot of time for some usage, or should be factored out somehow... Anyway, I did:

$ git log -G AcquireGraphics include/vcl/outdev.hxx

commit 665bc42d7504c3896a485c8bab17b8eff7d119df
Author: Chris Sherlock <[hidden email]>
Date:   Wed Apr 23 00:57:50 2014 +1000

    Rename VCL's ImplInitGraphics to AcquireGraphics

    Turns out, we don't try to initialize a graphics context, much less
    *acquire* one. e.g. Window instances can have many frames (subwindows),
    of which one some are really being used at any time so we try to
    "steal" one of the graphics contexts from the frame to use ourself,
    later on that frame will steal it from someone else, etc.

    Change-Id: I66d5dbb7015301bc2d2be51627061c91e1f2ee5d

This has nothing to do with the const, but explains the clash - kind of.

Normally the bool variants will - at some point - call
  mpGraphics = ...->AcquireGraphics();

so much about the background stuff I was looking into.
Back to the original mail.

$ git grep "mpGraphics.*AcquireGraphics" vcl/
if ( mpGraphics || AcquireGraphics() )
if ( !mpGraphics && !AcquireGraphics() )
... 69 times

shows this idom is used some times in vcl. And IMHO it should be everywhere
reduced to:

if ( !AcquireGraphics() )
or
if ( AcquireGraphics() )

All the three bool variants start with:

$ git grep "bool.*::AcquireGraphics" vcl
vcl/source/gdi/print.cxx:bool Printer::AcquireGraphics() const
vcl/source/gdi/virdev.cxx:bool VirtualDevice::AcquireGraphics() const
vcl/source/window/window.cxx:bool Window::AcquireGraphics() const

bool *::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

The additional mutex check in debug builds is nice too.

For security I would rename the variants to Do / Impl and add an inline to the
OutputDevice header like so the "header" can't be stripped accidently:

inline bool OutputDevice::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

    return DoAcqureGraphics();
}

That also prevents the function call, if that really matters here.

Anyone want to make this an easy hack?
Revert the function name to ImplInitGraphics?

Comments?

Jan-Marek
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice