Re: [LyX/master] Fix crash when citeengine is unknown.

[Jürgen Spitzmüller]

Am Donnerstag, den 08.03.2018, 16:32 -0500 schrieb Richard Heck:
> Question that popped up while looking at this
> 
> Is it possible for more than one citeengine to be loaded by a
> document at one
> time? 

No.

> If so, how (other than by editing the LyX file itself)? Assuming it
> is not
> possible, then is it OK to change BufferParams::cite_engine_ to a
> std::string 
> (and simplify code elsewhere)?

Yes, sure. I don't know why I didn't do that from the beginning.
Possibly just an oversight when separating the cite engines from the
modules.

Jürgen

> 
> Richard
> 

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fix crash when citeengine is unknown.

[Richard Heck]

On 02/16/2018 12:24 PM, Jürgen Spitzmüller wrote:
> 2018-02-16 18:05 GMT+01:00 Richard Heck  >:
>
> On 02/14/2018 01:21 PM, Jürgen Spitzmüller wrote:
> > Am Mittwoch, den 14.02.2018, 12:50 -0500 schrieb Richard Heck:
> >> I wonder if what we really need to do here is add the 'unknown'
> cite
> >> engine the way we do unknown layouts. Otherwise, if, say, the
> font is
> >> modified in Document>Settings, then I am guessing we won't save the
> >> document with the same engine it had before: It isn't known there.
> >> That
> >> would mean we needed some other way to tell if the engine was
> 'real',
> >> rather than testing the pointer, but we do all of this with
> 'unknown'
> >> layouts.
> > Makes sense. Alas, I cannot look more closely now. Heading off to a
> > conference.
>
> I can have a look at this next week if you like. Good little
> project
>
>
> Yes, please.

Question that popped up while looking at this

Is it possible for more than one citeengine to be loaded by a document
at one
time? If so, how (other than by editing the LyX file itself)? Assuming
it is not
possible, then is it OK to change BufferParams::cite_engine_ to a
std::string
(and simplify code elsewhere)?

Richard

Re: [LyX/master] Fix crash when citeengine is unknown.

[Jürgen Spitzmüller]

2018-02-16 18:05 GMT+01:00 Richard Heck :

> On 02/14/2018 01:21 PM, Jürgen Spitzmüller wrote:
> > Am Mittwoch, den 14.02.2018, 12:50 -0500 schrieb Richard Heck:
> >> I wonder if what we really need to do here is add the 'unknown' cite
> >> engine the way we do unknown layouts. Otherwise, if, say, the font is
> >> modified in Document>Settings, then I am guessing we won't save the
> >> document with the same engine it had before: It isn't known there.
> >> That
> >> would mean we needed some other way to tell if the engine was 'real',
> >> rather than testing the pointer, but we do all of this with 'unknown'
> >> layouts.
> > Makes sense. Alas, I cannot look more closely now. Heading off to a
> > conference.
>
> I can have a look at this next week if you like. Good little project
>

Yes, please.

Jürgen


>
> Richard
>
>

Re: [LyX/master] Fix crash when citeengine is unknown.

[Richard Heck]

On 02/14/2018 01:21 PM, Jürgen Spitzmüller wrote:
> Am Mittwoch, den 14.02.2018, 12:50 -0500 schrieb Richard Heck:
>> I wonder if what we really need to do here is add the 'unknown' cite
>> engine the way we do unknown layouts. Otherwise, if, say, the font is
>> modified in Document>Settings, then I am guessing we won't save the
>> document with the same engine it had before: It isn't known there.
>> That
>> would mean we needed some other way to tell if the engine was 'real',
>> rather than testing the pointer, but we do all of this with 'unknown'
>> layouts.
> Makes sense. Alas, I cannot look more closely now. Heading off to a
> conference.

I can have a look at this next week if you like. Good little project

Richard

Re: [LyX/master] Fix crash when citeengine is unknown.

[Jürgen Spitzmüller]

Am Mittwoch, den 14.02.2018, 12:50 -0500 schrieb Richard Heck:
> I wonder if what we really need to do here is add the 'unknown' cite
> engine the way we do unknown layouts. Otherwise, if, say, the font is
> modified in Document>Settings, then I am guessing we won't save the
> document with the same engine it had before: It isn't known there.
> That
> would mean we needed some other way to tell if the engine was 'real',
> rather than testing the pointer, but we do all of this with 'unknown'
> layouts.

Makes sense. Alas, I cannot look more closely now. Heading off to a
conference.

Jürgen

> 
> Richard
> 

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fix crash when citeengine is unknown.

[Richard Heck]

On 02/14/2018 11:12 AM, Jürgen Spitzmüller wrote:
> Am Mittwoch, den 14.02.2018, 10:11 -0500 schrieb Richard Heck:
>>> I wonder how that happens. 
>> LyXCiteEngine const * CiteEnginesList::operator[](string const & str)
>> const
>> {
>> LyXCiteEnginesList::const_iterator it = englist_.begin();
>> for (; it != englist_.end(); ++it)
>> if (it->getID() == str) {
>> LyXCiteEngine const & eng = *it;
>> return 
>> }
>> return 0;
>> }
>>
>> The crucial point seems to be that we are in a const function, so we
>> return a null pointer. Boom.
> It didn't mean technically. That's clear. I mean how the empty string
> comes into the LyX file. Any sane LyX 2.3 document should have _some_
> cite engine set.

Oh, sorry. It's not empty in the LyX file. The value is coming from the
bibliography part of the GuiDocument:

    QString const engine =
        biblioModule->citeEngineCO->itemData(
                biblioModule->citeEngineCO->currentIndex()).toString();

This index was supposed to be set from the engine read from the LyX
file. But that engine doesn't exist, so it didn't get set properly.
Probably that needs to be taken into account.

>
>>
>>> Anyway, would be the following more safe (if
>>> we have an unknown non-empty string)?
>>>
>>>return theCiteEnginesList[fromqstr(engine)]
>>> && theCiteEnginesList[fromqstr(engine)]->getCiteFramework()
>>>== "biblatex";
>>>
>>> This should also cover the empty case.
>> Maybe what would be best is to create and use
>> CiteEnginesList::hasEngine(const & string). Then add an assertion to the 
>> function quoted above. This is how things are done in other places.
> Maybe, but for now, isn't a check whether the pointer is null better
> than checking just the string emptiness?

In light of what was said above, this is equivalent at this point in the
code. But there seem to be other places in GuiDocument that we rely upon
the pointer not to be null, and we need also to do something about
those. Also at BufferParams::useBiblatex().

I wonder if what we really need to do here is add the 'unknown' cite
engine the way we do unknown layouts. Otherwise, if, say, the font is
modified in Document>Settings, then I am guessing we won't save the
document with the same engine it had before: It isn't known there. That
would mean we needed some other way to tell if the engine was 'real',
rather than testing the pointer, but we do all of this with 'unknown'
layouts.

Richard

Re: [LyX/master] Fix crash when citeengine is unknown.

[Jürgen Spitzmüller]

Am Mittwoch, den 14.02.2018, 10:11 -0500 schrieb Richard Heck:
> > I wonder how that happens. 
> 
> LyXCiteEngine const * CiteEnginesList::operator[](string const & str)
> const
> {
> LyXCiteEnginesList::const_iterator it = englist_.begin();
> for (; it != englist_.end(); ++it)
> if (it->getID() == str) {
> LyXCiteEngine const & eng = *it;
> return 
> }
> return 0;
> }
> 
> The crucial point seems to be that we are in a const function, so we
> return a null pointer. Boom.

It didn't mean technically. That's clear. I mean how the empty string
comes into the LyX file. Any sane LyX 2.3 document should have _some_
cite engine set.

> 
> > Anyway, would be the following more safe (if
> > we have an unknown non-empty string)?
> > 
> >return theCiteEnginesList[fromqstr(engine)]
> > && theCiteEnginesList[fromqstr(engine)]->getCiteFramework()
> >== "biblatex";
> > 
> > This should also cover the empty case.
> 
> Maybe what would be best is to create and use
> CiteEnginesList::hasEngine(const & string). Then add an assertion to
> the
> function quoted above. This is how things are done in other places.

Maybe, but for now, isn't a check whether the pointer is null better
than checking just the string emptiness?

Jürgen

> 
> Richard
> 
> 

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fix crash when citeengine is unknown.

[Richard Heck]

On 02/14/2018 03:04 AM, Jürgen Spitzmüller wrote:
> Am Montag, den 12.02.2018, 16:31 -0500 schrieb Richard Heck:
>>>  src/frontends/qt4/GuiDocument.cpp |6 ++
>>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/frontends/qt4/GuiDocument.cpp
>>> b/src/frontends/qt4/GuiDocument.cpp
>>> index 85318bc..8149b50 100644
>>> --- a/src/frontends/qt4/GuiDocument.cpp
>>> +++ b/src/frontends/qt4/GuiDocument.cpp
>>> @@ -4007,6 +4007,12 @@ bool GuiDocument::isBiblatex() const
>>> biblioModule->citeEngineCO->itemData(
>>> biblioModule->citeEngineCO-
 currentIndex()).toString();
>>>  
>>> +   // this can happen if the cite engine is unknown, which
>>> can happen
>>> +   // if one is using a file that came from someone else,
>>> etc. in that
>>> +   // case, we crash if we proceed.
>>> +   if (engine.isEmpty())
>>> +   return false;
>>> +
>>> return theCiteEnginesList[fromqstr(engine)]-
 getCiteFramework() == "biblatex";
>>>  }
>>>  
> I wonder how that happens. 

LyXCiteEngine const * CiteEnginesList::operator[](string const & str) const
{
    LyXCiteEnginesList::const_iterator it = englist_.begin();
    for (; it != englist_.end(); ++it)
        if (it->getID() == str) {
            LyXCiteEngine const & eng = *it;
            return 
        }
    return 0;
}

The crucial point seems to be that we are in a const function, so we
return a null pointer. Boom.

> Anyway, would be the following more safe (if
> we have an unknown non-empty string)?
>
>return theCiteEnginesList[fromqstr(engine)]
>   && theCiteEnginesList[fromqstr(engine)]->getCiteFramework()
>  == "biblatex";
>
> This should also cover the empty case.

Maybe what would be best is to create and use
CiteEnginesList::hasEngine(const & string). Then add an assertion to the
function quoted above. This is how things are done in other places.

Richard

Re: [LyX/master] Fix crash when citeengine is unknown.

[Jürgen Spitzmüller]

Am Montag, den 12.02.2018, 16:31 -0500 schrieb Richard Heck:
> >  src/frontends/qt4/GuiDocument.cpp |6 ++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/frontends/qt4/GuiDocument.cpp
> > b/src/frontends/qt4/GuiDocument.cpp
> > index 85318bc..8149b50 100644
> > --- a/src/frontends/qt4/GuiDocument.cpp
> > +++ b/src/frontends/qt4/GuiDocument.cpp
> > @@ -4007,6 +4007,12 @@ bool GuiDocument::isBiblatex() const
> > biblioModule->citeEngineCO->itemData(
> > biblioModule->citeEngineCO-
> > >currentIndex()).toString();
> >  
> > +   // this can happen if the cite engine is unknown, which
> > can happen
> > +   // if one is using a file that came from someone else,
> > etc. in that
> > +   // case, we crash if we proceed.
> > +   if (engine.isEmpty())
> > +   return false;
> > +
> > return theCiteEnginesList[fromqstr(engine)]-
> > >getCiteFramework() == "biblatex";
> >  }
> >  

I wonder how that happens. Anyway, would be the following more safe (if
we have an unknown non-empty string)?

   return theCiteEnginesList[fromqstr(engine)]
&& theCiteEnginesList[fromqstr(engine)]->getCiteFramework()
   == "biblatex";

This should also cover the empty case.

Jürgen

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fix crash when citeengine is unknown.

[Scott Kostyshak]

On Mon, Feb 12, 2018 at 09:31:03PM +, Richard Heck wrote:
> On 02/12/2018 04:27 PM, Richard Heck wrote:
> > commit 5ee3396459602e0982234cab064c5c960af7e4fc
> > Author: Richard Heck 
> > Date:   Mon Feb 12 16:26:27 2018 -0500
> >
> > Fix crash when citeengine is unknown.
> 
> Scott, this is also needed in 2.3.x. Trivial and completely safe crash-fix.

Go ahead for 2.3.0. Thanks.

Scott


signature.asc
Description: PGP signature

Re: [LyX/master] Fix crash when citeengine is unknown.

[Richard Heck]

On 02/12/2018 04:27 PM, Richard Heck wrote:
> commit 5ee3396459602e0982234cab064c5c960af7e4fc
> Author: Richard Heck 
> Date:   Mon Feb 12 16:26:27 2018 -0500
>
> Fix crash when citeengine is unknown.

Scott, this is also needed in 2.3.x. Trivial and completely safe crash-fix.

Richard


> ---
>  src/frontends/qt4/GuiDocument.cpp |6 ++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/src/frontends/qt4/GuiDocument.cpp 
> b/src/frontends/qt4/GuiDocument.cpp
> index 85318bc..8149b50 100644
> --- a/src/frontends/qt4/GuiDocument.cpp
> +++ b/src/frontends/qt4/GuiDocument.cpp
> @@ -4007,6 +4007,12 @@ bool GuiDocument::isBiblatex() const
>   biblioModule->citeEngineCO->itemData(
>   
> biblioModule->citeEngineCO->currentIndex()).toString();
>  
> + // this can happen if the cite engine is unknown, which can happen
> + // if one is using a file that came from someone else, etc. in that
> + // case, we crash if we proceed.
> + if (engine.isEmpty())
> + return false;
> +
>   return theCiteEnginesList[fromqstr(engine)]->getCiteFramework() == 
> "biblatex";
>  }
>