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
AmiSSL support #3677
Conversation
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.
…e internal Curl MD5 functions instead if we're using AmiSSL.
…id mixing bsdsocket and C library functions
More specifically, even though GCC is built without any trace of ixemul, the defines are still present and everything breaks.
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.
It seems to be lacking some #ifdefs or macro definitions:
|
Oops, looks like I cocked up the |
configure.ac
Outdated
AC_SUBST(HAVE_PROTO_BSDSOCKET_H, [1])], | ||
[], | ||
[] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this header check can't be put among all the other headers that are checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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:
|
SetComment(outs.filename, url); | ||
if(strlen(urlnode->url) > 78) | ||
urlnode->url[79] = '\0'; | ||
SetComment(outs.filename, urlnode->url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The red CI builds look like false positives. |
Thanks! I did some edits and squashes before merge, but I hope I didn't ruin anything! =) |
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.