On May 27, 2009, at 11:39 PM, Davyd McColl wrote:
> On Wed, May 27, 2009 at 10:44 PM, Hyrum K. Wright <hyrum_wright_at_mail.utexas.edu
> > wrote:
>
> On May 27, 2009, at 3:24 PM, Davyd McColl wrote:
>
> Good day
>
> I have some C++ code which uses the svn_fs_* functions. I've noticed
> a bit of an oddity, and I'm hoping that someone can just tell me
> that I'm forgetting to do something. It seems that when I try to
> perform functions on dirs or files in the repo, if I specify a path
> that doesn't exist in the repo, I don't just get failure for the
> called function -- I also get a segfault in apr_terminate() when the
> last call is made to apr_terminate() (as I understand it, the last
> call in a stack of apr_initalize()[N] apr_terminate()[N] is the one
> called...
>
> A simple code example:
>
> const char *szPath = "/";
> apr_initialize();
> apr_pool_t *pt = svn_pool_create(NULL);
> svn_fs_initialize(pt);
> svn_repos_t *repo;
> svn_repos_open(&repo, argv[REPO_ARG], pt);
> svn_fs_t *fs = svn_repos_fs(repo);
> svn_fs_root_t *root;
> svn_fs_revision_root(&root, fs, 1, pt);
>
> apr_pool_t *ptSub = svn_pool_create(pt);
> apr_hash_t *hash;
> svn_fs_dir_entries(&hash, root, szPath, ptSub);
>
> svn_pool_destroy(ptSub);
> svn_pool_destroy(pt);
> apr_terminate();
> return 0;
>
> With szPath set to "/" as above, all works as expected and the
> application exits cleanly. However, if szPath were set to a path not
> available in the repo at the revision set (for example, if I set the
> path to "/foobar"), then not only does the svn_fs_dir_entries() call
> fail (yes, I know I'm not checking for the error here, but I know it
> does fail -- and I really couldn't expect otherwise for a path not
> in the repo...), but when the time comes for the apr_terminate()
> call, the application segfaults.
>
> I'm using subversion-1.6.1, compiled from source on my own machine
> with Visual Studio 2008. I'm assuming that there's some crucial step
> that I'm missing out in the code above and would dearly appreciate
> any constructive thoughts on the problem
>
> You've already named the crucial step you are missing: you aren't
> checking for errors.
>
> Subversion errors need to be checked for, and handled, if
> appropriate. By not handling any returned errors, you are
> effectively leaking the error memory for any error returned. To
> help ensure API users (include Subversion's own code) properly
> handle errors, Subversion is configured to crash on cleanup in debug
> mode if an error is left unhandled somewhere in the code. My guess
> is that this is what is happening to your code.
>
> If you are sure you want to ignore the returned errors, which I
> don't recommend, you can wrap each function which returns an error
> with a call to svn_error_clear().
>
> -Hyrum
>
> I would have said "fair enough", but this code example above is a
> little contrived for simplicity: in my real code, I do something
> more along the lines of:
>
> svn_error_t *err = svn_fs_dir_entries(&hash, root, szPath, ptSub);
> if (err != SVN_NO_ERROR)
> {
> // log the error and return from this caller level
> }
>
> In other words, I do have a kind of error handling which doesn't do
> anything more in the logic of the program (just aborts the current
> logic whilst logging an error), but which essentially ends up with
> the same code path as above. Perhaps a better question then is: what
> *should* I be doing when the dvn_fs_dir_entries() (or other
> svn_fs_* functions, for that matter), fail? Is there some kind of
> invocation that I should be making to svn_* functions to handle the
> problem? I've can't apr_hash_clear() on the hash pointer, for
> example (expected: there was nothing to return into the hash
> pointer). What should I be doing that I'me obviously not doing?
> Sorry if this sounds like a desperate cry for help -- but it is.
> Reading the subversion documentation in the headers and the
> automatically generated documentation online hasn't shed a glaring
> light on something that I should be doing in the event that this
> function fails.
>
> It's critical that I find the correct "oops, something went wrong"
> steps to follow -- this behaviour can also be replicated when trying
> to read the contents of a file in the repo which doesn't exist. The
> error only crops up at apr_terminate() time and obviously can't be
> handled there. There's also a very good chance that the final
> apr_terminate() call will be made well before the main application
> is supposed to exit: even if I was sloppy enough not to care if my
> app crashed at exit, I couldn't let this one slide.
The bottom line here is that the svn_error_t * should eventually be
cleared using svn_error_clear(), whether you do that in the if
statement above, or somewhere else is immaterial, but it needs to
happen before you get to apr_terminate(). Are you calling that
function on your returned errors (possibly after logging them)?
[meta comment: aborting on bad behavior like this isn't very user
friendly, but it does keep developers honest, and helps find
legitimate memory leaks. Since errors are allocated in the global
pool (because we can't depending on any other pool to last the length
the error may be needed), they will leak if not manually cleared.
sorry this is causing you pain, but you'll be thankful down the road.]
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=2356032
To unsubscribe from this discussion, e-mail: [users-unsubscribe_at_subversion.tigris.org].
Received on 2009-05-28 06:47:54 CEST