Skip to content

Commit 5ceebe0

Browse files
authored
Fix Secure Discovery Server client disposals guid and handshake_handle assertion (#5257)
* Refs #21696: Add regression test for SecurityManager assertion Signed-off-by: Mario Dominguez <[email protected]> * Refs #21696: Fix SecurityManager assertion Signed-off-by: Mario Dominguez <[email protected]> * Refs #21696: Regression test for guid disposal issue Signed-off-by: Mario Dominguez <[email protected]> * Refs #21696: Fix remote disposal guid pdpclient issue Signed-off-by: Mario Dominguez <[email protected]> * Refs #21696: Apply Jesus rev Signed-off-by: Mario Dominguez <[email protected]> * Refs #21696: Apply NIT Signed-off-by: Mario Dominguez <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]>
1 parent 0616997 commit 5ceebe0

File tree

7 files changed

+208
-51
lines changed

7 files changed

+208
-51
lines changed

src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp

+9-2
Original file line numberDiff line numberDiff line change
@@ -769,9 +769,16 @@ void PDPClient::announceParticipantState(
769769
// if we are matched to a server report demise
770770
if (svr.is_connected)
771771
{
772-
//locators.push_back(svr.metatrafficMulticastLocatorList);
772+
GuidPrefix_t srv_guid_prefix = svr.guidPrefix;
773+
#if HAVE_SECURITY
774+
if (getRTPSParticipant()->is_secure())
775+
{
776+
// Need the mangled guid prefix in this case
777+
srv_guid_prefix = get_participant_proxy_data(svr.guidPrefix)->m_guid.guidPrefix;
778+
}
779+
#endif // HAVE_SECURITY
773780
locators.push_back(svr.metatrafficUnicastLocatorList);
774-
remote_readers.emplace_back(svr.guidPrefix,
781+
remote_readers.emplace_back(srv_guid_prefix,
775782
endpoints->reader.reader_->getGuid().entityId);
776783
}
777784
}

src/cpp/rtps/security/SecurityManager.cpp

+7
Original file line numberDiff line numberDiff line change
@@ -935,6 +935,10 @@ bool SecurityManager::on_process_handshake(
935935
}
936936
else
937937
{
938+
// Return the handshake handle
939+
authentication_plugin_->return_handshake_handle(remote_participant_info->handshake_handle_,
940+
exception);
941+
remote_participant_info->handshake_handle_ = nullptr;
938942
EPROSIMA_LOG_ERROR(SECURITY, "WriterHistory cannot add the CacheChange_t");
939943
}
940944
}
@@ -946,6 +950,9 @@ bool SecurityManager::on_process_handshake(
946950
}
947951
else
948952
{
953+
// Return the handshake handle
954+
authentication_plugin_->return_handshake_handle(remote_participant_info->handshake_handle_, exception);
955+
remote_participant_info->handshake_handle_ = nullptr;
949956
EPROSIMA_LOG_ERROR(SECURITY, "WriterHistory cannot retrieve a CacheChange_t");
950957
}
951958
}

test/dds/communication/security/CMakeLists.txt

+44-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ add_definitions(
1818
-DBOOST_ASIO_STANDALONE
1919
-DASIO_STANDALONE
2020
)
21-
21+
2222
include_directories(${Asio_INCLUDE_DIR})
2323

2424
###############################################################################
@@ -393,6 +393,48 @@ if(SECURITY)
393393
"PATH=$<TARGET_FILE_DIR:${PROJECT_NAME}>\\;$<TARGET_FILE_DIR:fastcdr>\\;${WIN_PATH}")
394394
endif()
395395

396+
add_test(NAME SecureDiscoverServerMultipleClientsHandShakeAssertion
397+
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_BINARY_DIR}/multiple_secure_ds_pubsub_secure_crypto_communication.py
398+
--pub $<TARGET_FILE:CommunicationPublisher>
399+
--xml-pub secure_ds_simple_secure_msg_crypto_pub_profile.xml
400+
--sub $<TARGET_FILE:CommunicationSubscriber>
401+
--xml-sub secure_ds_simple_secure_msg_crypto_sub_profile.xml
402+
--samples 100 #not important to receive all samples
403+
--servers $<TARGET_FILE:fast-discovery-server>
404+
--xml-servers secure_simple_ds_server_profile.xml
405+
--n-clients 30
406+
--relaunch-clients
407+
--validation-method server)
408+
409+
# Set test with label NoMemoryCheck
410+
set_property(TEST SecureDiscoverServerMultipleClientsHandShakeAssertion PROPERTY LABELS "NoMemoryCheck")
411+
412+
if(WIN32)
413+
string(REPLACE ";" "\\;" WIN_PATH "$ENV{PATH}")
414+
set_property(TEST SecureDiscoverServerMultipleClientsHandShakeAssertion APPEND PROPERTY ENVIRONMENT
415+
"PATH=$<TARGET_FILE_DIR:${PROJECT_NAME}>\\;$<TARGET_FILE_DIR:fastcdr>\\;${WIN_PATH}")
416+
endif()
417+
418+
add_test(NAME SecureDiscoverServerMultipleClientsDisposalsReceived
419+
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_BINARY_DIR}/multiple_secure_ds_pubsub_secure_crypto_communication.py
420+
--pub $<TARGET_FILE:CommunicationPublisher>
421+
--xml-pub secure_ds_simple_secure_msg_crypto_pub_profile.xml
422+
--sub $<TARGET_FILE:CommunicationSubscriber>
423+
--xml-sub secure_ds_simple_secure_msg_crypto_sub_profile.xml
424+
--samples 5
425+
--servers $<TARGET_FILE:fast-discovery-server>
426+
--xml-servers secure_simple_ds_server_profile.xml
427+
--exit-on-disposal-received)
428+
429+
# Set test with label NoMemoryCheck
430+
set_property(TEST SecureDiscoverServerMultipleClientsDisposalsReceived PROPERTY LABELS "NoMemoryCheck")
431+
432+
if(WIN32)
433+
string(REPLACE ";" "\\;" WIN_PATH "$ENV{PATH}")
434+
set_property(TEST SecureDiscoverServerMultipleClientsDisposalsReceived APPEND PROPERTY ENVIRONMENT
435+
"PATH=$<TARGET_FILE_DIR:${PROJECT_NAME}>\\;$<TARGET_FILE_DIR:fastcdr>\\;${WIN_PATH}")
436+
endif()
437+
396438
endif()
397439

398-
endif()
440+
endif()

test/dds/communication/security/PublisherMain.cpp

+6-1
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ int main(
4040
{
4141
int arg_count = 1;
4242
bool exit_on_lost_liveliness = false;
43+
bool exit_on_disposal_received = false;
4344
bool fixed_type = false;
4445
bool zero_copy = false;
4546
uint32_t seed = 7800;
@@ -55,6 +56,10 @@ int main(
5556
{
5657
exit_on_lost_liveliness = true;
5758
}
59+
else if (strcmp(argv[arg_count], "--exit_on_disposal_received") == 0)
60+
{
61+
exit_on_disposal_received = true;
62+
}
5863
else if (strcmp(argv[arg_count], "--fixed_type") == 0)
5964
{
6065
fixed_type = true;
@@ -137,7 +142,7 @@ int main(
137142
DomainParticipantFactory::get_instance()->load_XML_profiles_file(xml_file);
138143
}
139144

140-
PublisherModule publisher(exit_on_lost_liveliness, fixed_type, zero_copy);
145+
PublisherModule publisher(exit_on_lost_liveliness, exit_on_disposal_received, fixed_type, zero_copy);
141146

142147
if (publisher.init(seed, magic))
143148
{

test/dds/communication/security/PublisherModule.cpp

+4
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,10 @@ void PublisherModule::on_participant_discovery(
230230
{
231231
std::cout << "Publisher participant " << // participant->getGuid() <<
232232
" removed participant " << info.info.m_guid << std::endl;
233+
if (exit_on_disposal_received_)
234+
{
235+
run_ = false;
236+
}
233237
}
234238
else if (info.status == ParticipantDiscoveryInfo::DROPPED_PARTICIPANT)
235239
{

test/dds/communication/security/PublisherModule.hpp

+3
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ class PublisherModule
4141

4242
PublisherModule(
4343
bool exit_on_lost_liveliness,
44+
bool exit_on_disposal_received,
4445
bool fixed_type = false,
4546
bool zero_copy = false)
4647
: exit_on_lost_liveliness_(exit_on_lost_liveliness)
48+
, exit_on_disposal_received_(exit_on_disposal_received)
4749
, fixed_type_(zero_copy || fixed_type) // If zero copy active, fixed type is required
4850
, zero_copy_(zero_copy)
4951
{
@@ -91,6 +93,7 @@ class PublisherModule
9193
std::condition_variable cv_;
9294
unsigned int matched_ = 0;
9395
bool exit_on_lost_liveliness_ = false;
96+
bool exit_on_disposal_received_ = false;
9497
bool fixed_type_ = false;
9598
bool zero_copy_ = false;
9699
bool run_ = true;

test/dds/communication/security/multiple_secure_ds_pubsub_secure_crypto_communication.py

+135-46
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import os
1919
import subprocess
2020
import sys
21+
import time
2122

2223
class ParseOptions():
2324
"""Parse arguments."""
@@ -93,9 +94,106 @@ def __parse_args(self):
9394
type=str,
9495
help='Path to the xml configuration file containing discovery server.'
9596
)
97+
parser.add_argument(
98+
'-nc',
99+
'--n-clients',
100+
type=int,
101+
help='Number of pubsub clients to launch (1 of each).'
102+
)
103+
parser.add_argument(
104+
'-rc',
105+
'--relaunch-clients',
106+
action='store_true',
107+
help='Whether to kill clients and relaunch them.'
108+
)
109+
parser.add_argument(
110+
'-vm',
111+
'--validation-method',
112+
type=str,
113+
help='Validation method to use [server, subscriber].'
114+
)
115+
parser.add_argument(
116+
'-edr',
117+
'--exit-on-disposal-received',
118+
action='store_true',
119+
help='Let the publisher finish the process if receives a disposal.'
120+
)
96121

97122
return parser.parse_args()
98123

124+
def launch_discovery_server_processes(servers, xml_servers):
125+
126+
ds_procs = []
127+
128+
for i in range(0, len(servers)):
129+
server_cmd = []
130+
131+
if not os.path.isfile(servers[i]):
132+
print(f'Discovery server executable file does not exists: {servers[i]}')
133+
sys.exit(1)
134+
135+
if not os.access(servers[i], os.X_OK):
136+
print(
137+
'Discovery server executable does not have execution permissions:'
138+
f'{servers[i]}')
139+
sys.exit(1)
140+
141+
server_cmd.append(servers[i])
142+
server_cmd.extend(['--xml-file', xml_servers[i]])
143+
server_cmd.extend(['--server-id', str(i)])
144+
145+
ds_proc = subprocess.Popen(server_cmd)
146+
print(
147+
'Running Discovery Server - commmand: ',
148+
' '.join(map(str, server_cmd)))
149+
150+
ds_procs.append(ds_proc)
151+
152+
return ds_procs
153+
154+
def launch_client_processes(n_clients, pub_command, sub_command):
155+
156+
pub_procs = []
157+
sub_procs = []
158+
159+
for i in range(0, n_clients):
160+
sub_proc = subprocess.Popen(sub_command)
161+
print(
162+
f'Running Subscriber - commmand: ',
163+
' '.join(map(str, sub_command)))
164+
165+
pub_proc = subprocess.Popen(pub_command)
166+
print(
167+
'Running Publisher - commmand: ',
168+
' '.join(map(str, pub_command)))
169+
170+
sub_procs.append(sub_proc)
171+
pub_procs.append(pub_proc)
172+
173+
return sub_procs, pub_procs
174+
175+
def cleanup(pub_procs, sub_procs, ds_procs):
176+
177+
[sub_proc.kill() for sub_proc in sub_procs]
178+
[pub_proc.kill() for pub_proc in pub_procs]
179+
[ds_proc.kill() for ds_proc in ds_procs]
180+
181+
def cleanup_clients(pub_procs, sub_procs):
182+
183+
[sub_proc.kill() for sub_proc in sub_procs]
184+
[pub_proc.kill() for pub_proc in pub_procs]
185+
186+
def terminate(ok = True):
187+
if ok:
188+
try:
189+
sys.exit(os.EX_OK)
190+
except AttributeError:
191+
sys.exit(0)
192+
else:
193+
try:
194+
sys.exit(os.EX_SOFTWARE)
195+
except AttributeError:
196+
sys.exit(1)
99197

100198
def run(args):
101199
"""
@@ -108,6 +206,8 @@ def run(args):
108206
"""
109207
pub_command = []
110208
sub_command = []
209+
n_clients = 1
210+
relaunch_clients = False
111211

112212
script_dir = os.path.dirname(os.path.realpath(__file__))
113213

@@ -152,71 +252,60 @@ def run(args):
152252
if args.wait:
153253
pub_command.extend(['--wait', str(args.wait)])
154254

255+
if args.exit_on_disposal_received:
256+
pub_command.extend(['--exit_on_disposal_received'])
257+
155258
if args.samples:
156259
pub_command.extend(['--samples', str(args.samples)])
157260
sub_command.extend(['--samples', str(args.samples)])
158261

262+
if args.n_clients:
263+
n_clients = int(args.n_clients)
264+
265+
if args.relaunch_clients:
266+
relaunch_clients = True
267+
159268
if len(args.servers) != len(args.xml_servers):
160269
print(
161270
'Number of servers arguments should be equal to the number of xmls provided.')
162271
sys.exit(1)
163272

164-
ds_procs = []
165-
for i in range(0, len(args.servers)):
166-
server_cmd = []
273+
ds_procs = launch_discovery_server_processes(args.servers, args.xml_servers)
167274

168-
if not os.path.isfile(args.servers[i]):
169-
print(f'Discovery server executable file does not exists: {args.servers[i]}')
170-
sys.exit(1)
275+
sub_procs, pub_procs = launch_client_processes(n_clients, pub_command, sub_command)
171276

172-
if not os.access(args.servers[i], os.X_OK):
173-
print(
174-
'Discovery server executable does not have execution permissions:'
175-
f'{args.servers[i]}')
176-
sys.exit(1)
277+
terminate_ok = True
177278

178-
server_cmd.append(args.servers[i])
179-
server_cmd.extend(['--xml-file', args.xml_servers[i]])
180-
server_cmd.extend(['--server-id', str(i)])
279+
if relaunch_clients:
280+
time.sleep(3)
181281

182-
ds_proc = subprocess.Popen(server_cmd)
183-
print(
184-
'Running Discovery Server - commmand: ',
185-
' '.join(map(str, server_cmd)))
282+
cleanup_clients(pub_procs, sub_procs)
283+
sub_procs, pub_procs = launch_client_processes(n_clients, pub_command, sub_command)
186284

187-
ds_procs.append(ds_proc)
285+
time.sleep(3)
188286

189-
sub_proc = subprocess.Popen(sub_command)
190-
print(
191-
f'Running Subscriber - commmand: ',
192-
' '.join(map(str, sub_command)))
193-
194-
pub_proc = subprocess.Popen(pub_command)
195-
print(
196-
'Running Publisher - commmand: ',
197-
' '.join(map(str, pub_command)))
198-
199-
try:
200-
outs, errs = sub_proc.communicate(timeout=15)
201-
except subprocess.TimeoutExpired:
202-
print('Subscriber process timed out, terminating...')
203-
sub_proc.kill()
204-
pub_proc.kill()
205-
[ds_proc.kill() for ds_proc in ds_procs]
287+
if args.validation_method == 'server':
288+
# Check If discovery servers are still running
289+
for ds_proc in ds_procs:
290+
retcode = ds_proc.poll()
291+
if retcode is not None and retcode is not 0:
292+
print('Discovery Server process dead, terminating...')
293+
terminate_ok = False
294+
else:
206295
try:
207-
sys.exit(os.EX_SOFTWARE)
208-
except AttributeError:
209-
sys.exit(1)
296+
for sub_proc in sub_procs:
297+
outs, errs = sub_proc.communicate(timeout=15)
210298

299+
if args.exit_on_disposal_received:
300+
for pub_proc in pub_procs:
301+
outs, errs = pub_proc.communicate(timeout=5)
211302

212-
pub_proc.kill()
213-
ds_proc.kill()
214-
[ds_proc.kill() for ds_proc in ds_procs]
215-
try:
216-
sys.exit(os.EX_OK)
217-
except AttributeError:
218-
sys.exit(0)
303+
except subprocess.TimeoutExpired:
304+
print('Target process timed out, terminating...')
305+
terminate_ok = False
219306

307+
cleanup(pub_procs, sub_procs, ds_procs)
308+
terminate(terminate_ok)
220309

221310
if __name__ == '__main__':
222311

0 commit comments

Comments
 (0)