Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Branko Čibej
On 30.10.2018 10:04, [hidden email] wrote:

> Author: rpluem
> Date: Tue Oct 30 09:04:14 2018
> New Revision: 1845204
>
> URL: http://svn.apache.org/viewvc?rev=1845204&view=rev
> Log:
> Fix issue SVN-4782: Do not use (const char*)1 in httpd modules as value for r->notes.
>
> mod_authz_svn.c and mod_dav_svn.c add keys to r->notes to memorize boolean
> states (FORCE_AUTHN_NOTE, IN_SOME_AUTHN_NOTE, authz_svn-anon-ok,
> NO_MAP_TO_STORAGE_NOTE). They use (const char*)1 as values for the keys. This
> causes any call to apr_table_clone for r->notes to crash with a SEGFAULT,
> because (const char*)1 is an invalid address. mod_http2 in httpd calls
> apr_table_clone for r->notes and hence the httpd process crashes.
> Hence replace the value of (const char*)1 in these cases with a value of "1".
>
> * subversion/mod_authz_svn/mod_authz_svn.c
>   (access_checker, check_user_id): Replace value of (const char*)1 with "1"
>    in apr_table_setn calls for r->notes table for keys FORCE_AUTHN_NOTE,
>    IN_SOME_AUTHN_NOTE, authz_svn-anon-ok to set a value with an valid address.
>
> * subversion/mod_authz_svn/mod_dav_svn.c
>   (dav_svn__translate_name): Replace value of (const char*)1 with "1"
>    in apr_table_setn calls for r->notes table for keys NO_MAP_TO_STORAGE_NOTE
>    to set a value with an valid address.


Hi Ruediger,

This looks perfect, thank you. It's important enough to be added to
CHANGES, in the server-side bugfixes section, so please add a line
there, for 1.12.0.

The backport process is similar to APR's and I assume httpd's, we use a
STATUS file for nominations with 3 PMC +1 required for core changes. We
have a script for proposing backports, here's an example:

[[[
.../repos/1.11.x$ ../trunk/tools/dist/nominate.pl r1845204 "Prevents a crash in mod_http2."
Index: STATUS
===================================================================
--- STATUS (revision 1845205)
+++ STATUS (working copy)
@@ -48,6 +48,14 @@ Candidate changes:
    Votes:
      +1: brane
 
+ * r1845204
+   Fix issue SVN-4782: Do not use (const char*)1 in httpd modules as value for
+   r->notes.
+   Justification:
+     Prevents a crash in mod_http2.
+   Votes:
+     +1: brane
+
 Veto-blocked changes:
 =====================
 
Commit this nomination?
]]]

(typing y<enter> will commit, anything else will revert the change to
STATUS).

Our currently maintained branches are 1.9.x, 1.10.x and 1.11.x; fixing a
crash is important enough to backport to all of them. Your vote won't be
binding since you're not a PMC member, but there's nothing wrong with
keeping it there.

Also, please always use the trunk version of the nominate.pl script.

-- Brane

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Daniel Shahaf-2
Branko Čibej wrote on Tue, 30 Oct 2018 10:22 +0100:
> It's important enough to be added to CHANGES, in the server-side
> bugfixes section, so please add a line there, for 1.12.0.

Since we'll have to add this revision four times to CHANGES (on for each
of 1.9, 1.10, 1.11, 1.12), perhaps we should start using the CHANGES-in-log-
message convention a bit more?  In this case, it would be something like
.
    [U:server] mod_dav_svn, mod_authz_svn: Fix segfault under mod_http2 (SVN-4782)
.
added to the the log message (as documented in tools/dist/release.py:write_changelog()).

(Rüdiger, this isn't something you could've known.  It so happens that
today we start, for the first time in our history, to support three
minor lines in parallel; that's why I'm bringing up streamlining the
CHANGES process.)

> The backport process is similar to APR's and I assume httpd's, we use a
> STATUS file for nominations with 3 PMC +1 required for core changes. We
> have a script for proposing backports, here's an example:

We also have a script for automatically merging backports that have received
the required number of votes, which might be of interest to apr/httpd's RM's.
(See commits by the role account 'svn-role' to our tree.)

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Ruediger Pluem
In reply to this post by Branko Čibej


On 2018/10/30 09:22:56, Branko Čibej <[hidden email]> wrote:

>
> [[[
> .../repos/1.11.x$ ../trunk/tools/dist/nominate.pl r1845204 "Prevents a crash in mod_http2."
> Index: STATUS
> ===================================================================
> --- STATUS (revision 1845205)
> +++ STATUS (working copy)
> @@ -48,6 +48,14 @@ Candidate changes:
>     Votes:
>       +1: brane
>  
> + * r1845204
> +   Fix issue SVN-4782: Do not use (const char*)1 in httpd modules as value for
> +   r->notes.
> +   Justification:
> +     Prevents a crash in mod_http2.
> +   Votes:
> +     +1: brane
> +
>  Veto-blocked changes:
>  =====================
>  
> Commit this nomination?
> ]]]
>
> (typing y<enter> will commit, anything else will revert the change to
> STATUS).
>
> Our currently maintained branches are 1.9.x, 1.10.x and 1.11.x; fixing a

Done for all 3 branches via the perl script as shown above
BTW, I had to apply the following patch to nominate.pl to get it running with perl 5.16.3 on CentOS 7:

Index: tools/dist/backport.pl
===================================================================
--- tools/dist/backport.pl      (revision 1845203)
+++ tools/dist/backport.pl      (working copy)
@@ -791,8 +791,8 @@
 
     # Add to state votes that aren't '+0' or 'edit'
     $state->{$_->{digest}}++ for grep
-                                 ($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
-                                 @votesarray;
+                                 (($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
+                                 @votesarray);
   }
 }
 
I am happy to commit this if this is fine with later versions of perl which I have not at hand for testing.

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Ruediger Pluem
In reply to this post by Daniel Shahaf-2


On 2018/10/30 18:09:42, Daniel Shahaf <[hidden email]> wrote:

> Branko Čibej wrote on Tue, 30 Oct 2018 10:22 +0100:
> > It's important enough to be added to CHANGES, in the server-side
> > bugfixes section, so please add a line there, for 1.12.0.
>
> Since we'll have to add this revision four times to CHANGES (on for each
> of 1.9, 1.10, 1.11, 1.12), perhaps we should start using the CHANGES-in-log-
> message convention a bit more?  In this case, it would be something like
> .
>     [U:server] mod_dav_svn, mod_authz_svn: Fix segfault under mod_http2 (SVN-4782)
> .

Should I add the above to the commit log? Or should I add an entry to CHANGES directly?
I can do either way whatever you prefer.

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Daniel Shahaf-2
Ruediger Pluem wrote on Tue, Oct 30, 2018 at 20:00:01 -0000:

>
>
> On 2018/10/30 18:09:42, Daniel Shahaf <[hidden email]> wrote:
> > Branko Čibej wrote on Tue, 30 Oct 2018 10:22 +0100:
> > > It's important enough to be added to CHANGES, in the server-side
> > > bugfixes section, so please add a line there, for 1.12.0.
> >
> > Since we'll have to add this revision four times to CHANGES (on for each
> > of 1.9, 1.10, 1.11, 1.12), perhaps we should start using the CHANGES-in-log-
> > message convention a bit more?  In this case, it would be something like
> > .
> >     [U:server] mod_dav_svn, mod_authz_svn: Fix segfault under mod_http2 (SVN-4782)
> > .
>
> Should I add the above to the commit log? Or should I add an entry to CHANGES directly?
> I can do either way whatever you prefer.

In the log message please; release.py write-changelog will pick it from there
when we roll 1.12.0-alpha1.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Daniel Shahaf-2
In reply to this post by Ruediger Pluem
Ruediger Pluem wrote on Tue, Oct 30, 2018 at 19:57:58 -0000:

> BTW, I had to apply the following patch to nominate.pl to get it running with perl 5.16.3 on CentOS 7:
>
> Index: tools/dist/backport.pl
> ===================================================================
> --- tools/dist/backport.pl      (revision 1845203)
> +++ tools/dist/backport.pl      (working copy)
> @@ -791,8 +791,8 @@
>  
>      # Add to state votes that aren't '+0' or 'edit'
>      $state->{$_->{digest}}++ for grep
> -                                 ($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
> -                                 @votesarray;
> +                                 (($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
> +                                 @votesarray);
>    }
>  }
>  
> I am happy to commit this if this is fine with later versions of perl which I have not at hand for testing.

Works for me with and without the change, Perl 5.24.1 on Debian stretch.
+1 to commit.

Consider using «grep +(foo), @bar» or «grep { foo } @bar» instead of
«grep ((foo), bar»: the three alternatives are equivalent, so it's just
a question of which one is more readable/idiomatic.

Cheers,

Daniel
(Feel free to join us on IRC, by the way.  #svn-dev on freenode)
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Ruediger Pluem
In reply to this post by Daniel Shahaf-2


On 2018/10/30 21:03:53, Daniel Shahaf <[hidden email]> wrote:
> Ruediger Pluem wrote on Tue, Oct 30, 2018 at 20:00:01 -0000:
> >
> >
> > On 2018/10/30 18:09:42, Daniel Shahaf <[hidden email]> wrote:
> > > Branko Čibej wrote on Tue, 30 Oct 2018 10:22 +0100:

> >
> > Should I add the above to the commit log? Or should I add an entry to CHANGES directly?
> > I can do either way whatever you prefer.
>
> In the log message please; release.py write-changelog will pick it from there
> when we roll 1.12.0-alpha1.
>

Adjusted svn:log property of r1845204.

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1845204 - in /subversion/trunk/subversion: mod_authz_svn/mod_authz_svn.c mod_dav_svn/mod_dav_svn.c

Ruediger Pluem
In reply to this post by Daniel Shahaf-2


On 2018/10/30 21:11:57, Daniel Shahaf <[hidden email]> wrote:

> Ruediger Pluem wrote on Tue, Oct 30, 2018 at 19:57:58 -0000:
> > BTW, I had to apply the following patch to nominate.pl to get it running with perl 5.16.3 on CentOS 7:
> >
> > Index: tools/dist/backport.pl
> > ===================================================================
> > --- tools/dist/backport.pl      (revision 1845203)
> > +++ tools/dist/backport.pl      (working copy)
> > @@ -791,8 +791,8 @@
> >  
> >      # Add to state votes that aren't '+0' or 'edit'
> >      $state->{$_->{digest}}++ for grep
> > -                                 ($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
> > -                                 @votesarray;
> > +                                 (($_->{approval} or $_->{vote} =~ /^(-1|-0|[+]1)$/),
> > +                                 @votesarray);
> >    }
> >  }
> >  
> > I am happy to commit this if this is fine with later versions of perl which I have not at hand for testing.
>
> Works for me with and without the change, Perl 5.24.1 on Debian stretch.
> +1 to commit.
>
> Consider using «grep +(foo), @bar» or «grep { foo } @bar» instead of
> «grep ((foo), bar»: the three alternatives are equivalent, so it's just
> a question of which one is more readable/idiomatic.

Used the grep +(foo), @bar syntax and committed in r1845312

Regards

Rüdiger