On Wed, Nov 25, 2009 at 09:15:03PM +0000, Philip Martin wrote:
> Stefan Sperling <[email protected]> writes:
>
> > On Wed, Nov 25, 2009 at 03:51:43PM -0500, Mark Phippard wrote:
> >> On Wed, Nov 25, 2009 at 3:27 PM, <[email protected]> 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)
