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

GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers #41927

Closed
wants to merge 2 commits into from

Conversation

jcferretti
Copy link

@jcferretti jcferretti commented Jun 2, 2024

Rationale for this change

Avoid pointer access after deletion bug explained in #41919

What changes are included in this PR?

Change SetToken argument from a raw pointer to an std::shared_ptr.

Are these changes tested?

This PR leans on existing CI tests and compile-time type checking, no additional/specific testing was performed.

Are there any user-facing changes?

No

@jcferretti jcferretti requested a review from lidavidm as a code owner June 2, 2024 18:19
Copy link

github-actions bot commented Jun 2, 2024

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@jcferretti jcferretti changed the title Avoid using raw pointers for auth handlers. Fixes #41919, #38565 GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 Jun 2, 2024
Copy link

github-actions bot commented Jun 2, 2024

⚠️ GitHub issue #41919 has been automatically assigned in GitHub to PR creator.

@jcferretti
Copy link
Author

The AppVeyor github check failure seems unrelated to change:

error    libmamba Failed to download package from https://conda.anaconda.org/conda-forge/nomkl-1.0-h5ca1d4c_0.tar.bz2 (status 404)
Multi-download failed. Reason: Transfer finalized, status: 404 [https://conda.anaconda.org/conda-forge/nomkl-1.0-h5ca1d4c_0.tar.bz2] 568 bytes
# >>>>>>>>>>>>>>>>>>>>>> ERROR REPORT <<<<<<<<<<<<<<<<<<<<<<
    Traceback (most recent call last):
      File "C:\Miniconda38-x64\lib\site-packages\conda\exception_handler.py", line 18, in __call__
        return func(*args, **kwargs)
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 959, in exception_converter
        raise e
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 952, in exception_converter
        exit_code = _wrapped_main(*args, **kwargs)
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 898, in _wrapped_main
        result = do_call(parsed_args, p)
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 767, in do_call
        exit_code = create(args, parser)
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 602, in create
        return install(args, parser, "create")
      File "C:\Miniconda38-x64\lib\site-packages\mamba\mamba.py", line 558, in install
        transaction.fetch_extract_packages()
    RuntimeError: Multi-download failed. Reason: Transfer finalized, status: 404 [https://conda.anaconda.org/conda-forge/nomkl-1.0-h5ca1d4c_0.tar.bz2] 568 bytes

@kou kou changed the title GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers Jun 2, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Could you adjust style by pre-commit run -a?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jun 2, 2024
@jcferretti
Copy link
Author

Could you adjust style by pre-commit run -a?

Done.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lidavidm
Copy link
Member

lidavidm commented Jun 3, 2024

That said, this doesn't seem like a guaranteed fix. https://stackoverflow.com/a/47117985/262727

Mutating the shared_ptr while another thread is copying it is still thread-unsafe, just perhaps a little less likely to cause problems than before. I'd still want to at least document the API as thread-unsafe while perhaps allowing this PR to avoid some basic errors

@jcferretti
Copy link
Author

jcferretti commented Jun 3, 2024

That said, this doesn't seem like a guaranteed fix. https://stackoverflow.com/a/47117985/262727

Mutating the shared_ptr while another thread is copying it is still thread-unsafe

I didn't think this was a problem but it is. This fix is wrong.
https://comp.lang.cpp.narkive.com/4JTZFIE0/std-shared-ptr-thread-safety

@jcferretti
Copy link
Author

Closing, since this is the wrong fix:
https://comp.lang.cpp.narkive.com/4JTZFIE0/std-shared-ptr-thread-safety

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants