You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Ph. Marek" <ph...@bmlv.gv.at> on 2005/04/04 08:09:40 UTC
missing error checking in svn_config__parse_file
This is against trunk
Regards
Phil
[[[
* subversion/libsvn_subr/config_file.c
(svn_config__parse_file): Check the error code possibly
returned
]]]
=== subversion/libsvn_subr/config_file.c
==================================================================
--- subversion/libsvn_subr/config_file.c (revision 404)
+++ subversion/libsvn_subr/config_file.c (local)
@@ -491,6 +491,9 @@
err = parse_option (&ch, &ctx);
break;
}
+
+ if (err != SVN_NO_ERROR)
+ return err;
}
while (ch != EOF);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: missing error checking in svn_config__parse_file
Posted by "Ph. Marek" <ph...@bmlv.gv.at>.
On Monday 04 April 2005 13:41, Peter N. Lundblad wrote:
> And if you read that function, you see that itt will modify ch when it
> returns an error. Same holds for parse_section AFAICT.
I saw that.
> If there is a place
> where ch is not set to EOF, please show it or -even better - provide a
> patch to fix that.
Well, my proposed patch for escape sequences :-)
> The problem with your patch (ignoring the leak) is that it makes the code
> very confusing. In many (all I think) places, it sets both err and the
> character to EOF. Then there is a special test at the end of the loop.
Yes.
> > And from my feeling, the test should be for
> >
> > err != SVN_NO_ERROR,
> > not some other secondary variable, which is likely to be forgotten (as it
> > happened me with parsing escape-characters!)
>
> IMO this is a valid point. I don't know why the code is like it is, and
> there is no comment explaining it either. Also, parse_option and
> parse_section are poorly documented.
That's why I overlooked that point.
> Maybe the reason is performance, although I think this extra pointer check
> would hardly make a measurable difference, even in a parsing loop like
> this.
And all functions set err. So it doesn't make sense to set another variable to
a special value too, because testing a char is not faster than testing a
pointer.
> If you think this hsould be fixed, feel free to submit a complete patch
> which we could discuss.
>
> > > OTOH, looking at the code below, it seems that we leak an error if err
> > > got set in the loop and ferror returns true. It needs a deeper look,
> > > though.
> >
> > Ok.
> > Leaving it to you.
>
> Heh...
Well, I don't think I'll find the time this week. So feel free to do that :-)
Regards,
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: missing error checking in svn_config__parse_file
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 4 Apr 2005, Ph. Marek wrote:
> On Monday 04 April 2005 11:47, Peter N. Lundblad wrote:
> > This leaks the open file. also note that whenever err is set, ch gets set
> > to EOF, so the loop will terminate in that case. So, you don't need this
> > fix.
> No, it doesn't. The call to parse_option has no ch=EOF, and I'm not sure if
> it's set everywhere.
> else
> err = parse_option (&ch, &ctx);
> break;
And if you read that function, you see that itt will modify ch when it
returns an error. Same holds for parse_section AFAICT. If there is a place
where ch is not set to EOF, please show it or -even better - provide a
patch to fix that.
The problem with your patch (ignoring the leak) is that it makes the code
very confusing. In many (all I think) places, it sets both err and the
character to EOF. Then there is a special test at the end of the loop.
> And from my feeling, the test should be for
> err != SVN_NO_ERROR,
> not some other secondary variable, which is likely to be forgotten (as it
> happened me with parsing escape-characters!)
>
IMO this is a valid point. I don't know why the code is like it is, and
there is no comment explaining it either. Also, parse_option and
parse_section are poorly documented.
Maybe the reason is performance, although I think this extra pointer check
would hardly make a measurable difference, even in a parsing loop like
this.
If you think this hsould be fixed, feel free to submit a complete patch
which we could discuss.
> > OTOH, looking at the code below, it seems that we leak an error if err got
> > set in the loop and ferror returns true. It needs a deeper look, though.
> Ok.
> Leaving it to you.
Heh...
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: missing error checking in svn_config__parse_file
Posted by Branko Čibej <br...@xbc.nu>.
Ph. Marek wrote:
>On Monday 04 April 2005 11:47, Peter N. Lundblad wrote:
>
>
>>This leaks the open file. also note that whenever err is set, ch gets set
>>to EOF, so the loop will terminate in that case. So, you don't need this
>>fix.
>>
>>
>No, it doesn't.
>
Do tell. Yes it does, it's just not as obvious as you seem to think it
should be.
>And from my feeling, the test should be for
> err != SVN_NO_ERROR,
>
>
Nope, see HACKING. We never compar to SVN_NO_ERROR.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: missing error checking in svn_config__parse_file
Posted by "Ph. Marek" <ph...@bmlv.gv.at>.
On Monday 04 April 2005 11:47, Peter N. Lundblad wrote:
> This leaks the open file. also note that whenever err is set, ch gets set
> to EOF, so the loop will terminate in that case. So, you don't need this
> fix.
No, it doesn't. The call to parse_option has no ch=EOF, and I'm not sure if
it's set everywhere.
else
err = parse_option (&ch, &ctx);
break;
And from my feeling, the test should be for
err != SVN_NO_ERROR,
not some other secondary variable, which is likely to be forgotten (as it
happened me with parsing escape-characters!)
> OTOH, looking at the code below, it seems that we leak an error if err got
> set in the loop and ferror returns true. It needs a deeper look, though.
Ok.
Leaving it to you.
Regards,
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: missing error checking in svn_config__parse_file
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 4 Apr 2005, Ph. Marek wrote:
> [[[
>
> * subversion/libsvn_subr/config_file.c
> (svn_config__parse_file): Check the error code possibly
> returned
>
> ]]]
>
> === subversion/libsvn_subr/config_file.c
> ==================================================================
> --- subversion/libsvn_subr/config_file.c (revision 404)
> +++ subversion/libsvn_subr/config_file.c (local)
> @@ -491,6 +491,9 @@
> err = parse_option (&ch, &ctx);
> break;
> }
> +
> + if (err != SVN_NO_ERROR)
> + return err;
> }
> while (ch != EOF);
>
This leaks the open file. also note that whenever err is set, ch gets set
to EOF, so the loop will terminate in that case. So, you don't need this
fix.
OTOH, looking at the code below, it seems that we leak an error if err got
set in the loop and ferror returns true. It needs a deeper look, though.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org