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

AmiSSL support #3677

Closed
wants to merge 14 commits into from

Conversation

Projects
None yet
3 participants
@chris-y
Copy link
Contributor

commented Mar 13, 2019

This adds a configure option --with-amissl which selects AmiSSL as the SSL engine.
AmiSSL is an Amiga native library which provides a wrapper over OpenSSL.
It also requires all programs using it to use bsdsocket.library directly, rather than accessing socket functions through clib, which libcurl was not necessarily doing previously. Configure will now check for the headers and ensure they are included if found.

chris-y added some commits Mar 8, 2019

Fix linker errors with AmiSSL 68k build.
Because of the way 68k Amiga functions are called, assigning a library function to a variable does not work correctly, so we create a dummy function to workaround this problem.
This also affects the calling of MD5 functions.
This only affects Amiga OS3 libraries, not the new OS4 library interface, but the workaround is harmless there.
AmigaOS 3 library functions can't be assigned to variables, so use th…
…e internal Curl MD5 functions instead if we're using AmiSSL.
ixemul needs to die
More specifically, even though GCC is built without any trace of ixemul, the defines are still present and everything breaks.
Check for presence of proto/bsdsocket.h
Require this header in order to enable AmiSSL support
If it is available, use it. This will bypass any C lib interface.
Note that the code in amigaos.c which opens/closes bsdsocket.library should not really be used - this should be opened in user code.  It has been left in for compatibility but probably should be removed.
@bagder

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

It seems to be lacking some #ifdefs or macro definitions:

easy.c: In function ‘curl_global_cleanup’:
easy.c:276:3: error: implicit declaration of function ‘Curl_amiga_cleanup’; did you mean ‘Curl_digest_cleanup’? [-Werror=implicit-function-declaration]
   Curl_amiga_cleanup();
   ^~~~~~~~~~~~~~~~~~
   Curl_digest_cleanup
easy.c:276:3: error: nested extern declaration of ‘Curl_amiga_cleanup’ [-Werror=nested-externs]
@chris-y

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Oops, looks like I cocked up the #ifdefs by nesting them.
Should be something more like #if defined(__AMIGA__) && defined(HAVE_PROTO_BSDSOCKET_H) && !defined(USE_AMISSL)
I'll correct this later today.

AC_SUBST(HAVE_PROTO_BSDSOCKET_H, [1])],
[],
[]
)

This comment has been minimized.

Copy link
@bagder

bagder Mar 13, 2019

Member

And this header check can't be put among all the other headers that are checked?

This comment has been minimized.

Copy link
@chris-y

chris-y Mar 13, 2019

Author Contributor

I put it there as it needs to be resolved before any of the socket functions are checked, due to required added headers like the winsock check above it.
There's a case for moving it down to the gethostbyname section and making it part of the bsdsocket.library check there (I can't see any other socket lib function checks above this point). That would also confirm that functions from the library are working as expected which would be a useful additional test before it gets enabled.

This comment has been minimized.

Copy link
@chris-y

chris-y Mar 14, 2019

Author Contributor

I've now moved this to be part of the "gethostbyname is in bsdsocket.library" check.

@@ -248,7 +248,7 @@ Curl_ssl_connect(struct connectdata *conn, int sockindex)
conn->ssl[sockindex].use = TRUE;
conn->ssl[sockindex].state = ssl_connection_negotiating;

result = Curl_ssl->connect(conn, sockindex);
result = Curl_ssl->connect_blocking(conn, sockindex);

This comment has been minimized.

Copy link
@bagder

bagder Mar 13, 2019

Member

This isn't gcc's fault, this is some silly headers you use that seems to implement some of the socket API functions as macros and thus banning the function names from use everywhere. I'd say that is pretty bad style but I also realize you're not to blame for that...

This comment has been minimized.

Copy link
@chris-y

chris-y Mar 13, 2019

Author Contributor

Yes, it's the bsdsocket inline header. All Amiga 68k libraries are called using macros like that, the macros set the values in the correct registers then jump to the function code. Unfortunately not much I can do about that, it was a design decision taken ~35 years ago!

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

I would imagine that it should keep including the "mandatory" three last include files (in the same order) to reduce the risk of memory screwups when built debug-enabled. That's why test 1132 fails:

 ./../lib/amigaos.c doesn't include "memdebug.h"
 ./../lib/amigaos.c doesn't include "curl_memory.h"
SetComment(outs.filename, url);
if(strlen(urlnode->url) > 78)
urlnode->url[79] = '\0';
SetComment(outs.filename, urlnode->url);

This comment has been minimized.

Copy link
@bagder

bagder Mar 14, 2019

Member

Thinking of this code path: shouldn't this rather be done if the --xattr option is used, like it works for other platforms? That code path also scrubs off any username + password that might exist in the URL, while this amiga-specific block doesn't.

(No, this isn't really related to this particular change but it struck me now when I saw that this code is updated which means it is used...)

This comment has been minimized.

Copy link
@chris-y

chris-y Mar 14, 2019

Author Contributor

I agree, although I can't see where the code needs moving to, unless it is literally a few lines up under the /* Set file extended attributes */ comment?

This comment has been minimized.

Copy link
@bagder

bagder Mar 15, 2019

Member

That's what I had in mind, yes. But I'm sorry, that's a separate issue and should be done separately from this PR. I just happened to notice since you fixed that code now.

chris-y added some commits Mar 14, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

The red CI builds look like false positives.

@bagder bagder closed this in 1e85365 Mar 15, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Thanks! I did some edits and squashes before merge, but I hope I didn't ruin anything! =)

@lock lock bot locked as resolved and limited conversation to collaborators Jun 13, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.