ra_svn vwrite_tuple() optional elements robustness

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

ra_svn vwrite_tuple() optional elements robustness

Daniel Shahaf-2
During the r1767197 thread, I noticed that vwrite_tuple() doesn't
enforce its precondition that in the optional part of a tuple, either
all values are valid or no value is; a call such as

    vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)

would happily generate a «( 42 ) » tuple, instead of triggering an
SVN_ERR_MALFUNCTION().

This would be easy to fix, and would prevent a class of bugs.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: ra_svn vwrite_tuple() optional elements robustness

Stefan Fuhrmann-3
On 06.11.2016 02:21, Daniel Shahaf wrote:

> During the r1767197 thread, I noticed that vwrite_tuple() doesn't
> enforce its precondition that in the optional part of a tuple, either
> all values are valid or no value is; a call such as
>
>      vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)
>
> would happily generate a «( 42 ) » tuple, instead of triggering an
> SVN_ERR_MALFUNCTION().
>
> This would be easy to fix, and would prevent a class of bugs.
This function seems to have a number of bugs,
for instance:

* optional sub-structs don't get a "("
* the OPT flag is global instead of per-struct

If there is an efficient way to fix these, please
feel free to do so ;)

-- Stefan^2.
Reply | Threaded
Open this post in threaded view
|

Re: ra_svn vwrite_tuple() optional elements robustness

Daniel Shahaf-2
Stefan Fuhrmann wrote on Mon, Nov 14, 2016 at 12:50:51 +0100:

> On 06.11.2016 02:21, Daniel Shahaf wrote:
> >During the r1767197 thread, I noticed that vwrite_tuple() doesn't
> >enforce its precondition that in the optional part of a tuple, either
> >all values are valid or no value is; a call such as
> >
> >     vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)
> >
> >would happily generate a «( 42 ) » tuple, instead of triggering an
> >SVN_ERR_MALFUNCTION().
> >
> >This would be easy to fix, and would prevent a class of bugs.
> This function seems to have a number of bugs,
> for instance:
>
> * optional sub-structs don't get a "("
> * the OPT flag is global instead of per-struct

I hope at least the parser routines don't have such bugs.

(excuse brevity; can't be verbose at the moment)
Reply | Threaded
Open this post in threaded view
|

Re: ra_svn vwrite_tuple() optional elements robustness

Stefan Fuhrmann
On 15.11.2016 00:43, Daniel Shahaf wrote:

> Stefan Fuhrmann wrote on Mon, Nov 14, 2016 at 12:50:51 +0100:
>> On 06.11.2016 02:21, Daniel Shahaf wrote:
>>> During the r1767197 thread, I noticed that vwrite_tuple() doesn't
>>> enforce its precondition that in the optional part of a tuple, either
>>> all values are valid or no value is; a call such as
>>>
>>>      vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)
>>>
>>> would happily generate a «( 42 ) » tuple, instead of triggering an
>>> SVN_ERR_MALFUNCTION().
>>>
>>> This would be easy to fix, and would prevent a class of bugs.
>> This function seems to have a number of bugs,
>> for instance:
>>
>> * optional sub-structs don't get a "("
>> * the OPT flag is global instead of per-struct
>
> I hope at least the parser routines don't have such bugs.
>
> (excuse brevity; can't be verbose at the moment)

As it turns out, the current implementation is correct but
forbids optional sub-tuples.  See r1795714.

-- Stefan^2.
Reply | Threaded
Open this post in threaded view
|

Re: ra_svn vwrite_tuple() optional elements robustness

Daniel Shahaf-2
Stefan Fuhrmann wrote on Sun, 21 May 2017 20:09 +0200:

> On 15.11.2016 00:43, Daniel Shahaf wrote:
> > Stefan Fuhrmann wrote on Mon, Nov 14, 2016 at 12:50:51 +0100:
> >> On 06.11.2016 02:21, Daniel Shahaf wrote:
> >>> During the r1767197 thread, I noticed that vwrite_tuple() doesn't
> >>> enforce its precondition that in the optional part of a tuple, either
> >>> all values are valid or no value is; a call such as
> >>>
> >>>      vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)
> >>>
> >>> would happily generate a «( 42 ) » tuple, instead of triggering an
> >>> SVN_ERR_MALFUNCTION().
> >>>
> >>> This would be easy to fix, and would prevent a class of bugs.
> >> This function seems to have a number of bugs,
> >> for instance:
> >>
> >> * optional sub-structs don't get a "("
> >> * the OPT flag is global instead of per-struct
> >
> > I hope at least the parser routines don't have such bugs.
> >
> > (excuse brevity; can't be verbose at the moment)
>
> As it turns out, the current implementation is correct but
> forbids optional sub-tuples.  See r1795714.

r1795714 broke buildbot.  I think that's because it caused the '(' case
to advance ap, whereas before that revision it didn't.  (Thta change
wasn't mentioned in the log message, by the way.)

Thanks for getting to the bottom of this.  I assume the other cases discussed
in this thread are also fine...

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: ra_svn vwrite_tuple() optional elements robustness

Stefan Fuhrmann


On 21.05.2017 20:28, Daniel Shahaf wrote:

> Stefan Fuhrmann wrote on Sun, 21 May 2017 20:09 +0200:
>> On 15.11.2016 00:43, Daniel Shahaf wrote:
>>> Stefan Fuhrmann wrote on Mon, Nov 14, 2016 at 12:50:51 +0100:
>>>> On 06.11.2016 02:21, Daniel Shahaf wrote:
>>>>> During the r1767197 thread, I noticed that vwrite_tuple() doesn't
>>>>> enforce its precondition that in the optional part of a tuple, either
>>>>> all values are valid or no value is; a call such as
>>>>>
>>>>>       vwrite_tuple("(?r?r)", SVN_INVALID_REVNUM, (svn_revnum_t) 42)
>>>>>
>>>>> would happily generate a «( 42 ) » tuple, instead of triggering an
>>>>> SVN_ERR_MALFUNCTION().
>>>>>
>>>>> This would be easy to fix, and would prevent a class of bugs.
>>>> This function seems to have a number of bugs,
>>>> for instance:
>>>>
>>>> * optional sub-structs don't get a "("
>>>> * the OPT flag is global instead of per-struct
>>>
>>> I hope at least the parser routines don't have such bugs.
>>>
>>> (excuse brevity; can't be verbose at the moment)
>>
>> As it turns out, the current implementation is correct but
>> forbids optional sub-tuples.  See r1795714.
>
> r1795714 broke buildbot.  I think that's because it caused the '(' case
> to advance ap, whereas before that revision it didn't.  (Thta change
> wasn't mentioned in the log message, by the way.)

Oops - sorry for that!

I must have somehow thick-fingered a copy'n'pasto there ...
r1795718 should repair this.

-- Stefan^2.