RE: svn commit: r1786446 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c

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

RE: svn commit: r1786446 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c

Bert Huijben-5


> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: vrijdag 10 maart 2017 21:57
> To: [hidden email]
> Subject: svn commit: r1786446 -
> /subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>
> Author: stefan2
> Date: Fri Mar 10 20:56:59 2017
> New Revision: 1786446
>
> URL: http://svn.apache.org/viewvc?rev=1786446&view=rev
> Log:
> * subversion/libsvn_fs_fs/transaction.c
>   (get_shared_rep): Don't leak error objects.
>
> Found by: danielsh
>
> Modified:
>     subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tra
> nsaction.c?rev=1786446&r1=1786445&r2=1786446&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Fri Mar 10
> 20:56:59 2017
> @@ -2429,9 +2429,6 @@ get_shared_rep(representation_t **old_re
>        err = svn_stream_contents_same2(&same, contents, old_contents,
>                                        scratch_pool);
>
> -      /* Restore FILE's read / write position. */
> -      SVN_ERR(svn_io_file_seek(file, APR_SET, &old_position, scratch_pool));
> -
>        /* A mismatch should be extremely rare.
>         * If it does happen, log the event and don't share the rep. */
>        if (!same || err)
> @@ -2462,6 +2459,9 @@ get_shared_rep(representation_t **old_re
>            svn_error_clear(err);
>            *old_rep = NULL;
>          }
> +
> +      /* Restore FILE's read / write position. */
> +      SVN_ERR(svn_io_file_seek(file, APR_SET, &old_position, scratch_pool));

I'm not sure if that is a safe fix... Need more context for that.
You could just have used svn_error_compose_create() and combine both errors.

        Bert

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1786446 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c

Stefan Fuhrmann
On 20.03.2017 17:22, Bert Huijben wrote:

>
>> -----Original Message-----
>> From: [hidden email] [mailto:[hidden email]]
>> Sent: vrijdag 10 maart 2017 21:57
>> To: [hidden email]
>> Subject: svn commit: r1786446 -
>> /subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>>
>> Author: stefan2
>> Date: Fri Mar 10 20:56:59 2017
>> New Revision: 1786446
>>
>> URL: http://svn.apache.org/viewvc?rev=1786446&view=rev
>> Log:
>> * subversion/libsvn_fs_fs/transaction.c
>>    (get_shared_rep): Don't leak error objects.
>>
>> Found by: danielsh
>>
>> Modified:
>>      subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tra
>> nsaction.c?rev=1786446&r1=1786445&r2=1786446&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Fri Mar 10
>> 20:56:59 2017
>> @@ -2429,9 +2429,6 @@ get_shared_rep(representation_t **old_re
>>         err = svn_stream_contents_same2(&same, contents, old_contents,
>>                                         scratch_pool);
>>
>> -      /* Restore FILE's read / write position. */
>> -      SVN_ERR(svn_io_file_seek(file, APR_SET, &old_position, scratch_pool));
>> -
>>         /* A mismatch should be extremely rare.
>>          * If it does happen, log the event and don't share the rep. */
>>         if (!same || err)
>> @@ -2462,6 +2459,9 @@ get_shared_rep(representation_t **old_re
>>             svn_error_clear(err);
>>             *old_rep = NULL;
>>           }
>> +
>> +      /* Restore FILE's read / write position. */
>> +      SVN_ERR(svn_io_file_seek(file, APR_SET, &old_position, scratch_pool));
> I'm not sure if that is a safe fix... Need more context for that.
> You could just have used svn_error_compose_create() and combine both errors.
Thanks for the review!

I think the code is safe as it stands: err will be chained
by svn_error_createf and then gets cleaned up.

Is there something I'm missing?

-- Stefan^2.