Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl formpost accesses libcurl private data #3532

Closed
bagder opened this issue Feb 6, 2019 · 10 comments
Closed

curl formpost accesses libcurl private data #3532

bagder opened this issue Feb 6, 2019 · 10 comments

Comments

@bagder
Copy link
Member

bagder commented Feb 6, 2019

I did this

I noticed this code for the command line tool:

#include "mime.h"

It includes a private libcurl header and uses that to access the contents of a struct without the struct being in the public libcurl API.

This makes curl depend on and use things that aren't protected by API and ABI conventions and restrictions. This is fragile and wrong. This mistake was added in commit fec7a85, used since curl 7.56.0.

(Neither the mime.h and the strcase.h headers should be included by command line tool code.)

I expected the following

I should be able to change that struct within libcurl without breaking existing curl builds since the layout of it is not provided or guaranteed in the libcurl API/ABI.

curl/libcurl version

7.64.0 and git master

operating system

All

/ cc @monnerat

@MarcelRaad
Copy link
Member

MarcelRaad commented Feb 6, 2019

Then the two lib include paths should probably be removed here?

AM_CPPFLAGS = -I$(top_srcdir)/include \

Edit: Ah no, some files are actually used on purpose :-(

@bagder
Copy link
Member Author

bagder commented Feb 6, 2019

some files are actually used on purpose

Yes, that's what makes this slightly complicated. We can of course stop that practice, to the price of code duplication. Or we have to think of another way to make sure we only include the blessed headers...

bagder added a commit that referenced this issue Feb 6, 2019
The mime.h (and strcase.h) include files are not meant to be included by
the command line tool.

Fixes #3532
@monnerat
Copy link
Contributor

monnerat commented Feb 6, 2019

I'm afraid to say you gave me the bad example: strcase.h inclusion is from you ! (811a693). I think it can be dropped nowadays.

There is no public API to get the parent mime, thus it was simpler to implement it that way.
Other comments follow in your PR.

@bagder
Copy link
Member Author

bagder commented Feb 6, 2019

strcase.h is actually OK, I was wrong about it above (although I still think we should make only curlx.h be allowed to be included directly).

As I mentioned in the PR, the problem is unfortunately even bigger than I first noticed since tool_setopt.c is also using internal information from mime.h...

@bagder
Copy link
Member Author

bagder commented Feb 6, 2019

Try with this patch that explicitly makes mime.h unavailable for the command line tool:

diff --git a/lib/mime.h b/lib/mime.h
index 4d5c70404..7f9e8dde3 100644
--- a/lib/mime.h
+++ b/lib/mime.h
@@ -1,6 +1,6 @@
-#ifndef HEADER_CURL_MIME_H
+#if !defined(HEADER_CURL_MIME_H) && defined(BUILDING_LIBCURL)
 #define HEADER_CURL_MIME_H
 /***************************************************************************
  *                                  _   _ ____  _
  *  Project                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |

bagder added a commit that referenced this issue Feb 6, 2019
The mime.h include file MUST NOT be included by the command line tool
code.

Fixes #3532
@monnerat
Copy link
Contributor

monnerat commented Feb 7, 2019

the problem is unfortunately even bigger than I first noticed since tool_setopt.c is also using internal information from mime.h...

And there is even more references to the structure fields since we have no public getters for them :-(

Unfortunately, I see no other solution than to mimic the private mime structure into an intermediate structure in the cli tool... if we want to avoid defining some public getter function.

@bagder
Copy link
Member Author

bagder commented Feb 7, 2019

I see no other solution than to mimic the private mime structure into an intermediate structure in the cli tool... if we want to avoid defining some public getter function.

I agree.

The thing about a public getter function is that we've had zero requests or interest from others for such functions so it feels something with rather low demand. But I'm also not totally against it if we really can't solve our problem "decently" without it.

What do you think, how painful/complicated would fixing this issue be without new API functions?

@monnerat
Copy link
Contributor

monnerat commented Feb 7, 2019

The thing about a public getter function is that we've had zero requests or interest from others for such functions

This is quite normal: purpose of such functions is not targetting the file transfer itself, but extending the API, like a C++ subclass. If we were using this language, I would declare these methods as protected rather than public or private. the cli tool is probably the only piece of software that needs extra functionalities around our private structures. (brainstorming idea: introduce protected-like API, maybe not man-documented, with special naming like cURL_* not intended to be called by third party sw, for the cli tool: this may even simplify future cli tool features development... unless this is what curlx_* already is)

What do you think, how painful/complicated would fixing this issue be without new API functions?

This is not complicated, but adds a lot of redundant code without featuring anything new. I can do it, but it will take some time.

@bagder
Copy link
Member Author

bagder commented Feb 8, 2019

I would declare these methods as protected rather than public or private

Well, an API is just one level of public. It's there or not. And making a public API just for the command line tool and not for other users feels wrong on several levels.

not intended to be called by third party sw, for the cli tool: this may even simplify future cli tool features development... unless this is what curlx_* already is)

curlx_ is for code reuse from the library into the command line tool, not exactly using libcurl - those source codes are built separately for the command line tool and linked into the binary. But it saves us from having the code in two places.

The command line tool has no special magic powers over libcurl and can and should only use libcurl just like any other application can. Someone should be able to write a "curl2" application and it of course should be able to do exactly everything curl can.

This is not complicated, but adds a lot of redundant code

Okay, so let's have a look. What's your suggested additional API we could/should add to help facilitate this in the command line tool without "a lot of redundant code"?

@monnerat
Copy link
Contributor

monnerat commented Feb 8, 2019

The command line tool has no special magic powers over libcurl and can and should only use libcurl just like any other application can. Someone should be able to write a "curl2" application and it of course should be able to do exactly everything curl can.

Sure: everything is possible.
As long as you concentrate on libcurl goal which is file transfer, it's lovely. As soon as you want to manipulate the API-gathered data outside libcurl, it is awful because almost every attribute is "write-only". I know there are not many people asking for reading back attributes and there will never have a large demand for it since people want to transfer files, not extend the library (I consider the --libcurl option as an extension since it is not featured by libcurl).
I understand libcurl has not been designed with that stuff in mind and I'm not asking for a revolution on this point.

What's your suggested additional API we could/should add to help facilitate this in the command line tool without "a lot of redundant code"?

I've already spent all my time since yesterday in implementing an intermediate structure: I'm still coding and as I told yesterday it will take me some additional days... i prefer to concentrate on it rather than defining a new API now.

@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

3 participants