curl formpost accesses libcurl private data

[bagder]

I did this

I noticed this code for the command line tool:

curl/src/tool_formparse.c

Line 24 in fef38a0

#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]

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

curl/src/Makefile.am

Line 38 in 179311e

AM_CPPFLAGS = -I$(top_srcdir)/include \

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

[bagder]

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...

[monnerat]

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]

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]

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                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |

[monnerat]

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]

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]

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]

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]

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.