[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix possible crash in svn_fs_fs__lock() / svn_fs_fs__unlock()

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Thu, 5 Feb 2015 13:48:11 +0300

Sergey Raevskiy <sergey.raevskiy_at_visualsvn.com> writes:

> This happens beacuse lb.infos field is getting initialized only in function
> lock_body() (see the code below). So, if svn_fs_fs__with_write_lock() fails
> without actual invoking the lock_body(), lb.infos will be left uninitialized.

[...]

> I've attached the patch with crashing test and simple fix for this issue.

Comments inline.

> @@ -1056,9 +1053,6 @@ unlock_body(void *baton, apr_pool_t *pool)
> int i, max_components = 0, outstanding = 0;
> apr_pool_t *iterpool = svn_pool_create(pool);
>
> - ub->infos = apr_array_make(ub->result_pool, ub->targets->nelts,
> - sizeof(struct unlock_info_t));
> -
> SVN_ERR(ub->fs->vtable->youngest_rev(&youngest, ub->fs, pool));
> SVN_ERR(ub->fs->vtable->revision_root(&root, ub->fs, youngest, pool));

The unlock_body() function has multiple calling sites — svn_fs_fs__unlock()
and unlock_single(). This patch moves the ub->infos initialization into
svn_fs_fs__unlock(), but leaves unlock_single() unchanged. Hence, we will
most likely see another segfault due to us accessing uninitialized memory:

  Use of uninitialised value of size 8
     at 0x59BA8D6: apr_array_push (...)
     by 0x6413D1B: unlock_body (lock.c:1089)
     by 0x6414BE6: get_lock (lock.c:1181)
     by 0x6412CD1: svn_fs_fs__allow_locked_operation (lock.c:511)
     by 0x642233B: commit_body (transaction.c:3251)
     by 0x6408E9B: with_lock (fs_fs.c:221)
     by 0x6422130: svn_fs_fs__commit (transaction.c:3613)
     by 0x6425E0D: svn_fs_fs__commit_txn (tree.c:2224)
     by 0x4019B4: lock_expiration (locks-test.c:659)
     by 0x4E3DB34: test_thread (svn_test_main.c:525)
     by 0x5BE1181: start_thread (pthread_create.c:312)
     by 0x5EF230C: clone (clone.S:111)

  Invalid write of size 8
     at 0x6413D1C: unlock_body (lock.c:1089)
     by 0x6414BE6: get_lock (lock.c:1181)
     by 0x6412CD1: svn_fs_fs__allow_locked_operation (lock.c:511)
     by 0x642233B: commit_body (transaction.c:3251)
     by 0x6408E9B: with_lock (fs_fs.c:221)
     by 0x6422130: svn_fs_fs__commit (transaction.c:3613)
     by 0x6425E0D: svn_fs_fs__commit_txn (tree.c:2224)
     by 0x4019B4: lock_expiration (locks-test.c:659)
     by 0x4E3DB34: test_thread (svn_test_main.c:525)
     by 0x5BE1181: start_thread (pthread_create.c:312)
     by 0x5EF230C: clone (clone.S:111)

> + SVN_ERR(create_greek_fs(&fs, &newrev, "obtain-write-lock-failure-test",
> + opts, pool));
> + SVN_ERR(svn_fs_create_access(&access, "bubba", pool));
> + SVN_ERR(svn_fs_set_access(fs, access));

Should probably be named "test-obtain-write-lock-failure".

+ /* Make a read only 'write-lock' file. This prevents any write operations
+ from being executed. */
+ SVN_ERR(svn_io_set_file_read_only("obtain-write-lock-failure-test/write-lock",
+ TRUE, pool));

I suppose there is no reason to use ignore_enoent = TRUE here, right? The
'write-lock' is always there and if it is not, the test shouldn't give a
false positive.

Regards,
Evgeny Kotkov
Received on 2015-02-05 11:50:33 CET

This is an archived mail posted to the Subversion Dev mailing list.