Re: [PATCH] Fix for issue 3813
Noorul Islam K M wrote on Thu, Jun 23, 2011 at 15:25:37 +0530: > "Bert Huijben" writes: > > >> -Original Message- > >> From: Noorul Islam K M [mailto:noo...@collab.net] > >> Sent: donderdag 23 juni 2011 5:44 > >> To: Daniel Shahaf > >> Cc: Julian Foad; Subversion; Bert Huijben > >> Subject: Re: [PATCH] Fix for issue 3813 > >> > >> Daniel Shahaf writes: > >> > >> > Looks like Bert committed a functionally equivalent fix as part of > > r1138474. > >> > > >> > >> Bert, > >> > >> May I know why this patch was not considered? Looking at the commit I > >> could not see any difference. > > > > Applying a one line patch takes me 5 times more time than just editing the > > code. And as the final whitespace (and comment) didn't match your original > > patch, adding a patch by wasn't really appropriate. > > > > I thought if the patch was incorrect, I might be getting some feedback. > In this case your feedback is the interdiff between what you sent and what Bert applied. (Before I became a committer it was my habit to always study the interdiff, in addition to whatever comments I might have received on dev@.) > > Discussion on irc showed that we couldn't just apply the patch without the > > additional work from Stefan as that would open a window where somebody could > > look at otherwise hidden files. (I assumed he would apply your patch after > > that, but later I just applied the change anyway) > > > > I just added a 'found by' to the log message of r1138474. > > > > It was actually found by Daniel. > I have to agree with Noorul here; I've adjusted the log message to 'Patch by'. > Thanks and Regards > Noorul
Re: [PATCH] Fix for issue 3813
Stefan Sperling writes: > On Thu, Jun 23, 2011 at 03:25:37PM +0530, Noorul Islam K M wrote: > >> I thought if the patch was incorrect, I might be getting some feedback. > > Sorry, sometimes things just fall through the cracks :( > > Please don't take this as something personal. Nothing personal at all. > Communications can be delayed or simply get lost since we're working > in a distributed team in very different timezones, rather than in the > same office where it's usually enough to get up and talk to the person > at another table in the same room. > > It's not easy to learn how to effectively communicate and get attention > via email, but that's what we all have to do. And we also have to learn > to tolerate occasional miscommunication. > >> > Discussion on irc showed that we couldn't just apply the patch without the >> > additional work from Stefan as that would open a window where somebody >> > could >> > look at otherwise hidden files. (I assumed he would apply your patch after >> > that, but later I just applied the change anyway) > > I wasn't aware of the patch proposed in this thread and how it > related to the change I made. I replied to one of Daniel's questions I > saw in this thread, and doing so inspired me to take a look at the code > myself, but that was it. > > Maybe having more than just an issue number in the subject would help > prevent this sort of thing? Our focus is usually on problems, not numbers. > So the subject could have hinted at what problem was being solved. > The numbers don't carry any information on their own in email. > They only make sense if people are browsing the issue tracker, find that > particular issue, and then go to the mailing list to see if something > was written about this issue (and that is a good thing, and it's why > we try to include the numbers if possible). But they don't help in other > cases, e.g. someone just catching up with dev@ email. Unless people are > bothered enough to look them up in the web interface (which I'm often not). Actually I improved the log message to include more information about the patch after Julian requested so. It is my mistake for not including description in subject line. After all these days in the list I should have learned it. Sorry about that. Thanks and Regards Noorul
Re: [PATCH] Fix for issue 3813
On Thu, Jun 23, 2011 at 03:25:37PM +0530, Noorul Islam K M wrote: > I thought if the patch was incorrect, I might be getting some feedback. Sorry, sometimes things just fall through the cracks :( Please don't take this as something personal. Communications can be delayed or simply get lost since we're working in a distributed team in very different timezones, rather than in the same office where it's usually enough to get up and talk to the person at another table in the same room. It's not easy to learn how to effectively communicate and get attention via email, but that's what we all have to do. And we also have to learn to tolerate occasional miscommunication. > > Discussion on irc showed that we couldn't just apply the patch without the > > additional work from Stefan as that would open a window where somebody could > > look at otherwise hidden files. (I assumed he would apply your patch after > > that, but later I just applied the change anyway) I wasn't aware of the patch proposed in this thread and how it related to the change I made. I replied to one of Daniel's questions I saw in this thread, and doing so inspired me to take a look at the code myself, but that was it. Maybe having more than just an issue number in the subject would help prevent this sort of thing? Our focus is usually on problems, not numbers. So the subject could have hinted at what problem was being solved. The numbers don't carry any information on their own in email. They only make sense if people are browsing the issue tracker, find that particular issue, and then go to the mailing list to see if something was written about this issue (and that is a good thing, and it's why we try to include the numbers if possible). But they don't help in other cases, e.g. someone just catching up with dev@ email. Unless people are bothered enough to look them up in the web interface (which I'm often not).
Re: [PATCH] Fix for issue 3813
"Bert Huijben" writes: >> -Original Message- >> From: Noorul Islam K M [mailto:noo...@collab.net] >> Sent: donderdag 23 juni 2011 5:44 >> To: Daniel Shahaf >> Cc: Julian Foad; Subversion; Bert Huijben >> Subject: Re: [PATCH] Fix for issue 3813 >> >> Daniel Shahaf writes: >> >> > Looks like Bert committed a functionally equivalent fix as part of > r1138474. >> > >> >> Bert, >> >> May I know why this patch was not considered? Looking at the commit I >> could not see any difference. > > Applying a one line patch takes me 5 times more time than just editing the > code. And as the final whitespace (and comment) didn't match your original > patch, adding a patch by wasn't really appropriate. > I thought if the patch was incorrect, I might be getting some feedback. > Discussion on irc showed that we couldn't just apply the patch without the > additional work from Stefan as that would open a window where somebody could > look at otherwise hidden files. (I assumed he would apply your patch after > that, but later I just applied the change anyway) > > I just added a 'found by' to the log message of r1138474. > It was actually found by Daniel. Thanks and Regards Noorul
RE: [PATCH] Fix for issue 3813
> -Original Message- > From: Noorul Islam K M [mailto:noo...@collab.net] > Sent: donderdag 23 juni 2011 5:44 > To: Daniel Shahaf > Cc: Julian Foad; Subversion; Bert Huijben > Subject: Re: [PATCH] Fix for issue 3813 > > Daniel Shahaf writes: > > > Looks like Bert committed a functionally equivalent fix as part of r1138474. > > > > Bert, > > May I know why this patch was not considered? Looking at the commit I > could not see any difference. Applying a one line patch takes me 5 times more time than just editing the code. And as the final whitespace (and comment) didn't match your original patch, adding a patch by wasn't really appropriate. Discussion on irc showed that we couldn't just apply the patch without the additional work from Stefan as that would open a window where somebody could look at otherwise hidden files. (I assumed he would apply your patch after that, but later I just applied the change anyway) I just added a 'found by' to the log message of r1138474. Bert
Re: [PATCH] Fix for issue 3813
Daniel Shahaf writes: > Looks like Bert committed a functionally equivalent fix as part of r1138474. > Bert, May I know why this patch was not considered? Looking at the commit I could not see any difference. Thanks and Regards Noorul > Noorul Islam K M wrote on Wed, Jun 22, 2011 at 16:40:50 +0530: >> +++ subversion/libsvn_wc/diff_editor.c (working copy) >> @@ -1532,13 +1531,8 @@ >> >> >> >> - /* This is the file that will contain the pristine repository version. It >> - is created in the admin temporary area. This file continues to exists >> - until after the diff callback is run, at which point it is deleted. */ >> - SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir, eb->db, >> fb->local_abspath, >> - pool, pool)); >>SVN_ERR(svn_stream_open_unique(&temp_stream, &fb->temp_file_path, >> - temp_dir, svn_io_file_del_on_pool_cleanup, >> + NULL, svn_io_file_del_on_pool_cleanup, >> fb->pool, fb->pool)); >> >>whb = apr_pcalloc(fb->pool, sizeof(*whb));
Re: [PATCH] Fix for issue 3813
Looks like Bert committed a functionally equivalent fix as part of r1138474. Noorul Islam K M wrote on Wed, Jun 22, 2011 at 16:40:50 +0530: > +++ subversion/libsvn_wc/diff_editor.c(working copy) > @@ -1532,13 +1531,8 @@ > > > > - /* This is the file that will contain the pristine repository version. It > - is created in the admin temporary area. This file continues to exists > - until after the diff callback is run, at which point it is deleted. */ > - SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir, eb->db, > fb->local_abspath, > - pool, pool)); >SVN_ERR(svn_stream_open_unique(&temp_stream, &fb->temp_file_path, > - temp_dir, svn_io_file_del_on_pool_cleanup, > + NULL, svn_io_file_del_on_pool_cleanup, > fb->pool, fb->pool)); > >whb = apr_pcalloc(fb->pool, sizeof(*whb));
Re: [PATCH] Fix for issue 3813
Greg Hudson wrote on Wed, Jun 22, 2011 at 11:01:05 -0400: > On Wed, 2011-06-22 at 02:29 -0400, Daniel Shahaf wrote: > > From looking at the code, svn_io_open_unique_file3() would force the > > file to have a mode of 0600|umask() instead of just 0600 > > The umask removes file permissions from the mode argument to open(); it > doesn't add permissions. (Unless there's something unusual about this > code.) The comments in the code are of the form "This is a hack to make the file created with mode 0644 rather than 0600 as it was without/before this hack". My '0600|umask()' syntax is inaccurate. (It's 0600|foo(), but foo() != umask().)
Re: [PATCH] Fix for issue 3813
On Wed, 2011-06-22 at 02:29 -0400, Daniel Shahaf wrote: > From looking at the code, svn_io_open_unique_file3() would force the > file to have a mode of 0600|umask() instead of just 0600 The umask removes file permissions from the mode argument to open(); it doesn't add permissions. (Unless there's something unusual about this code.)
Re: [PATCH] Fix for issue 3813
Julian Foad writes: > Noorul Islam K M wrote: > >> Subject: Re: [PATCH] Fix for issue 3813 > [...] >> [[[ >> Fix for issue 3813. >> [...] >> ]]] > > Hi Noorul. Please always include a brief description of the issue in > the email subject line and in the log message. Anyone reading the text > should be able to quickly know what it is talking about without having > to fire up the issue tracker. > Please find the updated log message. Also attaching the patch again. Log [[[ Fix for issue 3813. Make 'svn diff' to use system temporary area instead working copy admin area. Admin temporary area could be write protected which makes 'svn diff' to fail in certain cases. * subversion/libsvn_wc/diff_editor.c (apply_textdelta): Use the default temporary directory instead of admin temporary area to create temporary file. Patch by: Noorul Islam K M ]]] Thanks and Regards Noorul Index: subversion/libsvn_wc/diff_editor.c === --- subversion/libsvn_wc/diff_editor.c (revision 1137953) +++ subversion/libsvn_wc/diff_editor.c (working copy) @@ -1518,7 +1518,6 @@ struct file_baton *fb = file_baton; struct window_handler_baton *whb; struct edit_baton *eb = fb->eb; - const char *temp_dir; svn_stream_t *source; svn_stream_t *temp_stream; @@ -1532,13 +1531,8 @@ - /* This is the file that will contain the pristine repository version. It - is created in the admin temporary area. This file continues to exists - until after the diff callback is run, at which point it is deleted. */ - SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir, eb->db, fb->local_abspath, - pool, pool)); SVN_ERR(svn_stream_open_unique(&temp_stream, &fb->temp_file_path, - temp_dir, svn_io_file_del_on_pool_cleanup, + NULL, svn_io_file_del_on_pool_cleanup, fb->pool, fb->pool)); whb = apr_pcalloc(fb->pool, sizeof(*whb));
Re: [PATCH] Fix for issue 3813
Noorul Islam K M wrote: > Subject: Re: [PATCH] Fix for issue 3813 [...] > [[[ > Fix for issue 3813. > [...] > ]]] Hi Noorul. Please always include a brief description of the issue in the email subject line and in the log message. Anyone reading the text should be able to quickly know what it is talking about without having to fire up the issue tracker. Thanks. - Julian
Re: [PATCH] Fix for issue 3813
On Wed, Jun 22, 2011 at 09:29:29AM +0300, Daniel Shahaf wrote: > Noorul Islam K M wrote on Wed, Jun 22, 2011 at 11:37:13 +0530: > > > > I am not really sure whether this patch is correct one since the comment > > in diff_editor.c says this > > > > /* This is the file that will contain the pristine repository version. It > > is created in the admin temporary area. This file continues to exists > > until after the diff callback is run, at which point it is deleted. */ > > > > I can't think of the drawbacks of creating this particular temp file in > > /tmp. > > > > >From looking at the code, svn_io_open_unique_file3() would force the > file to have a mode of 0600|umask() instead of just 0600; i.e., the > contents of the files being diffed would leak, even if the wc root dir > was mode 0700 (in which case they don't currently leak). > > Is that a problem? It would be better to keep the perms of the temporary file at 600 indeed. > (The comment in svn_io_open_unique_file3() indicates that there's some > historical reason, catering to specific API consumers, for extending the > permissions of the tempfile.) The problem was that some tempfiles with mode 600 would be installed from the temp area (wherever it is) into the working copy. So working files suddenly had 600 perms, whereas before they had whatever the umask said. This did cause regression test failures. I punted on fixing the callers at that point (way back at the subconf 2009 hackathon in Munich) and made the tempfile function take care of it. This should be revisited to fix the tempfile leakage problem you describe (which some people would consider a security bug...)
Re: [PATCH] Fix for issue 3813
Noorul Islam K M wrote on Wed, Jun 22, 2011 at 11:37:13 +0530: > > I am not really sure whether this patch is correct one since the comment > in diff_editor.c says this > > /* This is the file that will contain the pristine repository version. It > is created in the admin temporary area. This file continues to exists > until after the diff callback is run, at which point it is deleted. */ > > I can't think of the drawbacks of creating this particular temp file in > /tmp. > >From looking at the code, svn_io_open_unique_file3() would force the file to have a mode of 0600|umask() instead of just 0600; i.e., the contents of the files being diffed would leak, even if the wc root dir was mode 0700 (in which case they don't currently leak). Is that a problem? (The comment in svn_io_open_unique_file3() indicates that there's some historical reason, catering to specific API consumers, for extending the permissions of the tempfile.) > Log > > [[[ > Fix for issue 3813. > > * subversion/libsvn_wc/diff_editor.c > (apply_textdelta): Use the default temporary directory instead of > admin temporary area to create temporary file. > > Patch by: Noorul Islam K M > ]]] > > Thanks and Regards > Noorul > > Index: subversion/libsvn_wc/diff_editor.c > === > --- subversion/libsvn_wc/diff_editor.c(revision 1137953) > +++ subversion/libsvn_wc/diff_editor.c(working copy) > @@ -1518,7 +1518,6 @@ >struct file_baton *fb = file_baton; >struct window_handler_baton *whb; >struct edit_baton *eb = fb->eb; > - const char *temp_dir; >svn_stream_t *source; >svn_stream_t *temp_stream; > > @@ -1532,13 +1531,8 @@ > > > > - /* This is the file that will contain the pristine repository version. It > - is created in the admin temporary area. This file continues to exists > - until after the diff callback is run, at which point it is deleted. */ > - SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir, eb->db, > fb->local_abspath, > - pool, pool)); >SVN_ERR(svn_stream_open_unique(&temp_stream, &fb->temp_file_path, > - temp_dir, svn_io_file_del_on_pool_cleanup, > + NULL, svn_io_file_del_on_pool_cleanup, > fb->pool, fb->pool)); > >whb = apr_pcalloc(fb->pool, sizeof(*whb));
[PATCH] Fix for issue 3813
I am not really sure whether this patch is correct one since the comment in diff_editor.c says this /* This is the file that will contain the pristine repository version. It is created in the admin temporary area. This file continues to exists until after the diff callback is run, at which point it is deleted. */ I can't think of the drawbacks of creating this particular temp file in /tmp. Log [[[ Fix for issue 3813. * subversion/libsvn_wc/diff_editor.c (apply_textdelta): Use the default temporary directory instead of admin temporary area to create temporary file. Patch by: Noorul Islam K M ]]] Thanks and Regards Noorul Index: subversion/libsvn_wc/diff_editor.c === --- subversion/libsvn_wc/diff_editor.c (revision 1137953) +++ subversion/libsvn_wc/diff_editor.c (working copy) @@ -1518,7 +1518,6 @@ struct file_baton *fb = file_baton; struct window_handler_baton *whb; struct edit_baton *eb = fb->eb; - const char *temp_dir; svn_stream_t *source; svn_stream_t *temp_stream; @@ -1532,13 +1531,8 @@ - /* This is the file that will contain the pristine repository version. It - is created in the admin temporary area. This file continues to exists - until after the diff callback is run, at which point it is deleted. */ - SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir, eb->db, fb->local_abspath, - pool, pool)); SVN_ERR(svn_stream_open_unique(&temp_stream, &fb->temp_file_path, - temp_dir, svn_io_file_del_on_pool_cleanup, + NULL, svn_io_file_del_on_pool_cleanup, fb->pool, fb->pool)); whb = apr_pcalloc(fb->pool, sizeof(*whb));
