Bump hiredis to v1.2.0 #40217

thomasdesr · 2023-10-10T02:11:38Z

Why are these changes needed?

Bump to latest to fix outstanding vulnerabilities

Related issue number

Closes #40279

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

rkooo567 · 2023-10-11T00:00:52Z

Hmm actually all the windows build seem to fail?

mattip · 2023-10-11T12:52:39Z

The windows error starts with

external/com_github_redis_hiredis/alloc.c(40): error C2065: 'strdup': undeclared identifier

Does that version work with MSVC?

bazel/ray_deps_setup.bzl

mattip · 2023-10-13T04:36:27Z

This diff will fix the failure in alloc.c. There is still a failure to build boringssl. I don't really understand a few things:

what changed, why does master build boringssl and this version does not?
why use boringssl for hiredis, and openssl for redis? It seems openssl is more prevalent, especially in the documentation.
If we are talking about security, why is openssl pinned to the very old openssl v1.1.1f when there are security releases, the final 1.1.1 version is 1.1.1w

Anyhow, here is what I have so far

diff --git a/thirdparty/patches/hiredis-windows-msvc.patch b/thirdparty/patches/hiredis-windows-msvc.patch
index 2d05b117c4..733cb75665 100644
--- a/thirdparty/patches/hiredis-windows-msvc.patch
+++ b/thirdparty/patches/hiredis-windows-msvc.patch
@@ -17,7 +17,7 @@ diff --git sds.h sds.h
 diff --git fmacros.h fmacros.h
 --- fmacros.h
 +++ fmacros.h
-@@ -12,0 +12,3 @@
+@@ -13,0 +13,3 @@
 +#if defined(_MSC_VER) && !defined(__clang__) && !defined(strdup)
 +#define strdup _strdup
 +#endif

rkooo567 · 2023-10-18T11:28:15Z

Seems like this is still failing. @iycheng do you know the answer for questions ^?

peytondmurray · 2023-11-09T18:14:21Z

@rkooo567 @iycheng It would be nice to keep this from falling through the cracks. @iycheng do you have any information about the questions above?

stale · 2023-12-15T03:59:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

Signed-off-by: Thomas Desrosiers <[email protected]>

Signed-off-by: mattip <[email protected]>

mattip · 2023-12-20T17:34:50Z

I rebased to see if by magic anything changed and boringssl now builds on windows.

rkooo567 · 2023-12-20T23:17:51Z

@mattip since we don't support ray ha in windows, the best way forward seems to just not include redis for windows build. Do you think it is possible to take this approach instead?

mattip · 2023-12-24T19:48:43Z

the best way forward seems to just not include redis for windows build

I see redis-client is a dependency of _raylet, gcs_pub_sub_lib, gcs_client_lib, gcs_table_storage_test_lib, gcs_table_storage_lib, chaos_redis_store_client_test. Will any of that work without redis?

mattip · 2024-01-03T13:11:03Z

Also see #42098

stephanie-wang · 2024-01-03T22:55:28Z

the best way forward seems to just not include redis for windows build

I see redis-client is a dependency of _raylet, gcs_pub_sub_lib, gcs_client_lib, gcs_table_storage_test_lib, gcs_table_storage_lib, chaos_redis_store_client_test. Will any of that work without redis?

Is it possible to include redis-client but not redis server?

mattip · 2024-01-07T08:39:38Z

Is it possible to include redis-client but not redis sever?

I don't think that would solve the current problem. The dependency is redis_client depends on hiredis, and hiredis does not build on windows due to a failure to build boringssl:

ERROR: C:/tmp/wqe5u2zk/external/com_github_redis_hiredis/BUILD.bazel:28:11: Compiling ssl.c failed: (Exit 2): cl.exe failed: error executing command
...
external/boringssl/src/include\openssl/base.h(299): error C2059: syntax error: '<parameter-list>'
external/boringssl/src/include\openssl/pkcs7.h(123): error C2059: syntax error: '<parameter-list>'
external/boringssl/src/include\openssl/x509.h(186): error C2059: syntax error: '<parameter-list>'
external/boringssl/src/include\openssl/x509.h(189): error C2059: syntax error: '<parameter-list>'
external/boringssl/src/include\openssl/x509.h(325): error C2059: syntax error: '('

rkooo567 · 2024-01-16T05:17:30Z

@mattip everything will work without redis actually. It is only used when GCS HA is enabled (which is not supported in windows anyway).

rkooo567 · 2024-01-16T05:18:09Z

is it possible to just make it build mocked version if it is used for Windows?

mattip · 2024-01-16T14:16:00Z

It is only used when GCS HA

What is the name of GCS HA in bazel terms? In other words: where is this non-windows-only dependency expressed in the bazel build?

mattip · 2024-01-16T14:31:03Z

is it possible to just make it build mocked version if it is used for Windows?

If redis_client is never needed for windows, it would be better to redo BUILD.bazel to include it only when building on non-windows. Is there a way to express this in bazel?

rkooo567 · 2024-01-16T21:45:59Z

ray/src/ray/gcs/gcs_server/gcs_server.cc

Line 76 in eb19da2

case StorageType::IN_MEMORY:

I don't think there's a way to express this in bazel build now, but redis is used only when this condition is met (and in Windows, you always go to IN_MEMORY).

rkooo567 · 2024-01-16T21:47:43Z

Not sure if there's a good way to just not include redis in this case. I wonder if we don't import any redis related dependency if the platform is windows within code?

mattip · 2024-01-16T21:56:36Z

Not sure if there's a good way to just not include redis in this case

There should be a way to tell bazel "do not add redis_client to the dependencies" in BUILD.bazel, would something like this work?

deps = select({
        "@bazel_tools//src/conditions:windows": [
            <everything but redis_client>
        ],
        "//conditions:default": [
            <everything including redis_client>
        ],
    }),

rkooo567 · 2024-01-16T22:13:49Z

I think that could work, but we also need #ifndef _WIN32 within code where redis client is imported and used (I believe gcs_server.cc).

rkooo567 · 2024-01-18T08:49:28Z

@mattip ^ Does it make sense? We can also discuss in person to quickly find a solution.

mattip · 2024-01-18T09:08:43Z

we also need #ifndef _WIN32 within code where redis client is imported

Yes, makes perfect sense. I will try to push that into this PR.

mattip · 2024-01-26T06:58:20Z

I am trying to untangle this. I have excluded redis_client, but that is not enough. I need to exclude the actual redis sources from gcs:

diff --git a/BUILD.bazel b/BUILD.bazel
index c1453b0037..f575eac8d0 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -2268,13 +2285,24 @@ ray_cc_library(
         ],
         exclude = [
             "src/ray/gcs/*_test.cc",
+            # These are conditionally included in ray_client
+            "src/ray/gcs/asio.cc",
+            "src/ray/gcs/redis_async_context.cc",
+            "src/ray/gcs/redis_client.cc",
+            "src/ray/gcs/redis_context.cc",
         ],
     ),
     hdrs = glob([
-        "src/ray/gcs/*.h",
-    ]),
+            "src/ray/gcs/*.h",
+        ],
+        exclude = [
+            "src/ray/gcs/asio.h",
+            "src/ray/gcs/redis_async_context.h",
+            "src/ray/gcs/redis_client.h",
+            "src/ray/gcs/redis_context.h",
+        ],
+    ),
     deps = [
-        ":hiredis",
         ":node_manager_fbs",
         ":node_manager_rpc",
         ":ray_common",
@@ -2284,7 +2312,10 @@ ray_cc_library(
         "//src/ray/protobuf:gcs_service_cc_proto",
         "//src/ray/util",
         "@boost//:asio",
-    ],
+    ] + select({
+        "@bazel_tools//src/conditions:windows": [],
+        "//conditions:default": [":hiredis", ":redis_client"],
+    }),
 )

 ray_cc_test(

but then various parts of the gcs table storage do not work. Is there someone who understands the interactions between redis and the gcs table storage who can give me some guidance?

rkooo567 · 2024-01-31T07:05:24Z

cc @iycheng can you make some comments on ^?

fishbone · 2024-02-07T02:09:39Z

@mattip I think exclude hiredis building from windows actually needs more work. let me take a look at the building failure first.

fishbone · 2024-02-07T02:53:01Z

This diff will fix the failure in alloc.c. There is still a failure to build boringssl. I don't really understand a few things:

what changed, why does master build boringssl and this version does not?

why use boringssl for hiredis, and openssl for redis? It seems openssl is more prevalent, especially in the documentation.

If we are talking about security, why is openssl pinned to the very old openssl v1.1.1f when there are security releases, the final 1.1.1 version is 1.1.1w

Sorry, I don't know the answer
I always think the dependence in bazel is a messy thing. Like grpc depends implicitly on boringssl and openssl is required for some other libs. Also it seems super hard to make things compiled successfully in bazel.

…dis-version

mattip · 2024-02-07T13:52:09Z

Getting closer. Now everything compiles, but there is a linking error:

hiredis.lib(ssl.obj) : error LNK2019: unresolved external symbol __imp_CertCloseStore referenced in function redisCreateSSLContextWithOptions
hiredis.lib(ssl.obj) : error LNK2019: unresolved external symbol __imp_CertEnumCertificatesInStore referenced in function redisCreateSSLContextWithOptions
hiredis.lib(ssl.obj) : error LNK2019: unresolved external symbol __imp_CertFreeCertificateContext referenced in function redisCreateSSLContextWithOptions
hiredis.lib(ssl.obj) : error LNK2019: unresolved external symbol __imp_CertOpenSystemStoreA referenced in function redisCreateSSLContextWithOptions

stack overflow thinks the link is missing Windows' crypt32.lib library. This would be the link command for hiredis.

Signed-off-by: mattip <[email protected]>

mattip · 2024-02-07T17:25:35Z

DCO run is failing with

Commit sha: b31089f, Author: Yi Cheng, Committer: Yi Cheng; The sign-off is missing.

Signed-off-by: mattip <[email protected]>

mattip · 2024-02-07T19:45:04Z

Ahh, additional_linker_inputs is not available in this version of bazel, I changed it to linkopts.

fishbone · 2024-02-07T23:47:03Z

all passed. merging it.

* Bump hiredis to v1.2.0 --------- Signed-off-by: Thomas Desrosiers <[email protected]> Signed-off-by: mattip <[email protected]> Co-authored-by: Matti Picus <[email protected]> Co-authored-by: Yi Cheng <[email protected]> Co-authored-by: Yi Cheng <[email protected]> Signed-off-by: Ratnopam Chakrabarti <[email protected]>

* Bump hiredis to v1.2.0 --------- Signed-off-by: Thomas Desrosiers <[email protected]> Signed-off-by: mattip <[email protected]> Co-authored-by: Matti Picus <[email protected]> Co-authored-by: Yi Cheng <[email protected]> Co-authored-by: Yi Cheng <[email protected]> Signed-off-by: tterrysun <[email protected]>

rkooo567 approved these changes Oct 10, 2023

View reviewed changes

thomasdesr force-pushed the thomas/bump-hiredis-version branch from b7e4c44 to 79c14fa Compare October 11, 2023 00:13

mattip reviewed Oct 11, 2023

View reviewed changes

bazel/ray_deps_setup.bzl Show resolved Hide resolved

rkooo567 mentioned this pull request Oct 11, 2023

[Core] Shepherding Security vulerability issue from hiredis dependency #40279

Closed

mattip force-pushed the thomas/bump-hiredis-version branch 2 times, most recently from e9885b6 to ac1dc5c Compare October 13, 2023 05:52

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Dec 15, 2023

thomasdesr and others added 2 commits December 20, 2023 19:31

Bump hiredis to v1.2.0

f256d4c

Signed-off-by: Thomas Desrosiers <[email protected]>

update patch

0fb56f9

Signed-off-by: mattip <[email protected]>

mattip force-pushed the thomas/bump-hiredis-version branch from ac1dc5c to 0fb56f9 Compare December 20, 2023 17:33

stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Dec 20, 2023

Merge branch 'master' into thomas/bump-hiredis-version

526e9c4

Yi Cheng added 2 commits February 7, 2024 05:55

fix

b31089f

:Merge remote-tracking branch 'upstream/master' into thomas/bump-hire…

b41b3d7

…dis-version

link hiredis with Crypt32.lib on windows

3a33abd

Signed-off-by: mattip <[email protected]>

mattip force-pushed the thomas/bump-hiredis-version branch from 0550483 to 3a33abd Compare February 7, 2024 17:22

use bazel 5.4.1 syntax

14e3d32

Signed-off-by: mattip <[email protected]>

fishbone merged commit 7fb3feb into ray-project:master Feb 7, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump hiredis to v1.2.0 #40217

Bump hiredis to v1.2.0 #40217

thomasdesr commented Oct 10, 2023 •

edited by rkooo567

Loading

rkooo567 commented Oct 11, 2023

mattip commented Oct 11, 2023

mattip commented Oct 13, 2023

rkooo567 commented Oct 18, 2023

peytondmurray commented Nov 9, 2023

stale bot commented Dec 15, 2023

mattip commented Dec 20, 2023

rkooo567 commented Dec 20, 2023

mattip commented Dec 24, 2023 •

edited

Loading

mattip commented Jan 3, 2024

stephanie-wang commented Jan 3, 2024

mattip commented Jan 7, 2024

rkooo567 commented Jan 16, 2024 •

edited

Loading

rkooo567 commented Jan 16, 2024

mattip commented Jan 16, 2024

mattip commented Jan 16, 2024

rkooo567 commented Jan 16, 2024

rkooo567 commented Jan 16, 2024

mattip commented Jan 16, 2024

rkooo567 commented Jan 16, 2024 •

edited

Loading

rkooo567 commented Jan 18, 2024

mattip commented Jan 18, 2024

mattip commented Jan 26, 2024

rkooo567 commented Jan 31, 2024

fishbone commented Feb 7, 2024

fishbone commented Feb 7, 2024

mattip commented Feb 7, 2024

mattip commented Feb 7, 2024

mattip commented Feb 7, 2024

fishbone commented Feb 7, 2024

Bump hiredis to v1.2.0 #40217

Bump hiredis to v1.2.0 #40217

Conversation

thomasdesr commented Oct 10, 2023 • edited by rkooo567 Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 commented Oct 11, 2023

mattip commented Oct 11, 2023

mattip commented Oct 13, 2023

rkooo567 commented Oct 18, 2023

peytondmurray commented Nov 9, 2023

stale bot commented Dec 15, 2023

mattip commented Dec 20, 2023

rkooo567 commented Dec 20, 2023

mattip commented Dec 24, 2023 • edited Loading

mattip commented Jan 3, 2024

stephanie-wang commented Jan 3, 2024

mattip commented Jan 7, 2024

rkooo567 commented Jan 16, 2024 • edited Loading

rkooo567 commented Jan 16, 2024

mattip commented Jan 16, 2024

mattip commented Jan 16, 2024

rkooo567 commented Jan 16, 2024

rkooo567 commented Jan 16, 2024

mattip commented Jan 16, 2024

rkooo567 commented Jan 16, 2024 • edited Loading

rkooo567 commented Jan 18, 2024

mattip commented Jan 18, 2024

mattip commented Jan 26, 2024

rkooo567 commented Jan 31, 2024

fishbone commented Feb 7, 2024

fishbone commented Feb 7, 2024

mattip commented Feb 7, 2024

mattip commented Feb 7, 2024

mattip commented Feb 7, 2024

fishbone commented Feb 7, 2024

thomasdesr commented Oct 10, 2023 •

edited by rkooo567

Loading

mattip commented Dec 24, 2023 •

edited

Loading

rkooo567 commented Jan 16, 2024 •

edited

Loading

rkooo567 commented Jan 16, 2024 •

edited

Loading