You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/01/06 09:40:53 UTC
[PATCH] Fix 2441
Hi All,
Slightly better fix for #2441.
With regards
Kamesh Jayachandran
[[[
Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
intolerably vague
* subversion/libsvn_repos/load.c
(contextual_error): will return a svn_error_t * indicating which
header block
problematic header occured with a header name and a reason for failure.
]]]
Re: [PATCH] Fix 2441
Posted by Julian Foad <ju...@btopenworld.com>.
Garrett Rooney wrote:
> On 1/11/06, Kamesh Jayachandran <ka...@collab.net> wrote:
>
>>Hi Julian,
>>Thanks for your feedback.
>>Attaching the new patch.
>>Limiting the header_data to 20 characters as it seems that maximum
>>header name that I see in my dump seems to be 20 characters in length.
>>In case some spurious binary line happen to be there in the stream we
>>limit to print only 20 characters. Don't think it is worth to loop
>>through the characters for the valid printable ascii range. Let me know.
>
> Limiting to 20 characters isn't enough. Putting 20 bytes of arbitrary
> binary crap into an error that'll eventually get printed out to the
> terminal is a bad thing,
It could be annoying, but that's all, I think. The potential for damage is no
greater than trying to locate the erroneous part by running "grep" on the dump
file.
> and can cause security problems (that's why
> apache scrubs data that goes to its log files, it doesn't want to let
> random escape characters through because they can be used to do bad
> things to terminal emulators).
I don't see security as a real concern here. Unlike web server logs which can,
more or less, be triggered by anyone on the net, there is no common scenario in
which someone will unwittingly run "svnadmin load" on a maliciously engineered
dump file. It's normally a rare and manual operation. If someone set up an
server that created repositories from submitted dump files, then stdout/stderr
would probably be captured and not sent straight to a terminal. If a
developer/administrator/helpdesk is experimenting with data from untrusted
sources they should be doing so in a safe environment (e.g. not as 'root').
Little harm can result anyway in most terminals.
> You'll have to verify that the
> characters you're putting in the log are printable, and if not convert
> them to some safe form (numerical encoding of each byte, or
> something).
While that would be polite to the user, I no longer think it is really
necessary. If it is, it should go in our generic error printing code rather
than in this specific case, because there are all sorts of other ways of
getting nasty characters into our error messages - e.g.
$ svn log filename_with_nasty_^[[41m_characters
svn: 'filename_with_nasty__characters' is not under version control
which changes the background to red starting from between the two adjacent
underscores.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/11/06, Kamesh Jayachandran <ka...@collab.net> wrote:
> Hi Julian,
> Thanks for your feedback.
> Attaching the new patch.
> Limiting the header_data to 20 characters as it seems that maximum
> header name that I see in my dump seems to be 20 characters in length.
> In case some spurious binary line happen to be there in the stream we
> limit to print only 20 characters. Don't think it is worth to loop
> through the characters for the valid printable ascii range. Let me know.
Limiting to 20 characters isn't enough. Putting 20 bytes of arbitrary
binary crap into an error that'll eventually get printed out to the
terminal is a bad thing, and can cause security problems (that's why
apache scrubs data that goes to its log files, it doesn't want to let
random escape characters through because they can be used to do bad
things to terminal emulators). You'll have to verify that the
characters you're putting in the log are printable, and if not convert
them to some safe form (numerical encoding of each byte, or
something).
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441: 'svnadmin load' malformed dumpfile errors are
intolerably vague
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Julian.
With regards
Kamesh Jayachandran
Julian Foad wrote:
> Kamesh Jayachandran wrote:
>> Hi Julian,
>> Thanks for your feedback.
>> Attaching the new patch.
> [...]
>> [[[
>> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
>> intolerably vague
>>
>> * subversion/libsvn_repos/load.c
>> (read_header_block): Error message with a hint.
>> ]]]
>
> OK, thanks, I've committed this patch, or something very close to it,
> because it does improve the situation. I have said in the log message
> that it is "part of" issue #2441.
>
> - Julian
>
>
>> if (header_str->data[i] == '\0')
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Dump stream contains a malformed "
>> + "header (with no colon) ':' "
>> + "after '%.20s'"),
>> + header_str->data);
>> i++;
> [...]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441: 'svnadmin load' malformed dumpfile errors are
intolerably vague
Posted by Julian Foad <ju...@btopenworld.com>.
Kamesh Jayachandran wrote:
> Hi Julian,
> Thanks for your feedback.
> Attaching the new patch.
[...]
> [[[
> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
> intolerably vague
>
> * subversion/libsvn_repos/load.c
> (read_header_block): Error message with a hint.
> ]]]
OK, thanks, I've committed this patch, or something very close to it, because
it does improve the situation. I have said in the log message that it is "part
of" issue #2441.
- Julian
> if (header_str->data[i] == '\0')
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Dump stream contains a malformed "
> + "header (with no colon) ':' "
> + "after '%.20s'"),
> + header_str->data);
> i++;
[...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441
Posted by Julian Foad <ju...@btopenworld.com>.
Kamesh Jayachandran wrote:
> Hi Julian,
> Thanks for your feedback.
> Attaching the new patch.
> Limiting the header_data to 20 characters as it seems that maximum
> header name that I see in my dump seems to be 20 characters in length.
> In case some spurious binary line happen to be there in the stream we
> limit to print only 20 characters. Don't think it is worth to loop
> through the characters for the valid printable ascii range. Let me know.
I think that's OK, but I also think we should improve most of the other vague
error messages in this same file, otherwise we can't claim to be fixing issue
#2441.
I'm having a go at doing so, starting with a bit of factorisation of the three
code paths that extract a property key or value string from the stream.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi Julian,
Thanks for your feedback.
Attaching the new patch.
Limiting the header_data to 20 characters as it seems that maximum
header name that I see in my dump seems to be 20 characters in length.
In case some spurious binary line happen to be there in the stream we
limit to print only 20 characters. Don't think it is worth to loop
through the characters for the valid printable ascii range. Let me know.
With regards
Kamesh Jayachandran
[[[
Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
intolerably vague
* subversion/libsvn_repos/load.c
(read_header_block): Error message with a hint.
]]]
Julian Foad wrote:
> Kamesh Jayachandran wrote:
>> Thanks Julian, DLR, Garrett.
>> Julian,
>> Not sure how to reduce it further eventhough I could understand your
>> points on stuffs like some spurious newline introduced by the program
>> that generates the dump file.
>
> See below.
>
>> [[[
>> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
>> intolerably vague
>>
>> * subversion/libsvn_repos/load.c
>> (contextual_header_block_error): New function to return a detailed
>> error message.
>> (read_header_block): Call contextual_header_block_error().
>>
>> ]]]
>
>> Index: subversion/libsvn_repos/load.c
>> ===================================================================
>> --- subversion/libsvn_repos/load.c (revision 17993)
>> +++ subversion/libsvn_repos/load.c (working copy)
>> @@ -105,6 +105,58 @@
>> }
>>
>>
>> +/* Compose and return an error describing the problem REASON with the
>> + * dump file header KEY, and the block where these header key is
>> located.
>> + */
>> +static svn_error_t *
>> +contextual_header_block_error (apr_hash_t *headers, const char *key,
>> const char *reason)
>> +{
>> + char *block;
>> + if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_REVISION_NUMBER,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed revision '%s' header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), block, key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_NODE_PATH,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed node '%s' header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), block, key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, + SVN_REPOS_DUMPFILE_UUID,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed UUID header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else if ((block = apr_hash_get (headers, +
>> SVN_REPOS_DUMPFILE_MAGIC_HEADER,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed magic header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed unknown header block "
>> + "near the line starting with a key '%s' %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> +}
>
> There's a lot of duplication in that function. At the least I would
> like to see the common code (well, text, mainly) factored out.
> Additionally I would not bother trying to report the content of
> another header from the same block, unless there is evidence that
> doing so is really useful. As I said before, I suspect that it is not
> often really useful.
>
>
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return contextual_header_block_error (*headers, header_str->data, +
>> _("with no ':' character"));
>
> Translating a sentence fragment doesn't work in all languages.
> Generally, text to be translated has to be in complete sentences.
>
>
> Here is what I am suggesting for the whole patch:
>
>> Index: subversion/libsvn_repos/load.c
>> ===================================================================
>> --- subversion/libsvn_repos/load.c (revision 18020)
>> +++ subversion/libsvn_repos/load.c (working copy)
>> @@ -153,9 +153,10 @@
>> while (header_str->data[i] != ':')
>> {
>> if (header_str->data[i] == '\0')
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Dump stream contains a malformed "
>> + "header (with no colon): '%s'"),
>> + header_str->data);
>> i++;
>> }
>> /* Create a 'name' string and point to it. */
>> @@ -165,9 +166,10 @@
>> /* Skip over the NULL byte and the space following it. */
>> i += 2;
>> if (i > header_str->len)
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Dump stream contains a malformed "
>> + "header (no value after colon): '%s'"),
>> + header_str->data);
>>
>> /* Point to the 'value' string. */
>> value = header_str->data + i;
>
>
>
> I can already see another problem: the supposed malformed header
> string is quite likely to be a block of arbitrary ("binary") data of
> arbitrary size. Therefore it would be polite not to print it raw but
> to restrict the length and character set of what we print.
>
>
> Further, I see that load.c also emits other vague dumpfile errors
> (e.g. in stream_malformed()) that could do with a bit more detail.
>
> - Julian
>
Re: [PATCH] Fix 2441
Posted by Julian Foad <ju...@btopenworld.com>.
Kamesh Jayachandran wrote:
> Thanks Julian, DLR, Garrett.
> Julian,
> Not sure how to reduce it further eventhough I could understand your
> points on stuffs like some spurious newline introduced by the program
> that generates the dump file.
See below.
> [[[
> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
> intolerably vague
>
> * subversion/libsvn_repos/load.c
> (contextual_header_block_error): New function to return a detailed
> error message.
> (read_header_block): Call contextual_header_block_error().
>
> ]]]
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 17993)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -105,6 +105,58 @@
> }
>
>
> +/* Compose and return an error describing the problem REASON with the
> + * dump file header KEY, and the block where these header key is located.
> + */
> +static svn_error_t *
> +contextual_header_block_error (apr_hash_t *headers, const char *key, const char *reason)
> +{
> + char *block;
> + if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_REVISION_NUMBER,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed revision '%s' header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), block, key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_NODE_PATH,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed node '%s' header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), block, key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_UUID,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed UUID header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else if ((block = apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_MAGIC_HEADER,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed magic header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed unknown header block "
> + "near the line starting with a key '%s' %s "
> + "in dumpfile stream"), key, reason);
> + }
> +}
There's a lot of duplication in that function. At the least I would like to
see the common code (well, text, mainly) factored out. Additionally I would
not bother trying to report the content of another header from the same block,
unless there is evidence that doing so is really useful. As I said before, I
suspect that it is not often really useful.
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return contextual_header_block_error (*headers, header_str->data,
> + _("with no ':' character"));
Translating a sentence fragment doesn't work in all languages. Generally, text
to be translated has to be in complete sentences.
Here is what I am suggesting for the whole patch:
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 18020)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -153,9 +153,10 @@
> while (header_str->data[i] != ':')
> {
> if (header_str->data[i] == '\0')
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Dump stream contains a malformed "
> + "header (with no colon): '%s'"),
> + header_str->data);
> i++;
> }
> /* Create a 'name' string and point to it. */
> @@ -165,9 +166,10 @@
> /* Skip over the NULL byte and the space following it. */
> i += 2;
> if (i > header_str->len)
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Dump stream contains a malformed "
> + "header (no value after colon): '%s'"),
> + header_str->data);
>
> /* Point to the 'value' string. */
> value = header_str->data + i;
I can already see another problem: the supposed malformed header string is
quite likely to be a block of arbitrary ("binary") data of arbitrary size.
Therefore it would be polite not to print it raw but to restrict the length and
character set of what we print.
Further, I see that load.c also emits other vague dumpfile errors (e.g. in
stream_malformed()) that could do with a bit more detail.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix 2441
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Julian, DLR, Garrett.
Julian,
Not sure how to reduce it further eventhough I could understand your
points on stuffs like some spurious newline introduced by the program
that generates the dump file.
Find the new patch attached.
With regards
Kamesh Jayachandran
[[[
Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
intolerably vague
* subversion/libsvn_repos/load.c
(contextual_header_block_error): New function to return a detailed
error message.
(read_header_block): Call contextual_header_block_error().
]]]
Daniel Rall wrote:
> On Fri, 06 Jan 2006, Kamesh Jayachandran wrote:
>
>
>> Hi All,
>> Slightly better fix for #2441.
>>
>
> Thanks Kamesh! Last couple-few minor things.
>
>
>> [[[
>> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
>> intolerably vague
>>
>> * subversion/libsvn_repos/load.c
>> (contextual_error): will return a svn_error_t * indicating which
>> header block
>> problematic header occured with a header name and a reason for failure.
>>
>
> Usage of contextual_error() is missing from the change log message.
>
>
>> ]]]
>>
>>
>
>
>> Index: subversion/libsvn_repos/load.c
>> ===================================================================
>> --- subversion/libsvn_repos/load.c (revision 17993)
>> +++ subversion/libsvn_repos/load.c (working copy)
>> @@ -105,6 +105,56 @@
>> }
>>
>>
>> +/** This function gives a contextual clue of dump file malformed content. **/
>> +static svn_error_t *
>> +contextual_error (apr_hash_t *headers, char *key, char *reason)
>>
>
> Something like contextual_dumpfile_error() or
> contextual_header_block_error() would provide a more explicit name.
>
>
>> +{
>> + char *block=NULL;
>> + if ((block = (char *)apr_hash_get (headers,
>>
> ^
> We usually put a space here (and below):
>
> if ((block = (char *) apr_hash_get (headers,
>
>
>> + SVN_REPOS_DUMPFILE_REVISION_NUMBER,
>> + APR_HASH_KEY_STRING)) != NULL)
>>
> ^
> Now they'll line up. Bonus.
>
>
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed revision %s header block "
>> + "near the line starting with a key %s %s "
>> + "in dumpfile stream"), block, key, reason);
>>
>
> Quote the key value '%s' to help the reader here and below.
>
>
>> + }
>> + else if ((block = (char *)apr_hash_get (headers,
>> + SVN_REPOS_DUMPFILE_NODE_PATH,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed node %s header block "
>> + "near the line starting with a key %s %s "
>> + "in dumpfile stream"), block, key, reason);
>> + }
>> + else if ((block = (char *)apr_hash_get (headers,
>> + SVN_REPOS_DUMPFILE_UUID,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed UUID header block "
>> + "near the line starting with a key %s %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else if ((block = (char *)apr_hash_get (headers,
>> + SVN_REPOS_DUMPFILE_MAGIC_HEADER,
>> + APR_HASH_KEY_STRING)) != NULL)
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed magic header block "
>> + "near the line starting with a key %s %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> + else
>> + {
>> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> + _("Found malformed unknown header block "
>> + "near the line starting with a key %s %s "
>> + "in dumpfile stream"), key, reason);
>> + }
>> +}
>> +
>> /*----------------------------------------------------------------------*/
>>
>> /** The parser and related helper funcs **/
>> @@ -153,9 +203,8 @@
>> while (header_str->data[i] != ':')
>> {
>> if (header_str->data[i] == '\0')
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return contextual_error (*headers, header_str->data,
>> + "with no : character");
>>
>
> Quote the ':' character here and below.
>
>
>> i++;
>> }
>> /* Create a 'name' string and point to it. */
>> @@ -165,9 +214,8 @@
>> /* Skip over the NULL byte and the space following it. */
>> i += 2;
>> if (i > header_str->len)
>> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
>> - _("Found malformed header block "
>> - "in dumpfile stream"));
>> + return contextual_error (*headers, header_str->data,
>> + "with no value after a : character");
>>
> ...
>
Re: [PATCH] Fix 2441
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 06 Jan 2006, Kamesh Jayachandran wrote:
> Hi All,
> Slightly better fix for #2441.
Thanks Kamesh! Last couple-few minor things.
> [[[
> Fix issue #2441, 'svnadmin load' malformed dumpfile errors are
> intolerably vague
>
> * subversion/libsvn_repos/load.c
> (contextual_error): will return a svn_error_t * indicating which
> header block
> problematic header occured with a header name and a reason for failure.
Usage of contextual_error() is missing from the change log message.
>
> ]]]
>
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 17993)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -105,6 +105,56 @@
> }
>
>
> +/** This function gives a contextual clue of dump file malformed content. **/
> +static svn_error_t *
> +contextual_error (apr_hash_t *headers, char *key, char *reason)
Something like contextual_dumpfile_error() or
contextual_header_block_error() would provide a more explicit name.
> +{
> + char *block=NULL;
> + if ((block = (char *)apr_hash_get (headers,
^
We usually put a space here (and below):
if ((block = (char *) apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_REVISION_NUMBER,
> + APR_HASH_KEY_STRING)) != NULL)
^
Now they'll line up. Bonus.
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed revision %s header block "
> + "near the line starting with a key %s %s "
> + "in dumpfile stream"), block, key, reason);
Quote the key value '%s' to help the reader here and below.
> + }
> + else if ((block = (char *)apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_NODE_PATH,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed node %s header block "
> + "near the line starting with a key %s %s "
> + "in dumpfile stream"), block, key, reason);
> + }
> + else if ((block = (char *)apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_UUID,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed UUID header block "
> + "near the line starting with a key %s %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else if ((block = (char *)apr_hash_get (headers,
> + SVN_REPOS_DUMPFILE_MAGIC_HEADER,
> + APR_HASH_KEY_STRING)) != NULL)
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed magic header block "
> + "near the line starting with a key %s %s "
> + "in dumpfile stream"), key, reason);
> + }
> + else
> + {
> + return svn_error_createf (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Found malformed unknown header block "
> + "near the line starting with a key %s %s "
> + "in dumpfile stream"), key, reason);
> + }
> +}
> +
> /*----------------------------------------------------------------------*/
>
> /** The parser and related helper funcs **/
> @@ -153,9 +203,8 @@
> while (header_str->data[i] != ':')
> {
> if (header_str->data[i] == '\0')
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return contextual_error (*headers, header_str->data,
> + "with no : character");
Quote the ':' character here and below.
> i++;
> }
> /* Create a 'name' string and point to it. */
> @@ -165,9 +214,8 @@
> /* Skip over the NULL byte and the space following it. */
> i += 2;
> if (i > header_str->len)
> - return svn_error_create (SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> - _("Found malformed header block "
> - "in dumpfile stream"));
> + return contextual_error (*headers, header_str->data,
> + "with no value after a : character");
...
Re: [PATCH] Fix 2441
Posted by Kamesh Jayachandran <ka...@collab.net>.
Sure would do from now on.
Thanks
With regards
Kamesh Jayachandran
Marc Sherman wrote:
> Kamesh Jayachandran wrote:
>
>> Hi All,
>> Slightly better fix for #2441.
>>
>
> Kamesh, could you please start using descriptive subjects on your
> [PATCH] messages? That's probably why your last couple didn't get much
> attention initially.
>
> - Marc
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
Re: [PATCH] Fix 2441
Posted by Marc Sherman <ms...@projectile.ca>.
Kamesh Jayachandran wrote:
> Hi All,
> Slightly better fix for #2441.
Kamesh, could you please start using descriptive subjects on your
[PATCH] messages? That's probably why your last couple didn't get much
attention initially.
- Marc
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org