You are viewing a plain text version of this content. The canonical link for it is here.
Posted to apreq-dev@httpd.apache.org by Michael Koziarski <mi...@koziarski.com> on 2008/12/15 21:45:36 UTC
Questions about apreq_params
Hey guys,
first, apologies for messing up the subscribe email!
We've just started using apreq for some apache module development and
it's working out quite nicely. However we've noticed a few strange
issues.
The first of which is that apreq_params returns NULL if we use the
following as the multipart body:
http://github.com/rails/rails/tree/master/actionpack/test/fixtures/multipart/large_text_file
apr_table_t *request_body = apreq_params(req, r->pool);
if (request_body == NULL)
{
// we are seeing this
}
The documentation implies that it never does, which makes me wonder.
Is that just a stale piece of documentation or have we found a bug in
libapreq? Should we be handling this case in our module or figuring
out a test case for you guys to fix in 2.10?
--
Cheers
Koz
Re: Questions about apreq_params
Posted by Michael Koziarski <mi...@koziarski.com>.
> Clearly the function can return NULL, what documentation do you refer to ?
>
> APREQ_DECLARE(apr_table_t *)apreq_params(apreq_handle_t *req, apr_pool_t *p)
> {
> const apr_table_t *args, *body;
> apreq_args(req, &args);
> apreq_body(req, &body);
>
> if (args != NULL)
> if (body != NULL)
> return apr_table_overlay(p, args, body);
> else
> return apr_table_copy(p, args);
> else
> if (body != NULL)
> return apr_table_copy(p, body);
> else
> return NULL;
>
> }
The source doc in 2.08 that I have downloaded locally:
/**
* Returns a table containing key-value pairs for the full request
* (args + body).
*
* @param req request handle
* @param p allocates the returned table.
*
* @return table representing all available params; is never NULL.
*/
APREQ_DECLARE(apr_table_t *) apreq_params(apreq_handle_t *req, apr_pool_t *p);
--
Cheers
Koz
Re: Questions about apreq_params
Posted by Michael Koziarski <mi...@koziarski.com>.
On Tue, Dec 16, 2008 at 12:15 AM, Philip M. Gollucci
<pg...@p6m7g8.com> wrote:
> Michael Koziarski wrote:
>>>
>>> It should definitely not be deprecated. Its kind of 'core' to the
>>> module.
>>
>> But unless I'm mistaken, it'll just silently fail in these situations
>> with malformed bodies? What would be the suggested action in that
>> situation? something like this?
>
> I think step one is to come up with a test that reproduces the break in the
> docs. Commit it as 'skip', then fix the code so the test passes ensuring we
> don't break anything else in the process.
OK, I'm about to spend a week travelling but I've added this to my
TODO list. I'm far from an expert with this stuff so apologies if it
takes me a little while. At the very least I'll get us a request body
which repeatedly triggers it.
> --
> ------------------------------------------------------------------------
> Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354
> Consultant - P6M7G8 Inc. http://p6m7g8.net
> Senior System Admin - RideCharge, Inc. http://ridecharge.com
> 1024D/DB9B8C1C B90B FBC3 A3A1 C71A 8E70 3F8C 75B8 8FFB DB9B 8C1C
>
> Work like you don't need the money,
> love like you'll never get hurt,
> and dance like nobody's watching.
>
--
Cheers
Koz
Re: Questions about apreq_params
Posted by "Philip M. Gollucci" <pg...@p6m7g8.com>.
Michael Koziarski wrote:
>> It should definitely not be deprecated. Its kind of 'core' to the module.
>
> But unless I'm mistaken, it'll just silently fail in these situations
> with malformed bodies? What would be the suggested action in that
> situation? something like this?
I think step one is to come up with a test that reproduces the break in
the docs. Commit it as 'skip', then fix the code so the test passes
ensuring we don't break anything else in the process.
--
------------------------------------------------------------------------
Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354
Consultant - P6M7G8 Inc. http://p6m7g8.net
Senior System Admin - RideCharge, Inc. http://ridecharge.com
1024D/DB9B8C1C B90B FBC3 A3A1 C71A 8E70 3F8C 75B8 8FFB DB9B 8C1C
Work like you don't need the money,
love like you'll never get hurt,
and dance like nobody's watching.
Re: Questions about apreq_params
Posted by Michael Koziarski <mi...@koziarski.com>.
> It should definitely not be deprecated. Its kind of 'core' to the module.
But unless I'm mistaken, it'll just silently fail in these situations
with malformed bodies? What would be the suggested action in that
situation? something like this?
if (content_body == NULL)
{
// try apreq_args and log the failure
// try apreq_body and log the failure
}
At present we're just treating these as Bad Requests?
--
Cheers
Koz
Re: Questions about apreq_params
Posted by "Philip M. Gollucci" <pg...@p6m7g8.com>.
Michael Koziarski wrote:
> So should apreq_params be deprecated given the lack of RV checking?
> It seems that it's not possible to use it safely? I can easily
> implement that same 'overlay or copy' logic (with error checking)
> myself, but this function will probably still trip up the next unlucky
> traveller. Any ideas on a best fix for the library, rather than just
> my own module?
It should definitely not be deprecated. Its kind of 'core' to the module.
--
------------------------------------------------------------------------
Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354
Consultant - P6M7G8 Inc. http://p6m7g8.net
Senior System Admin - RideCharge, Inc. http://ridecharge.com
1024D/DB9B8C1C B90B FBC3 A3A1 C71A 8E70 3F8C 75B8 8FFB DB9B 8C1C
Work like you don't need the money,
love like you'll never get hurt,
and dance like nobody's watching.
Re: Questions about apreq_params
Posted by Michael Koziarski <mi...@koziarski.com>.
On Mon, Dec 15, 2008 at 11:28 PM, Philip M. Gollucci
<pg...@p6m7g8.com> wrote:
> Okay that documentation is both correct an incorrect.
>
> Its a stop gab. When you give no body and no args, you'll get a NULL, but
> in reality you should never need to do this. I'll take suggestions on how
> to word that correctly.
I'll have a think and get back to you.
> In the meantime, I'm not convinced you are using the API quite correctly.
>
> Can you have a look at the code in module/t/c-modules/*
> in particular,
>
> o mod_apreq_request_test.c - basic case
> o mod_apreq_upload_test.c - file uploads
>
> At miniumm, you should be checking the RV code of apreq_args == APR_SUCCESS.
> I'm betting its not that for you and if you decode the number to a string
> you'll find your problem.
My expectation was that I could safely call apreq_params and have that
return the the table of all the parameters in the request. But
looking at how that function is implemented (discarding the RV codes)
it's not really all that useful. I believe the test script I was
using was malformed so that's probably what's going on.
So should apreq_params be deprecated given the lack of RV checking?
It seems that it's not possible to use it safely? I can easily
implement that same 'overlay or copy' logic (with error checking)
myself, but this function will probably still trip up the next unlucky
traveller. Any ideas on a best fix for the library, rather than just
my own module?
--
Cheers
Koz
Re: Questions about apreq_params
Posted by "Philip M. Gollucci" <pg...@p6m7g8.com>.
Okay that documentation is both correct an incorrect.
Its a stop gab. When you give no body and no args, you'll get a NULL,
but in reality you should never need to do this. I'll take suggestions
on how to word that correctly.
--------------------------------------------------------------------------
-AaB03x
Content-Disposition: form-data; name="foo"
bar
--AaB03x
Content-Disposition: form-data; name="file"; filename="file.txt"
Content-Type: text/plain
foo
--AaB03x--
---------------------------------------------------------------------------
In the meantime, I'm not convinced you are using the API quite correctly.
Can you have a look at the code in module/t/c-modules/*
in particular,
o mod_apreq_request_test.c - basic case
o mod_apreq_upload_test.c - file uploads
At miniumm, you should be checking the RV code of apreq_args ==
APR_SUCCESS. I'm betting its not that for you and if you decode the
number to a string you'll find your problem.
--
------------------------------------------------------------------------
Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354
Consultant - P6M7G8 Inc. http://p6m7g8.net
Senior System Admin - RideCharge, Inc. http://ridecharge.com
1024D/DB9B8C1C B90B FBC3 A3A1 C71A 8E70 3F8C 75B8 8FFB DB9B 8C1C
Work like you don't need the money,
love like you'll never get hurt,
and dance like nobody's watching.
Re: Questions about apreq_params
Posted by "Philip M. Gollucci" <pg...@p6m7g8.com>.
Michael Koziarski wrote:
> Hey guys,
>
> first, apologies for messing up the subscribe email!
>
> We've just started using apreq for some apache module development and
> it's working out quite nicely. However we've noticed a few strange
> issues.
>
> The first of which is that apreq_params returns NULL if we use the
> following as the multipart body:
>
> http://github.com/rails/rails/tree/master/actionpack/test/fixtures/multipart/large_text_file
>
> apr_table_t *request_body = apreq_params(req, r->pool);
> if (request_body == NULL)
> {
> // we are seeing this
> }
>
>
> The documentation implies that it never does, which makes me wonder.
> Is that just a stale piece of documentation or have we found a bug in
> libapreq? Should we be handling this case in our module or figuring
> out a test case for you guys to fix in 2.10?
Clearly the function can return NULL, what documentation do you refer to ?
APREQ_DECLARE(apr_table_t *)apreq_params(apreq_handle_t *req, apr_pool_t *p)
{
const apr_table_t *args, *body;
apreq_args(req, &args);
apreq_body(req, &body);
if (args != NULL)
if (body != NULL)
return apr_table_overlay(p, args, body);
else
return apr_table_copy(p, args);
else
if (body != NULL)
return apr_table_copy(p, body);
else
return NULL;
}
--
------------------------------------------------------------------------
Philip M. Gollucci (pgollucci@p6m7g8.com) c: 703.336.9354
Consultant - P6M7G8 Inc. http://p6m7g8.net
Senior System Admin - RideCharge, Inc. http://ridecharge.com
1024D/DB9B8C1C B90B FBC3 A3A1 C71A 8E70 3F8C 75B8 8FFB DB9B 8C1C
Work like you don't need the money,
love like you'll never get hurt,
and dance like nobody's watching.