You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2009/12/21 15:50:37 UTC
Re: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Wed, Nov 25, 2009 at 09:15:03PM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
>
> > On Wed, Nov 25, 2009 at 03:51:43PM -0500, Mark Phippard wrote:
> >> On Wed, Nov 25, 2009 at 3:27 PM, <st...@apache.org> wrote:
> >> > Author: stsp
> >> > Date: Wed Nov 25 20:27:38 2009
> >> > New Revision: 884250
> >> >
> >> > URL: http://svn.apache.org/viewvc?rev=884250&view=rev
> >> > Log:
> >> > Replace use of strcpy() in libsvn_fs_fs, my compiler keeps complaining.
> >> >
> >> > * subversion/libsvn_fs_fs/fs_fs.c
> >> > (get_shared_txn): Use strncpy() instead of strcpy(). Make sure to
> >> > NUL-terminate the string in all cases.
> >>
> >> Could this use apr_cpystrn?
> >>
> >> http://apr.apache.org/docs/apr/0.9/group__apr__strings.html#g69700a825e82dd646f9f166599040431
> >
> > Oh yes, thanks, that sounds useful.
> >
> > Looks like I should be making a habit of reading APR header files more
> > often :)
>
> Also strncpy does null-padding so the calls using MAX_KEY_SIZE copy
> 200 bytes every time; that's more than a typical CPU cache line.
Is this diff OK?
"make check" likes it but since I don't usually hack fsfs code
I'd like another pair of eyes on this.
Stefan
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 892649)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -34,6 +34,7 @@
#include <apr_lib.h>
#include <apr_md5.h>
#include <apr_sha1.h>
+#include <apr_strings.h>
#include <apr_thread_mutex.h>
#include "svn_pools.h"
@@ -423,24 +424,29 @@ path_node_origin(svn_fs_t *fs, const char *node_id
/* Functions for working with shared transaction data. */
-/* Return the transaction object for transaction TXN_ID from the
- transaction list of filesystem FS (which must already be locked via the
+/* Return in *SHARED_TXN the transaction object for transaction TXN_ID from
+ the transaction list of filesystem FS (which must already be locked via the
txn_list_lock mutex). If the transaction does not exist in the list,
- then create a new transaction object and return it (if CREATE_NEW is
- true) or return NULL (otherwise). */
-static fs_fs_shared_txn_data_t *
-get_shared_txn(svn_fs_t *fs, const char *txn_id, svn_boolean_t create_new)
+ then create a new transaction object and return it in *SHARED_TXN (if
+ CREATE_NEW is true) or set *SHARED_TXN to NULL (otherwise). */
+static svn_error_t *
+get_shared_txn(fs_fs_shared_txn_data_t **shared_txn, svn_fs_t *fs,
+ const char *txn_id, svn_boolean_t create_new)
{
fs_fs_data_t *ffd = fs->fsap_data;
fs_fs_shared_data_t *ffsd = ffd->shared;
fs_fs_shared_txn_data_t *txn;
+ const char *end;
for (txn = ffsd->txns; txn; txn = txn->next)
if (strcmp(txn->txn_id, txn_id) == 0)
break;
if (txn || !create_new)
- return txn;
+ {
+ *shared_txn = txn;
+ return SVN_NO_ERROR;
+ }
/* Use the transaction object from the (single-object) freelist,
if one is available, or otherwise create a new object. */
@@ -456,9 +462,10 @@ path_node_origin(svn_fs_t *fs, const char *node_id
txn->pool = subpool;
}
- assert(strlen(txn_id) < sizeof(txn->txn_id));
- strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
- txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
+ end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
+ if (end < txn->txn_id + strlen(txn_id))
+ return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG, NULL,
+ _("Transaction name '%s' was truncated"), txn_id);
txn->being_written = FALSE;
/* Link this transaction into the head of the list. We will typically
@@ -468,7 +475,8 @@ path_node_origin(svn_fs_t *fs, const char *node_id
txn->next = ffsd->txns;
ffsd->txns = txn;
- return txn;
+ *shared_txn = txn;
+ return SVN_NO_ERROR;
}
/* Free the transaction object for transaction TXN_ID, and remove it
@@ -665,9 +673,10 @@ unlock_proto_rev_body(svn_fs_t *fs, const void *ba
const struct unlock_proto_rev_baton *b = baton;
const char *txn_id = b->txn_id;
apr_file_t *lockfile = b->lockcookie;
- fs_fs_shared_txn_data_t *txn = get_shared_txn(fs, txn_id, FALSE);
+ fs_fs_shared_txn_data_t *txn;
apr_status_t apr_err;
+ SVN_ERR(get_shared_txn(&txn, fs, txn_id, FALSE));
if (!txn)
return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
_("Can't unlock unknown transaction '%s'"),
@@ -743,7 +752,9 @@ get_writable_proto_rev_body(svn_fs_t *fs, const vo
void **lockcookie = b->lockcookie;
const char *txn_id = b->txn_id;
svn_error_t *err;
- fs_fs_shared_txn_data_t *txn = get_shared_txn(fs, txn_id, TRUE);
+ fs_fs_shared_txn_data_t *txn;
+
+ SVN_ERR(get_shared_txn(&txn, fs, txn_id, TRUE));
/* First, ensure that no thread in this process (including this one)
is currently writing to this transaction's proto-rev file. */
@@ -6640,15 +6651,17 @@ recover_find_max_ids(svn_fs_t *fs, svn_revnum_t re
if (svn_fs_fs__key_compare(node_id, max_node_id) > 0)
{
- assert(strlen(node_id) < MAX_KEY_SIZE);
- strncpy(max_node_id, node_id, MAX_KEY_SIZE - 1);
- max_node_id[MAX_KEY_SIZE - 1] = '\0';
+ const char *end = apr_cpystrn(max_node_id, node_id, MAX_KEY_SIZE);
+ if (end < max_node_id + strlen(node_id))
+ return svn_error_createf(SVN_ERR_FS_NOT_ID, NULL,
+ _("Node ID '%s' was truncated"), node_id);
}
if (svn_fs_fs__key_compare(copy_id, max_copy_id) > 0)
{
- assert(strlen(copy_id) < MAX_KEY_SIZE);
- strncpy(max_copy_id, copy_id, MAX_KEY_SIZE - 1);
- max_copy_id[MAX_KEY_SIZE - 1] = '\0';
+ const char *end = apr_cpystrn(max_copy_id, copy_id, MAX_KEY_SIZE);
+ if (end < max_copy_id + strlen(copy_id))
+ return svn_error_createf(SVN_ERR_FS_NOT_ID, NULL,
+ _("Copy ID '%s' was truncated"), copy_id);
}
if (kind == svn_node_file)
Re: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> I'll stay out of the whole SVN_ERR_ASSERT discussion.
> If we boil down the diff as below is this OK?
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 892649)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -34,6 +34,7 @@
> #include <apr_lib.h>
> #include <apr_md5.h>
> #include <apr_sha1.h>
> +#include <apr_strings.h>
> #include <apr_thread_mutex.h>
>
> #include "svn_pools.h"
> @@ -457,8 +458,7 @@ get_shared_txn(svn_fs_t *fs, const char *txn_id, s
> }
>
> assert(strlen(txn_id) < sizeof(txn->txn_id));
> - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> + apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> txn->being_written = FALSE;
>
> /* Link this transaction into the head of the list. We will typically
> @@ -6641,14 +6641,12 @@ recover_find_max_ids(svn_fs_t *fs, svn_revnum_t re
> if (svn_fs_fs__key_compare(node_id, max_node_id) > 0)
> {
> assert(strlen(node_id) < MAX_KEY_SIZE);
> - strncpy(max_node_id, node_id, MAX_KEY_SIZE - 1);
> - max_node_id[MAX_KEY_SIZE - 1] = '\0';
> + apr_cpystrn(max_node_id, node_id, MAX_KEY_SIZE);
> }
> if (svn_fs_fs__key_compare(copy_id, max_copy_id) > 0)
> {
> assert(strlen(copy_id) < MAX_KEY_SIZE);
> - strncpy(max_copy_id, copy_id, MAX_KEY_SIZE - 1);
> - max_copy_id[MAX_KEY_SIZE - 1] = '\0';
> + apr_cpystrn(max_copy_id, copy_id, MAX_KEY_SIZE);
> }
Looks good, yes.
- Julian
Re: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 21, 2009 at 05:38:28PM +0100, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > Sent: maandag 21 december 2009 16:51
> > To: Bert Huijben
> > Cc: 'Stefan Sperling'; 'Philip Martin'; 'Mark Phippard';
> > dev@subversion.apache.org
> > Subject: RE: svn commit: r884250 -
> > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >
> > Bert Huijben wrote:
> > >
> > > > -----Original Message-----
> > > > From: Julian Foad [mailto:julian.foad@wandisco.com]
> > > > Sent: maandag 21 december 2009 16:05
> > > > To: Stefan Sperling
> > > > Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> > > > Subject: Re: svn commit: r884250 -
> > > > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> > > >
> > > > Stefan Sperling wrote:
>
> > > Maybe assertions and aborts are easier for Subversion developers, but
> > they are not for our users.
> >
> > That's just stating what happens if there is no user-friendly handling
> > defined. The whole point of SVN_ERR_ASSERT() is to make an internal
> > consistency check brief to code, easy to handle consistently in
> > high-level code (convert them into regular error messages at some place
> > if appropriate) and still certain to be caught (in a crude way - abort)
> > if we forget or choose not to write handler code.
>
> One problem with this is that we convert all assertions to
> SVN_ERR_ASSERTION_FAIL, so we can never convert them to anything more
> detailed without processing the file and line number. Which you can't
> expect any ordinary user will do.
I'll stay out of the whole SVN_ERR_ASSERT discussion.
If we boil down the diff as below is this OK?
Stefan
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 892649)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -34,6 +34,7 @@
#include <apr_lib.h>
#include <apr_md5.h>
#include <apr_sha1.h>
+#include <apr_strings.h>
#include <apr_thread_mutex.h>
#include "svn_pools.h"
@@ -457,8 +458,7 @@ get_shared_txn(svn_fs_t *fs, const char *txn_id, s
}
assert(strlen(txn_id) < sizeof(txn->txn_id));
- strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
- txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
+ apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
txn->being_written = FALSE;
/* Link this transaction into the head of the list. We will typically
@@ -6641,14 +6641,12 @@ recover_find_max_ids(svn_fs_t *fs, svn_revnum_t re
if (svn_fs_fs__key_compare(node_id, max_node_id) > 0)
{
assert(strlen(node_id) < MAX_KEY_SIZE);
- strncpy(max_node_id, node_id, MAX_KEY_SIZE - 1);
- max_node_id[MAX_KEY_SIZE - 1] = '\0';
+ apr_cpystrn(max_node_id, node_id, MAX_KEY_SIZE);
}
if (svn_fs_fs__key_compare(copy_id, max_copy_id) > 0)
{
assert(strlen(copy_id) < MAX_KEY_SIZE);
- strncpy(max_copy_id, copy_id, MAX_KEY_SIZE - 1);
- max_copy_id[MAX_KEY_SIZE - 1] = '\0';
+ apr_cpystrn(max_copy_id, copy_id, MAX_KEY_SIZE);
}
if (kind == svn_node_file)
RE: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2009-12-21 at 17:38 +0100, Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > > Maybe assertions and aborts are easier for Subversion developers, but
> > they are not for our users.
> >
> > That's just stating what happens if there is no user-friendly handling
> > defined. The whole point of SVN_ERR_ASSERT() is to make an internal
> > consistency check brief to code, easy to handle consistently in
> > high-level code (convert them into regular error messages at some place
> > if appropriate) and still certain to be caught (in a crude way - abort)
> > if we forget or choose not to write handler code.
>
> One problem with this is that we convert all assertions to
> SVN_ERR_ASSERTION_FAIL, so we can never convert them to anything more
> detailed without processing the file and line number. Which you can't
> expect any ordinary user will do.
Yes, but assertions check "can't happen" conditions, not conditions that
the user might be able to understand, so a user isn't going to make
sense of such a condition no matter what the message says. In the
example we were looking at, the error is that a txn-id has reached over
200 characters in length. Theoretically, this could happen if the user
has been storing the history of the universe in Subversion for 99
trillion years, but in practice a user will only encounter this error as
a result of a bug in the software.
> So... a buffer index out of range or buffer load assertion
> (libsvn_subr) assertion would have to have the exact same error
> handling as a missing header assertion (serf is full of these).
Right. A user isn't going to understand a buffer index out of range
error in any way that helps them. If the authors of Serf have decided
that a missing header is a "can't happen, fatal error" condition, then
the same applies. (We can argue about whether those conditions have been
correctly classified.)
> Specifying an error code provides more options to our (library)
> users.
Yes, and if there are classes of error that you need/want to handle
differently, that are all presently lumped under SVN_ERR_ASSERTION_FAIL
(or indeed under any other error code), let's discuss them.
Is the Serf "missing header" the one that you are concerned with?
> The only thing we can say for an assertion is: send a mail to
> users@subversion.a.o /.t.o. including this piece of information you
> (as user) don't understand. And probably no one at users@ understands
> either.
Users on users@ could reply, "yes, we already reported that to the devs,
and it will be fixed in 1.x.x".
> Currently I ask our users to mail our support list, but I'm happy to
> hardcode the users@ list address for this class of errors if we decide
> to show this unresolvable error in more cases in 1.7...
> Please add useful error messages on all places that a user might
> encounter, likely or unlikely...
> Or even if the webserver sends an incomplete or broken response.
> (Anybody else handled svnbridge errors from Codeplex users?)
Yup, I agree.
Please start a thread to request the Serf "missing header" assertions be
changed to normal errors or warnings or something. (Serf was notorious
for having lots of that sort of thing... often just aborting.)
> Only when an error can't be reached by a normal user an assertion
> could be the correct solution.
Yup, I agree.
- Julian
RE: svn commit: r884250 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: maandag 21 december 2009 16:51
> To: Bert Huijben
> Cc: 'Stefan Sperling'; 'Philip Martin'; 'Mark Phippard';
> dev@subversion.apache.org
> Subject: RE: svn commit: r884250 -
> /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>
> Bert Huijben wrote:
> >
> > > -----Original Message-----
> > > From: Julian Foad [mailto:julian.foad@wandisco.com]
> > > Sent: maandag 21 december 2009 16:05
> > > To: Stefan Sperling
> > > Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> > > Subject: Re: svn commit: r884250 -
> > > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> > >
> > > Stefan Sperling wrote:
> > Maybe assertions and aborts are easier for Subversion developers, but
> they are not for our users.
>
> That's just stating what happens if there is no user-friendly handling
> defined. The whole point of SVN_ERR_ASSERT() is to make an internal
> consistency check brief to code, easy to handle consistently in
> high-level code (convert them into regular error messages at some place
> if appropriate) and still certain to be caught (in a crude way - abort)
> if we forget or choose not to write handler code.
One problem with this is that we convert all assertions to SVN_ERR_ASSERTION_FAIL, so we can never convert them to anything more detailed without processing the file and line number. Which you can't expect any ordinary user will do.
So... a buffer index out of range or buffer load assertion (libsvn_subr) assertion would have to have the exact same error handling as a missing header assertion (serf is full of these). Specifying an error code provides more options to our (library) users.
The only thing we can say for an assertion is: send a mail to users@subversion.a.o /.t.o. including this piece of information you (as user) don't understand. And probably no one at users@ understands either.
Currently I ask our users to mail our support list, but I'm happy to hardcode the users@ list address for this class of errors if we decide to show this unresolvable error in more cases in 1.7...
Please add useful error messages on all places that a user might encounter, likely or unlikely...
Or even if the webserver sends an incomplete or broken response. (Anybody else handled svnbridge errors from Codeplex users?)
Only when an error can't be reached by a normal user an assertion could be the correct solution.
Bert
>
> - Julian
>
RE: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad@wandisco.com]
> > Sent: maandag 21 december 2009 16:05
> > To: Stefan Sperling
> > Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> > Subject: Re: svn commit: r884250 -
> > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >
> > Stefan Sperling wrote:
> > > - assert(strlen(txn_id) < sizeof(txn->txn_id));
> > > - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> > > - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> > > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> > > + if (end < txn->txn_id + strlen(txn_id))
> > > + return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
> > NULL,
> > > + _("Transaction name '%s' was truncated"), txn_id);
> >
> > The "was truncated" error message is not helpful. (That sounds like the
> > server is saying, "I have renamed one of your transactions".) Any reason
> > not to use SVN_ERR_ASSERT?
> >
> > The logic looks correct.
>
> Assertions are not nice to library users. (Not everybody uses 'svn' as his/her subversion client). An assertion/abort in release code will most likely make my users loose at least some of their work.
That's exactly what SVN_ERR_ASSERT was invented for, and the reason it
is catchable in client code and its action is redefinable. Please see
its definition and past discussions.
> I don't know if this case is user and or network triggerable, but littering our code with assertions makes our code slow and easier to crash, while the error return method was designed to be more graceful.
It's not "littering", it's responsible, neat, flexible design. Using
loads of custom error message for internal malfunctions that the user
would have no ability to recover from is "littering".
> I guess this code is server side.
Yes.
> For this case errors are transferred to the client, while a server assertion will only be visible as 'connection closed unexpectedly' or something similar.
Depends on how it's handled at the caller. We should make the server do
something more friendly than just abort. Not by replacing all low-level
assertions with long-winded error returns, but by catching the assertion
errors at a higher level and handling a whole group of them in one
place.
> Maybe assertions and aborts are easier for Subversion developers, but they are not for our users.
That's just stating what happens if there is no user-friendly handling
defined. The whole point of SVN_ERR_ASSERT() is to make an internal
consistency check brief to code, easy to handle consistently in
high-level code (convert them into regular error messages at some place
if appropriate) and still certain to be caught (in a crude way - abort)
if we forget or choose not to write handler code.
- Julian
RE: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
>
> > -----Original Message-----
> > From: Julian Foad [mailto:julian.foad@wandisco.com]
> > Sent: maandag 21 december 2009 16:05
> > To: Stefan Sperling
> > Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> > Subject: Re: svn commit: r884250 -
> > /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >
> > Stefan Sperling wrote:
> > > - assert(strlen(txn_id) < sizeof(txn->txn_id));
> > > - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> > > - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> > > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> > > + if (end < txn->txn_id + strlen(txn_id))
> > > + return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
> > NULL,
> > > + _("Transaction name '%s' was truncated"), txn_id);
> >
> > The "was truncated" error message is not helpful. (That sounds like the
> > server is saying, "I have renamed one of your transactions".) Any reason
> > not to use SVN_ERR_ASSERT?
> >
> > The logic looks correct.
>
> Assertions are not nice to library users. (Not everybody uses 'svn' as his/her subversion client). An assertion/abort in release code will most likely make my users loose at least some of their work.
That's exactly what SVN_ERR_ASSERT was invented for, and the reason it
is catchable in client code and its action is redefinable. Please see
its definition and past discussions.
> I don't know if this case is user and or network triggerable, but littering our code with assertions makes our code slow and easier to crash, while the error return method was designed to be more graceful.
It's not "littering", it's responsible, neat, flexible design. Using
loads of custom error message for internal malfunctions that the user
would have no ability to recover from is "littering".
> I guess this code is server side.
Yes.
> For this case errors are transferred to the client, while a server assertion will only be visible as 'connection closed unexpectedly' or something similar.
Depends on how it's handled at the caller. We should make the server do
something more friendly than just abort. Not by replacing all low-level
assertions with long-winded error returns, but by catching the assertion
errors at a higher level and handling a whole group of them in one
place.
> Maybe assertions and aborts are easier for Subversion developers, but they are not for our users.
That's just stating what happens if there is no user-friendly handling
defined. The whole point of SVN_ERR_ASSERT() is to make an internal
consistency check brief to code, easy to handle consistently in
high-level code (convert them into regular error messages at some place
if appropriate) and still certain to be caught (in a crude way - abort)
if we forget or choose not to write handler code.
- Julian
Re: svn commit: r884250 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> -----Original Message----- From: Julian Foad
>> [mailto:julian.foad@wandisco.com] Sent: maandag 21 december 2009
>> 16:05 To: Stefan Sperling Cc: Philip Martin; Mark Phippard;
>> dev@subversion.apache.org Subject: Re: svn commit: r884250 -
>> /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>>
>> Stefan Sperling wrote:
>> > - assert(strlen(txn_id) < sizeof(txn->txn_id)); -
>> > strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1); -
>> > txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
>> > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id)); if
>> > + (end < txn->txn_id + strlen(txn_id)) return
>> > + svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
>> NULL,
>> > + _("Transaction name '%s' was
>> > + truncated"), txn_id);
>> The "was truncated" error message is not helpful. (That sounds like
>> the server is saying, "I have renamed one of your transactions".)
>> Any reason not to use SVN_ERR_ASSERT?
>>
>> The logic looks correct.
>
> Assertions are not nice to library users. (Not everybody uses 'svn' as
> his/her subversion client). An assertion/abort in release code will
> most likely make my users loose at least some of their work.
>
> I don't know if this case is user and or network triggerable, but
> littering our code with assertions makes our code slow and easier to
> crash, while the error return method was designed to be more graceful.
>
> I guess this code is server side. For this case errors are transferred
> to the client, while a server assertion will only be visible as
> connection closed unexpectedly' or something similar.
According to key-gen.h the whole change is more-or-less pointless.
/* The alphanumeric keys passed in and out of svn_fs_fs__next_key
are guaranteed never to be longer than this many bytes,
*including* the trailing null byte. It is therefore safe
to declare a key as "char key[MAX_KEY_SIZE]".
Note that this limit will be a problem if the number of
keys in a table ever exceeds
18217977168218728251394687124089371267338971528174
76066745969754933395997209053270030282678007662838
67331479599455916367452421574456059646801054954062
15017704234999886990788594743994796171248406730973
80736524850563115569208508785942830080999927310762
50733948404739350551934565743979678824151197232629
947748581376,
but that's a risk we'll live with for now. */
#define MAX_KEY_SIZE 200
Are we going to exceed the limit in the forseable future? If not then
all this checking is superflous. The original strlen looked fine to me.
--
Philip
RE: svn commit: r884250 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: maandag 21 december 2009 16:05
> To: Stefan Sperling
> Cc: Philip Martin; Mark Phippard; dev@subversion.apache.org
> Subject: Re: svn commit: r884250 -
> /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>
> Stefan Sperling wrote:
> > - assert(strlen(txn_id) < sizeof(txn->txn_id));
> > - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> > - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> > + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> > + if (end < txn->txn_id + strlen(txn_id))
> > + return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG,
> NULL,
> > + _("Transaction name '%s' was truncated"), txn_id);
>
> The "was truncated" error message is not helpful. (That sounds like the
> server is saying, "I have renamed one of your transactions".) Any reason
> not to use SVN_ERR_ASSERT?
>
> The logic looks correct.
Assertions are not nice to library users. (Not everybody uses 'svn' as his/her subversion client). An assertion/abort in release code will most likely make my users loose at least some of their work.
I don't know if this case is user and or network triggerable, but littering our code with assertions makes our code slow and easier to crash, while the error return method was designed to be more graceful.
I guess this code is server side. For this case errors are transferred to the client, while a server assertion will only be visible as 'connection closed unexpectedly' or something similar.
Maybe assertions and aborts are easier for Subversion developers, but they are not for our users.
Bert
Re: svn commit: r884250 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> - assert(strlen(txn_id) < sizeof(txn->txn_id));
> - strncpy(txn->txn_id, txn_id, sizeof(txn->txn_id) - 1);
> - txn->txn_id[sizeof(txn->txn_id) - 1] = '\0';
> + end = apr_cpystrn(txn->txn_id, txn_id, sizeof(txn->txn_id));
> + if (end < txn->txn_id + strlen(txn_id))
> + return svn_error_createf(SVN_ERR_FS_TXN_NAME_TOO_LONG, NULL,
> + _("Transaction name '%s' was truncated"), txn_id);
The "was truncated" error message is not helpful. (That sounds like the
server is saying, "I have renamed one of your transactions".) Any reason
not to use SVN_ERR_ASSERT?
The logic looks correct.
- Julian