Impossible to POST binary data with CURLOPT_POSTFIELDS

[Spacen Jasset]

For libcurl 1.17.0


On August 1st url.c was changed (Revision 1.631) to make a copy of post
data so

that the caller need not keep the post buffer arround while curl operates,

unfortunatly this change has broken binary http posts since the code now
does a

strdup.

See: Curl_setstropt

It isn't possible to remedy this and keep the new semantics. The options
for a fix

are therefore:

0) Revert the code to do what it did before and require the buffer to be
present

durring the action

1) As (0) and then add a new api to make a copy of the post data

** The following options would change the API and application compatability:

2) Change CURLOPT_POSTFIELDS to include a size parameter

3) Require CURLOPT_POSTFIELDSIZE to be called first



In my copy of libcurl I have done (3) for the moment, but I have not
proper fix as

it involved changing the api or adding a new one.

To reproduce
============

I used wireshark to monitor the network traffic, and stepping into the code.

char buffer[] = "only\0the first word will get sent.";
long size = sizeof buffer;

curl_easy_setopt(curl, CURLOPT_URL, "http://<any-server>");
curl_easy_setopt(curl, CURLOPT_POSTFIELDS, buffer, size);
curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, size);
CURLcode result = curl_easy_perform(curl);

[Daniel Stenberg]

Post by Spacen Jasset
For libcurl 1.17.0

That's 7.17.0, with an initial seven not a one.

Post by Spacen Jasset
On August 1st url.c was changed (Revision 1.631) to make a copy of post data
so that the caller need not keep the post buffer arround while curl
operates, unfortunatly this change has broken binary http posts since the
code now does a strdup.
See: Curl_setstropt
It isn't possible to remedy this and keep the new semantics. The options for
0) Revert the code to do what it did before and require the buffer to be
present durring the action
1) As (0) and then add a new api to make a copy of the post data
2) Change CURLOPT_POSTFIELDS to include a size parameter
3) Require CURLOPT_POSTFIELDSIZE to be called first

Ouch, yes I can only confirm that you're of course 100% correct and I really
cannot believe I didn't see this problem before! :-(

I don't like #0, #1 or #2, and #3 would be unfortunate.

I'm thinking perhaps the best approach - which is a bit of a trade-off between
the old way and the current way - is to allow the options in any order but do
the actual memory duplication when the *perform() function is called...

Comments on this?

[Dan Fandrich]

Post by Daniel Stenberg

Post by Spacen Jasset
2) Change CURLOPT_POSTFIELDS to include a size parameter
3) Require CURLOPT_POSTFIELDSIZE to be called first

Ouch, yes I can only confirm that you're of course 100% correct and I really
cannot believe I didn't see this problem before! :-(
I don't like #0, #1 or #2, and #3 would be unfortunate.
I'm thinking perhaps the best approach - which is a bit of a trade-off
between the old way and the current way - is to allow the options in any
order but do the actual memory duplication when the *perform() function is
called...

That eliminates a key benefit of the new approach which is the ability to
create temporary strings on the stack, set them in libcurl and not worry
about them any more. Unless you mean to treat CURLOPT_POSTFIELDS as a
special case and only delay copying that one (better, but inconsistent).

What about copying a hybrid approach; leave the existing behaviour as-is
except:

- If CURLOPT_POSTFIELDSIZE *was* already set, copy that much data in
CURLOPT_POSTFIELDS instead of strdup().

- If it was *not* already set, do strdup() (as now); when
CURLOPT_POSTFIELDSIZE is set, compare the value to the amount of data
actually copied. If it's equal, great. If it's not, do the copy again,
right this time.

That second copy breaks the 7.17.0 model of copy-on-set, but is fully
backwards compatibile with pre-7.17.0 versions. At some point that
compatibility mode could be removed to attain full consistency, and replaced
with an error call. Then, CURLOPT_POSTFIELDSIZE would become mandatory
before CURLOPT_POSTFIELDS.

Post by Daniel Stenberg

Post by Spacen Jasset

Dan

[Daniel Stenberg]

Post by Dan Fandrich
- If CURLOPT_POSTFIELDSIZE *was* already set, copy that much data in
CURLOPT_POSTFIELDS instead of strdup().
- If it was *not* already set, do strdup() (as now); when
CURLOPT_POSTFIELDSIZE is set, compare the value to the amount of data
actually copied. If it's equal, great. If it's not, do the copy again,
right this time.

Yes, that's a better approach than my previous suggestion. And since the
7.17.0 approach is broken anyway, I think getting back to how things worked
pre-7.17.0 is sensible. I mean, so that the lib works with code as it was
written and working before 7.17.0.

Will you do the fix? I'm running a bit short of time here...

[Spacen Jasset]

Post by Daniel Stenberg

Post by Dan Fandrich
- If CURLOPT_POSTFIELDSIZE *was* already set, copy that much data in
CURLOPT_POSTFIELDS instead of strdup().
- If it was *not* already set, do strdup() (as now); when
CURLOPT_POSTFIELDSIZE is set, compare the value to the amount of data
actually copied. If it's equal, great. If it's not, do the copy
again, right this time.

Yes, that's a better approach than my previous suggestion. And since the
7.17.0 approach is broken anyway, I think getting back to how things
worked pre-7.17.0 is sensible. I mean, so that the lib works with code
as it was written and working before 7.17.0.
Will you do the fix? I'm running a bit short of time here...

I am a little confused, are we going to say that you use POSTFIELDSIZE
before setting the data? That won't be backwards compatible with old
apps since the null bytes will get in the way... Or are you going to
revert/change the code to not make a copy?


As far as I could see there was no possible fix to restore compatability
without reverting(or changing) the code for CURLOPT_POSTFIELDS back to
how it used to work -- not copying the data. -- can't even add a
var_args optional parameter becuase it's presence can't be detected.

NB - I don't personally mind what happens but it may break other
applications that use libcurl.

I am willing to sumbit a patch if you tell me what you would like done
(and it's not loads of code!)

[Dan Fandrich]

Post by Spacen Jasset
I am a little confused, are we going to say that you use POSTFIELDSIZE
before setting the data? That won't be backwards compatible with old apps
since the null bytes will get in the way... Or are you going to
revert/change the code to not make a copy?

Forcing users to set the size first would be the long-term solution. In
the interim, I'm suggesting first strduping the data as is presently done
when the CURLOPT_POSTFIELDS comes in, then copying it again when the size
comes in the size is different from what we copied. In the case of a
normal C string, the size will be the same and the second copy won't
happen.

Post by Spacen Jasset
As far as I could see there was no possible fix to restore compatability
without reverting(or changing) the code for CURLOPT_POSTFIELDS back to how
it used to work -- not copying the data. -- can't even add a var_args
optional parameter becuase it's presence can't be detected.

The rest of libcurl assumes that the data will be copied and must later
be freed, so the data needs to be copied at some point to maintain
consistency within libcurl.

Post by Spacen Jasset
NB - I don't personally mind what happens but it may break other
applications that use libcurl.

The proposed heuristic will be completely compatible with pre-7.17.0
programs. The only difference will be for programs written for 7.17.0
that assume the copy will take place early and that are sending binary
postfields, but they're broken now anyway, so this difference in behaviour
for them (late copy) is irrelevant.

Post by Spacen Jasset
I am willing to sumbit a patch if you tell me what you would like done (and
it's not loads of code!)

It will probably end up being one or two dozen new lines of code. I'll
let you tackle it unless you say otherwise.

Post by Spacen Jasset

Dan

[Spacen Jasset]

Post by Dan Fandrich

Post by Spacen Jasset
I am a little confused, are we going to say that you use POSTFIELDSIZE
before setting the data? That won't be backwards compatible with old apps
since the null bytes will get in the way... Or are you going to
revert/change the code to not make a copy?

Forcing users to set the size first would be the long-term solution. In
the interim, I'm suggesting first strduping the data as is presently done
when the CURLOPT_POSTFIELDS comes in, then copying it again when the size
comes in the size is different from what we copied. In the case of a
normal C string, the size will be the same and the second copy won't
happen.

Post by Spacen Jasset
As far as I could see there was no possible fix to restore compatability
without reverting(or changing) the code for CURLOPT_POSTFIELDS back to how
it used to work -- not copying the data. -- can't even add a var_args
optional parameter becuase it's presence can't be detected.

The rest of libcurl assumes that the data will be copied and must later
be freed, so the data needs to be copied at some point to maintain
consistency within libcurl.

Post by Spacen Jasset
NB - I don't personally mind what happens but it may break other
applications that use libcurl.

The proposed heuristic will be completely compatible with pre-7.17.0
programs. The only difference will be for programs written for 7.17.0
that assume the copy will take place early and that are sending binary
postfields, but they're broken now anyway, so this difference in behaviour
for them (late copy) is irrelevant.

Post by Spacen Jasset
I am willing to sumbit a patch if you tell me what you would like done (and
it's not loads of code!)

It will probably end up being one or two dozen new lines of code. I'll
let you tackle it unless you say otherwise.

Post by Spacen Jasset

Dan

Ok yes I see now. That's a good idea. There is the slight possibility
that someone will set the post data, free their buffer, and then set the
size, but that is unlikely isn't it. I'll have a go at implementing it
when I am at work tomorrow.


This fix will require the buffer address to be stored somewhere for
later when the size is set, can this go in the CURL structure? It won't
break the multi curl api will it? I must confess I've not looked at that
api yet.

[Colin Hogben]

Post by Dan Fandrich
I'm suggesting first strduping the data as is presently done
when the CURLOPT_POSTFIELDS comes in, then copying it again when the size
comes in the size is different from what we copied. In the case of a
normal C string, the size will be the same and the second copy won't
happen.

I think there is a potential risk if the user wishes to post a small
amount of binary data which contains no NUL, by setting
CURLOPT_POSTFIELDS first and then the size later. Trying to strdup it
will run off the end, maybe for megabytes if there no NUL byte anywhere
soon after it in memory, or you could reach an address which causes a
memory exception.

[Spacen Jasset]

Post by Colin Hogben

Post by Dan Fandrich
I'm suggesting first strduping the data as is presently done
when the CURLOPT_POSTFIELDS comes in, then copying it again when the size
comes in the size is different from what we copied. In the case of a
normal C string, the size will be the same and the second copy won't
happen.

I think there is a potential risk if the user wishes to post a small
amount of binary data which contains no NUL, by setting
CURLOPT_POSTFIELDS first and then the size later. Trying to strdup it
will run off the end, maybe for megabytes if there no NUL byte anywhere
soon after it in memory, or you could reach an address which causes a
memory exception.

Yes your right about this. The proposed solution won't work will it.
Doesn't matter if it's a small or big binary buffer with no nulls. On
manu platforums a trap signal is generated, and it's undefined behaviour
in C if you run off the end of an allocated piece of memory.

Therefore a different fix is required:- that is to copy the address of
the buffer and only extact the data from it when the perform command is
executed. This still seems a "bit risky" becase someone might deallocate
the buffer in the mean time, or at least they might if they read the
current curl documentation. It's also negates all the usefulness of the
data being eventually copied.

[Dan Fandrich]

Post by Daniel Stenberg
Yes, that's a better approach than my previous suggestion. And since the
7.17.0 approach is broken anyway, I think getting back to how things worked
pre-7.17.0 is sensible. I mean, so that the lib works with code as it was
written and working before 7.17.0.
Will you do the fix? I'm running a bit short of time here...

I should be able to get to it by the weekend. You need to go pack!

Post by Daniel Stenberg

Dan

[Patrick Monnerat]

Post by Spacen Jasset
0) Revert the code to do what it did before and require the buffer to
be present durring the action
1) As (0) and then add a new api to make a copy of the post data
** The following options would change the API and application
2) Change CURLOPT_POSTFIELDS to include a size parameter
3) Require CURLOPT_POSTFIELDSIZE to be called first

An other solution would be to revert CURLOPT_POSTFIELDS to the non-copy
version, and add a new option CURLOPT_COPYPOSTFIELDS.
This will restore binary compatibility with pre 7.17.0 applications and
allow the copy feature too...

[Daniel Stenberg]

Post by Patrick Monnerat
An other solution would be to revert CURLOPT_POSTFIELDS to the non-copy
version, and add a new option CURLOPT_COPYPOSTFIELDS. This will restore
binary compatibility with pre 7.17.0 applications and allow the copy feature
too...

Given the current facts and feedback, I guess this is the best idea so far. It
will of course make CURLOPT_POSTFIELDS act in a non-obvious way compared to
all the other setopt() arguments (thus it may lead to some surprising bugs for
users) and CURLOPT_COPYPOSTFIELDS would need to have the size set before it
can copy the fields (if binary contents are desired).

[Spacen Jasset]

Post by Daniel Stenberg

Post by Patrick Monnerat
An other solution would be to revert CURLOPT_POSTFIELDS to the
non-copy version, and add a new option CURLOPT_COPYPOSTFIELDS. This
will restore binary compatibility with pre 7.17.0 applications and
allow the copy feature too...

Given the current facts and feedback, I guess this is the best idea so
far. It will of course make CURLOPT_POSTFIELDS act in a non-obvious way
compared to all the other setopt() arguments (thus it may lead to some
surprising bugs for users) and CURLOPT_COPYPOSTFIELDS would need to have
the size set before it can copy the fields (if binary contents are
desired).

If this were adopted can't an extra varargs parameter be added. giving:

curl_easy_setopt(curl, CURLOPT_COPYPOSTFIELDS, data, size);

[Daniel Stenberg]

Post by Spacen Jasset

Post by Daniel Stenberg
Given the current facts and feedback, I guess this is the best idea so far.
It will of course make CURLOPT_POSTFIELDS act in a non-obvious way compared
to all the other setopt() arguments (thus it may lead to some surprising
bugs for users) and CURLOPT_COPYPOSTFIELDS would need to have the size set
before it can copy the fields (if binary contents are desired).

curl_easy_setopt(curl, CURLOPT_COPYPOSTFIELDS, data, size);

It could in theory, yes, but since we have ~140 options right now that takes a
single argument (after the option) only, I would rather not introduce it now.

[Patrick Monnerat]

Post by Daniel Stenberg
Given the current facts and feedback, I guess this is the best idea so

far. It will of course make CURLOPT_POSTFIELDS act in a non-obvious way
compared to all the other setopt() arguments (thus it may lead to some
surprising bugs for
users) and CURLOPT_COPYPOSTFIELDS would need to have the size set before
it can copy the fields (if binary contents are desired).


The attached patch should do it !

I did not tested it intensively, because I do not have anymore time to
work on it until Monday. So it's provided here for people in a hurry and
those wanting to test...

Have a good week-end.

[Patrick Monnerat]

Fixed in CVS:
_ CURLOPT_POSTFIELDS is now back to static (non-copy)
_ CURLOPT_COPYPOSTFIELDS option added to support copy mode: if
CURLOPT_POSTFIELDSIZE is needed, it must be used prior to
CURLOPT_COPYPOSTFIELDS and not altered after, unless another
CURLOPT_POSTFIELDS/CURLOPT_COPYPOSTFIELDS overwrites the former.
_ CURLOPT_POSTFIELDSIZE_LARGE may be used instead of
CURLOPT_POSTFIELDSIZE for CURLOPT_COPYPOSTFIELDS providing the given
size fits in a size_t.

[Spacen Jasset]

Post by Patrick Monnerat
_ CURLOPT_POSTFIELDS is now back to static (non-copy)
_ CURLOPT_COPYPOSTFIELDS option added to support copy mode: if
CURLOPT_POSTFIELDSIZE is needed, it must be used prior to
CURLOPT_COPYPOSTFIELDS and not altered after, unless another
CURLOPT_POSTFIELDS/CURLOPT_COPYPOSTFIELDS overwrites the former.
_ CURLOPT_POSTFIELDSIZE_LARGE may be used instead of
CURLOPT_POSTFIELDSIZE for CURLOPT_COPYPOSTFIELDS providing the given
size fits in a size_t.

Jolly good chaps!