#821 Setting CURLOPT_NOBODY=0 resets httpreq to HTTPREQ_GET

closed-fixed
libcurl (356)
6
2013-06-21
2009-04-03
Martin Storsjö
No

If setting CURLOPT_NOBODY = 0, the httpreq internal state is reset to HTTPREQ_GET, even though it actually should be something else.

One possible scenario is if the user sets options in this order (it's perfectly reasonable to reset the _NOBODY setting if the curl handle has been used for other things earlier):

curl_easy_setopt(slot->curl, CURLOPT_PUT, 1);
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);

The first setopt call sets httpreq to HTTPREQ_PUT in lib/url.c:914 (in 7.19.4), but the second call resets httpreq to HTTPREQ_GET in lib/url.c:896.

One workaround is simply to do all reset/disable settings before setting the new options.

Is this a bug, or should the documentation be updated to reflect this?

A sample patch is attached, which solves this particular case but doesn't handle all possible cases. One case it doesn't handle is setting CURLOPT_POST=1, then CURLOPT_NOBODY=0, which would require even more state to be saved or better handling in url.c. Or what about setting e.g. CURLOPT_POST=1, CUROPT_NOBODY=1, CURLOPT_NOBODY=0 - should such scenarios be supported?

The API makes it look like all these options modify distinct separate flags, even though they all modify one shared variable (and only some of them are stored as flags).

Discussion

1 2 > >> (Page 1 of 2)
  • A sample patch, fixing some (but not all possible) cases

     
    Attachments
  • Hm, yes I agree that the current behavior is a bit annoying and disturbing and we should do something about it.

    The main reason NOBODY=0 sets the request type is that because previously when someone did NOBODY=1 and then did NOBODY=0 on the same handle, it would do mostly a GET but with a HEAD method which was completely wrong...

    I'm now thinking that perhaps the fix is rather to if NOBODY is set to 0, it should only change the method (to GET) if it is at that time set to HEAD. If another method has been selected, it should just let that remain. That would make your little snippet work and it "feel right" to me.

    Or what to you think?

     
  • Yes, that's also a solution that would solve this particular case.

    It would still not solve the generic API issue; the way the API is written, one may believe that PUT=1, NOBODY=1, NOBODY=0 would yield the same result as just PUT=1, even though NOBODY=1 sets httpreq to HEAD, and NOBODY=0 sets it to GET. (I think the attached patch would get this one right, but probably not all corner cases.) Nothing in the API docs hints that the order of setting the options would matter (except for setting the same option to different values, where the last value should be the one that is used, of course).

    The way the API is designed, the best way would probably be to store all these settings as simple flags, without touching httpreq while they're set. When the request actually is started, the flags would be evaluated and the best matching httpreq would be selected. (This allows the user to set conflicting sets of flags, though; what should happen if the user has set HTTPGET=1, PUT=1, NOBODY=1?) In this way, the order different options are set doesn't matter at all, the only thing that matters is the last value that has been set to each option.

    This would, unfortunately, require a bit larger rewrite than just simply fixing this particular case I stumbled upon.

     
  • Thank you for your clear thoughts. You're of course right.

    I'll work on a somewhat larger fix and submit it here for further comments. It might just take me a day or two.

     
    • priority: 5 --> 6
     
  • Actually, doing it "the big change" will risk altering behavior as well and thus it might hurt existing applications.

    I think we should perhaps take this to the mailing list to see what people think, but the most conservative thing to do is possibly to just clearly and more accurately document the current behavior...

     
  • Yeah, a proper fix probably is a bit larger change - that's why I didn't send any readymade patch for that ;-)

    Documentation of the current behaviour, and perhaps minor fixes along the lines of my original patch is of course a good solution meanwhile.

     
  • attempt at changing how NOBODY=0 behaves

     
    Attachments
  • Right, so attached here is now my smaller take at basically not letting NOBODY set the http request at all until perform() is about to run. This allows for POST=1 NOBODY=0 without the latter ruining the post.

    I didn't actually try the code (only compiled/built) so there might be a blatant error in there.

     
  • I didn't do any major testing, but the patch you attached seems to fix my issue at least, and doesn't break anything else in my small test round...

     
1 2 > >> (Page 1 of 2)