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.

Sign up for GitHub

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

Jump to bottom

SSLContext changes #4

Closed
wants to merge 2 commits into from
Closed

SSLContext changes #4

wants to merge 2 commits into from

Conversation

ssudhakaran
Copy link
Contributor

@gemmellr
Copy link
Member

I've had a look and have some comments:

  • The changes should inclulde some tests for the new feature.
  • The setter javadoc should make clear that this overrides any other configuration that might have otherwise been uesd to create a context, and the getter only returns anything set with the setter.
  • The changes introduce various lines with whitespace errors (lines with only spaces, mixtures of tabs and space indents). Please ensure you use spaces only, and empty lines are in fact empty.
  • The change in IoHandler to force EXTERNAL is unrelated and not required here, please remove it.

Some pointers for any updates and future PRs (info which I'll add to the website/repo's at some point soon, so folks can see it in advance, or we can reference it later):

  • If you raise the PR with the JIRA key in the title (e.g. see PROTON-1393: fully sever delivery refs during removal #3), a bot will raise a note against the JIRA for open/close and also relay future comments to the JIRA.
  • Similarly if you could include the key in the commit log message, that way a bot records it on the JIRA upon eventual commit.
  • In general PRs should be a single commit rebased to sit at the HEAD of the current master, without any merge commits. Multiple commits can be useful if its a many-layered change and the steps help evaluate it however.

@asfgit asfgit closed this in 4d9a89b Feb 17, 2017
@gemmellr
Copy link
Member

I've pushed an updated set of changes, to address some of my earlier feedback and some issues with the subsequent commit. Give things a try out on master and comment here or on PROTON-1405 if you have any issues.

@gemmellr
Copy link
Member

Actually, if you could also confirm if its just working for you that would be good too, as I can then look to kick off the release process on 0.18.0 some time, perhaps next week.

SreeramGarlapati pushed a commit to SreeramGarlapati/qpid-proton-j that referenced this pull request Sep 20, 2018
@alanconway
PROTON-1574: Fix AddressSanitizer: stack-use-after-scope
c99ab4a 
This is a strange error and probably a false one, but this fix removes it and is
otherwise harmless.

8: TEST: (coerce_test<V>())
8: =================================================================
8: ==522==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffc0fb69b78 at pc 0x7f67732cfde3 bp 0x7ffc0fb69a70 sp 0x7ffc0fb69a68
8: WRITE of size 8 at 0x7ffc0fb69b78 thread T0
8:     #0 0x7f67732cfde2 in std::_Vector_base<unsigned char, std::allocator<unsigned char> >::_Vector_impl::_Vector_impl() /nix/store/pdidaf83cvkrgx8xjgjdnl5m1naqjbfk-gcc-7.1.0/include/c++/7.1.0/bits/stl_vector.h:89
8:     apache#1 0x7f67732cfde2 in std::_Vector_base<unsigned char, std::allocator<unsigned char> >::_Vector_base() /nix/store/pdidaf83cvkrgx8xjgjdnl5m1naqjbfk-gcc-7.1.0/include/c++/7.1.0/bits/stl_vector.h:127
8:     apache#2 0x7f67732cfde2 in std::vector<unsigned char, std::allocator<unsigned char> >::vector() /nix/store/pdidaf83cvkrgx8xjgjdnl5m1naqjbfk-gcc-7.1.0/include/c++/7.1.0/bits/stl_vector.h:263
8:     apache#3 0x7f67732cfde2 in proton::binary::binary() /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/include/proton/binary.hpp:41
8:     apache#4 0x7f67732cfde2 in proton::scalar_base::scalar_base() /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/src/scalar_base.cpp:36
8:     apache#5 0x53022f in proton::scalar::scalar() /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/include/proton/./scalar.hpp:35
8:     apache#6 0x53022f in void proton::coerce<double>(proton::value const&, double&) /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/include/proton/./value.hpp:151
8:     apache#7 0x621343 in double proton::coerce<double>(proton::value const&) /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/include/proton/./value.hpp:141
8:     apache#8 0x621343 in void test::coerce_test<proton::value>() /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/src/include/scalar_test.hpp:129
8:     apache#9 0x67c623 in void test::scalar_test_group<proton::value>(int&) /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/src/include/scalar_test.hpp:202
8:     apache#10 0x41e5b7 in main /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/src/value_test.cpp:83
8:     apache#11 0x7f677042752f in __libc_start_main (/nix/store/l48biijfr1j6d5kdg911051x2phfjrz7-glibc-2.25/lib/libc.so.6+0x2052f)
8:     apache#12 0x439119 in _start (/home/jdanek/Work/repos/qpid-proton/build_sanitizers/proton-c/bindings/cpp/value_test+0x439119)
8:
8: Address 0x7ffc0fb69b78 is located in stack of thread T0 at offset 184 in frame
8:     #0 0x53011f in void proton::coerce<double>(proton::value const&, double&) /home/jdanek/Work/repos/qpid-proton/proton-c/bindings/cpp/include/proton/./value.hpp:148
8:
8:   This frame has 3 object(s):
8:     [32, 33) '<unknown>'
8:     [96, 112) 'd'
8:     [160, 208) 's' <== Memory access at offset 184 is inside this variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ssudhakaran @gemmellr