You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jcferretti (via GitHub)" <gi...@apache.org> on 2024/06/02 18:19:46 UTC
[PR] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 [arrow]
jcferretti opened a new pull request, #41927:
URL: https://github.com/apache/arrow/pull/41927
### 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2143977023
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2143978058
:warning: GitHub issue #41919 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2145113816
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers [arrow]
Posted by "jcferretti (via GitHub)" <gi...@apache.org>.
jcferretti commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2145266611
> 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 believe that is incorrect. I believe you are applying the context for that stack overflow answer wrong here. The `shared_ptr` ensures the integrity of the count and its /own/ state. The shared_ptr does not ensure that accessing the pointer payload or /its/ operations are done in a thread safe manner. Here we need the first, not the second. When a call to `FlightClient.authenticate` ends up replacing the `auth_handler_` data member while SetToken is executing, the end result is that the shared_ptr in SetToken has its count reduced, atomically and consistently, and the `auth_handler_` data member shared_ptr is replaced with a new object.
I believe your concern would be valid if our problem was preventing concurrent access to the object pointed to; it is not.
I completely agree that the documentation should be improved here; had it been improved after the first time this issue was reported, that would have saved my team a considerable amount of effort debugging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers. Fixes #41919, #38565 [arrow]
Posted by "jcferretti (via GitHub)" <gi...@apache.org>.
jcferretti commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2143981936
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers [arrow]
Posted by "jcferretti (via GitHub)" <gi...@apache.org>.
jcferretti commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2145285930
Closing, since this is the wrong fix:
https://comp.lang.cpp.narkive.com/4JTZFIE0/std-shared-ptr-thread-safety
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers [arrow]
Posted by "jcferretti (via GitHub)" <gi...@apache.org>.
jcferretti commented on PR #41927:
URL: https://github.com/apache/arrow/pull/41927#issuecomment-2144127326
> Could you adjust style by `pre-commit run -a`?
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers [arrow]
Posted by "jcferretti (via GitHub)" <gi...@apache.org>.
jcferretti closed pull request #41927: GH-41919: [C++][FlightRPC] Avoid using raw pointers for auth handlers
URL: https://github.com/apache/arrow/pull/41927
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org