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

BIO_f_zlib: Properly handle BIO_CTRL_PENDING and BIO_CTRL_WPENDING ca… #9877

Closed
wants to merge 8 commits into from

Conversation

@t8m
Copy link
Member

t8m commented Sep 12, 2019

…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes #9866

Checklist
  • documentation is added or updated
  • tests are added or updated
…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes #9866
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
crypto/comp/c_zlib.c Outdated Show resolved Hide resolved
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

May I suggest cherry-picking 93227e5?

Update: oh never mind, you did already

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

All should be done now. Also added the temporary commit to enable zlib build in Travis.

Copy link
Member

levitte left a comment

Make sure the CIs agree, and remember to remove the temporary commit

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

zlib job is failing :-/

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 12, 2019

zlib job is failing :-/

Build failure seems to be unrelated: A weird compiler error in apps/lib/apps.c.

https://travis-ci.org/openssl/openssl/jobs/584077578

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 12, 2019

Where does this crazy macro expansion of strcmp(...) come from? Is there some zlib header interfering?

https://travis-ci.org/openssl/openssl/jobs/584077578#L602

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

Let's try with gcc.

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

BTW, there is probably also missing BIO_CTRL_WPENDING handling on BIO_f_asn1() but that is an experimental BIO filter so I do not know if it is worth fixing it.

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

So with gcc the zlib build passed.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

Cool. I hope the weird failure is a temporary travis glitch. My approval stands

@t8m t8m added the approval: done label Sep 12, 2019
@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 12, 2019

the weird failure is a temporary travis glitch.

My gut feelings tell me that it is not a glitch and we should better find out what causes the problem. (Wich doesn't mean I question the approval.)

This looks like a handcrafted strcmp() implementation, inlined by macro. My wild guess is that 'apps/lib/apps.c' gets to see some zlib header file which it better shouldn't. In particular, since there seems to be a problem with the macro expansion, which is caught by -Warray-bounds.

apps/lib/apps.c:2369:2014: error: array index 2 is past the end of the array (which contains 2 elements) [-Werror,-Warray-bounds]
    if (!private || filename == ((void*)0) || __extension__ ({ size_t __s1_len, __s2_len; (__builtin_constant_p (filename) && __builtin_constant_p ("-") && (__s1_len = __builtin_strlen (filename), __s2_len = __builtin_strlen ("-"), (!((size_t)(const void *)((filename) + 1) - (size_t)(const void *)(filename) == 1) || __s1_len >= 4) && (!((size_t)(const void *)(("-") + 1) - (size_t)(const void *)("-") == 1) || __s2_len >= 4)) ? __builtin_strcmp (filename, "-") : (__builtin_constant_p (filename) && ((size_t)(const void *)((filename) + 1) - (size_t)(const void *)(filename) == 1) && (__s1_len = __builtin_strlen (filename), __s1_len < 4) ? (__builtin_constant_p ("-") && ((size_t)(const void *)(("-") + 1) - (size_t)(const void *)("-") == 1) ? __builtin_strcmp (filename, "-") : (__extension__ ({ const unsigned char *__s2 = (const unsigned char *) (const char *) ("-"); int __result = (((const unsigned char *) (const char *) (filename))[0] - __s2[0]); if (__s1_len > 0 && __result == 0) { __result = (((const unsigned char *) (const char *) (filename))[1] - __s2[1]); if (__s1_len > 1 && __result == 0) { __result = (((const unsigned char *) (const char *) (filename))[2] - __s2[2]); if (__s1_len > 2 && __result == 0) __result = (((const unsigned char *) (const char *) (filename))[3] - __s2[3]); } } __result; }))) : (__builtin_constant_p ("-") && ((size_t)(const void *)(("-") + 1) - (size_t)(const void *)("-") == 1) && (__s2_len = __builtin_strlen ("-"), __s2_len < 4) ? (__builtin_constant_p (filename) && ((size_t)(const void *)((filename) + 1) - (size_t)(const void *)(filename) == 1) ? __builtin_strcmp (filename, "-") : (- (__extension__ ({ const unsigned char *__s2 = (const unsigned char *) (const char *) (filename); int __result = (((const unsigned char *) (const char *) ("-"))[0] - __s2[0]); if (__s2_len > 0 && __result == 0) { __result = (((const unsigned char *) (const char *) ("-"))[1] - __s2[1]); if (__s2_len > 1 && __result == 0) { __result = (((const unsigned char *) (const char *) ("-"))[2] - __s2[2]); if (__s2_len > 2 && __result == 0) __result = (((const unsigned char *) (const char *) ("-"))[3] - __s2[3]); } } __result; })))) : __builtin_strcmp (filename, "-")))); }) == 0)
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

My wild guess is that 'apps/lib/apps.c' gets to see some zlib header file which it better shouldn't.

That sure is a wild guess. Why have we never seen this before? If your wild guess is correct, this should happen with current master as well... Try it.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 12, 2019

Because we never build with zlib on the CIs?

I can do some investigations.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

I can do some investigations.

Please do

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

It really looks like some weird glitch - this macro comes from old glibc headers bits/string2.h - not sure how it got to the travis build. I would be very surprised if it was included (copied into) the zlib headers.

levitte pushed a commit that referenced this pull request Sep 12, 2019
…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes #9866

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9877)
levitte pushed a commit that referenced this pull request Sep 12, 2019
…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes #9866

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9877)

(cherry picked from commit 6beb8b3)
@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

Merged to master and 1.1.1.

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

Just playing with the environment to see where the strcmp macro comes from.

@petrovr

This comment has been minimized.

Copy link

petrovr commented Sep 12, 2019

On my system OpenSSL always is build with zlib-dynamic.
Failed test_enc/171in "1.1.1 stable" pass(!) in "master". Definitely master pass all tests - #9524 (comment) ( 12 days ago ).
So i cannot see direct relation with continuous integration system.
Something different "fixes" :) or "hides" :( issue.

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

Hmm, the glibc which has this code is not that much old - 2.23 (which is the one on debian/ubuntu travis builds use) has this code. The question however is why this errors out only in the zlib build and not in other builds compiled by clang.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 12, 2019

BTW, I also wonder why the error display is so darn broken... make -s doesn't do its job right...

@t8m

This comment has been minimized.

Copy link
Member Author

t8m commented Sep 12, 2019

Actually the -D__NO_STRING_INLINES is what suppresses this warning/error in other clang builds. So case closed - this is just glibc/clang incompatibility without -D__NO_STRING_INLINES.

@t8m t8m closed this Sep 12, 2019
kiyolee added a commit to kiyolee/openssl that referenced this pull request Jan 18, 2020
…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes openssl#9866

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#9877)

(cherry picked from commit 6beb8b3)
kiyolee added a commit to kiyolee/openssl that referenced this pull request Jan 18, 2020
…lls.

There can be data to write in output buffer and data to read that were
not yet read in the input stream.

Fixes openssl#9866

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#9877)

(cherry picked from commit 6beb8b3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.