You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2016/11/06 01:21:36 UTC
ra_svn vwrite_tuple() optional elements robustness
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
Re: ra_svn vwrite_tuple() optional elements robustness
Posted by Stefan Fuhrmann <st...@apache.org>.
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.
Re: ra_svn vwrite_tuple() optional elements robustness
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: ra_svn vwrite_tuple() optional elements robustness
Posted by Stefan Fuhrmann <st...@apache.org>.
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.
Re: ra_svn vwrite_tuple() optional elements robustness
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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)
Re: ra_svn vwrite_tuple() optional elements robustness
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
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.