Skip to content

Commit c0a0c99

Browse files
author
Marcel Hecko
committed
Fix segfault-prone code paths across core and apps
- AmArg::pop_back: replace undefined erase(end()) with pop_back() - trans_table: replace assert-only dynamic_cast guards with runtime NULL checks and error returns (asserts compiled out in release) - SBC: add args.size() bounds checks before accessing AmArg elements in postControlCmd, reloadProfile, loadProfile, setActiveProfile, setRegexMap, loadCallcontrolModules - sems.cpp: rewrite signal handler to only set sig_atomic_t flags; move unsafe work (mutex, event broadcast) to process_pending_signals() called from the SIP event loop - AmConferenceChannel: remove dangerous delete-this in on_flushed() callback; object lifetime is managed by owning AmConferenceChannel - ip_util: fix uninitialized str[0] read in am_inet_ntop_sip IPv6 path; set '[' before inet_ntop and add buffer overflow guards - AmSmtpClient: check socket() return before connect() - AmEventDispatcher::broadcast: collect queue pointers under lock then post events outside lock to prevent iterator invalidation - DSMChartReader::importModule: add missing dlclose() on error paths - AmUtils/arg_conversion: sprintf -> snprintf to prevent buffer overflows - log.cpp: check malloc return in __lds before using funcname pointer - tcp_trsp: guard memmove against integer underflow when addr_shift exceeds input_len - RpcPeer: check socket() return before fcntl/connect
1 parent 28a98a7 commit c0a0c99

File tree

16 files changed

+145
-52
lines changed

16 files changed

+145
-52
lines changed

apps/dsm/DSMChartReader.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,13 +243,15 @@ bool DSMChartReader::importModule(const string& mod_cmd, const string& mod_path)
243243
SCFactoryCreate fc = NULL;
244244
if ((fc = (SCFactoryCreate)dlsym(h_dl,SC_FACTORY_EXPORT_STR)) == NULL) {
245245
ERROR("invalid SC module '%s' (SC_EXPORT missing?)\n", fname.c_str());
246+
dlclose(h_dl);
246247
return false;
247248
}
248-
249+
249250
DSMModule* mod = (DSMModule*)fc();
250251
if (!mod) {
251-
ERROR("module '%s' did not return functions.\n",
252+
ERROR("module '%s' did not return functions.\n",
252253
fname.c_str());
254+
dlclose(h_dl);
253255
return false;
254256
}
255257
mods.push_back(mod);

apps/jsonrpc/RpcPeer.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,10 @@ int JsonrpcNetstringsConnection::connect(const string& host, int port,
9494
// }
9595

9696
fd = socket(PF_INET, SOCK_STREAM, 0);
97+
if(fd < 0) {
98+
res_str = "socket() failed: " + string(strerror(errno));
99+
return 300;
100+
}
97101
sa.sin_port = htons(port);
98102
sa.sin_family = PF_INET;
99103

apps/sbc/SBC.cpp

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,11 @@ void SBCFactory::reloadProfiles(const AmArg& args, AmArg& ret) {
542542
}
543543

544544
void SBCFactory::reloadProfile(const AmArg& args, AmArg& ret) {
545+
if (!args.size()) {
546+
ret.push(400);
547+
ret.push("Parameters error: missing arguments");
548+
return;
549+
}
545550
bool failed = false;
546551
string res = "OK";
547552
AmArg p;
@@ -583,7 +588,7 @@ void SBCFactory::reloadProfile(const AmArg& args, AmArg& ret) {
583588
}
584589

585590
void SBCFactory::loadProfile(const AmArg& args, AmArg& ret) {
586-
if (!args[0].hasMember("name") || !args[0].hasMember("path")) {
591+
if (!args.size() || !args[0].hasMember("name") || !args[0].hasMember("path")) {
587592
ret.push(400);
588593
ret.push("Parameters error: expected ['name': profile_name] "
589594
"and ['path': profile_path]");
@@ -623,7 +628,7 @@ void SBCFactory::getActiveProfile(const AmArg& args, AmArg& ret) {
623628
}
624629

625630
void SBCFactory::setActiveProfile(const AmArg& args, AmArg& ret) {
626-
if (!args[0].hasMember("active_profile")) {
631+
if (!args.size() || !args[0].hasMember("active_profile")) {
627632
ret.push(400);
628633
ret.push("Parameters error: expected ['active_profile': <active_profile list>] ");
629634
return;
@@ -651,7 +656,7 @@ void SBCFactory::getRegexMapNames(const AmArg& args, AmArg& ret) {
651656
}
652657

653658
void SBCFactory::setRegexMap(const AmArg& args, AmArg& ret) {
654-
if (!args[0].hasMember("name") || !args[0].hasMember("file") ||
659+
if (!args.size() || !args[0].hasMember("name") || !args[0].hasMember("file") ||
655660
!isArgCStr(args[0]["name"]) || !isArgCStr(args[0]["file"])) {
656661
ret.push(400);
657662
ret.push("Parameters error: expected ['name': <name>, 'file': <file name>]");
@@ -673,6 +678,11 @@ void SBCFactory::setRegexMap(const AmArg& args, AmArg& ret) {
673678
}
674679

675680
void SBCFactory::loadCallcontrolModules(const AmArg& args, AmArg& ret) {
681+
if (!args.size()) {
682+
ret.push(400);
683+
ret.push("Parameters error: missing arguments");
684+
return;
685+
}
676686
string load_cc_plugins = args[0].asCStr();
677687
if (!load_cc_plugins.empty()) {
678688
INFO("loading call control plugins '%s' from '%s'\n",
@@ -691,6 +701,11 @@ void SBCFactory::loadCallcontrolModules(const AmArg& args, AmArg& ret) {
691701
}
692702

693703
void SBCFactory::postControlCmd(const AmArg& args, AmArg& ret) {
704+
if (args.size()<2) {
705+
ret.push(400);
706+
ret.push("Parameters error: expected at least cmd and ltag");
707+
return;
708+
}
694709
SBCControlEvent* evt;
695710
if (args.size()<3) {
696711
evt = new SBCControlEvent(args[1].asCStr());

apps/sbc/arg_conversion.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,23 +17,23 @@ static string arg2string(const AmArg &a)
1717
switch (a.getType()) {
1818
case AmArg::CStr:
1919
p = a.asCStr();
20-
sprintf(tmp, "%c%zd/", CSTR_LABEL, strlen(p));
20+
snprintf(tmp, sizeof(tmp), "%c%zd/", CSTR_LABEL, strlen(p));
2121
s = tmp;
2222
s += p;
2323
return s;
2424

2525
case AmArg::Array:
26-
sprintf(tmp, "%c%zd/", ARRAY_LABEL, a.size());
26+
snprintf(tmp, sizeof(tmp), "%c%zd/", ARRAY_LABEL, a.size());
2727
s = tmp;
2828
for (size_t i = 0; i < a.size(); i ++) s += arg2string(a[i]);
2929
return s;
3030

3131
case AmArg::Struct:
32-
sprintf(tmp, "%c%zd/", STRUCT_LABEL, a.size());
32+
snprintf(tmp, sizeof(tmp), "%c%zd/", STRUCT_LABEL, a.size());
3333
s = tmp;
3434
for (AmArg::ValueStruct::const_iterator it = a.asStruct()->begin();
3535
it != a.asStruct()->end(); ++it) {
36-
sprintf(tmp, "%zd/", it->first.size());
36+
snprintf(tmp, sizeof(tmp), "%zd/", it->first.size());
3737
s += tmp;
3838
s += it->first;
3939
s += arg2string(it->second);

apps/voicemail/AmSmtpClient.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,13 @@ bool AmSmtpClient::connect(const string& _server_ip, unsigned short _server_port
9595
}
9696

9797
sd = socket(PF_INET, SOCK_STREAM, 0);
98+
if(sd < 0) {
99+
ERROR("socket() failed: %s\n",strerror(errno));
100+
return false;
101+
}
98102
if(::connect(sd,(struct sockaddr *)&addr,sizeof(addr)) == -1) {
99103
ERROR("%s\n",strerror(errno));
104+
close();
100105
return false;
101106
}
102107

core/AmArg.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ void AmArg::pop_back(AmArg &a) {
202202
return;
203203
}
204204
a = v_array->back();
205-
v_array->erase(v_array->end());
205+
v_array->pop_back();
206206
}
207207

208208
void AmArg::pop_back() {

core/AmConferenceChannel.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ int ChannelWritingFile::write_to_file(const void* buf, unsigned int len) {
121121
}
122122

123123
void ChannelWritingFile::on_flushed() {
124-
DBG("file on_flushed, deleting self\n");
125-
delete this; // uh!
124+
DBG("file on_flushed\n");
125+
// Note: this object is owned by AmConferenceChannel and will be
126+
// cleaned up when the channel is destroyed. Do not delete here
127+
// as the parent still holds a raw pointer to us.
126128
}

core/AmEventDispatcher.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -204,16 +204,18 @@ bool AmEventDispatcher::broadcast(AmEvent* ev)
204204
for (size_t i=0;i<EVENT_DISPATCHER_BUCKETS;i++) {
205205
queues_mut[i].lock();
206206

207-
EvQueueMapIter it = queues[i].begin();
208-
while (it != queues[i].end()) {
209-
EvQueueMapIter this_evq = it;
210-
it++;
211-
queues_mut[i].unlock();
212-
this_evq->second.q->postEvent(ev->clone());
213-
queues_mut[i].lock();
214-
posted = true;
207+
std::vector<AmEventQueueInterface*> bucket_queues;
208+
for (EvQueueMapIter it = queues[i].begin();
209+
it != queues[i].end(); ++it) {
210+
bucket_queues.push_back(it->second.q);
215211
}
212+
216213
queues_mut[i].unlock();
214+
215+
for (size_t j=0;j<bucket_queues.size();j++) {
216+
bucket_queues[j]->postEvent(ev->clone());
217+
posted = true;
218+
}
217219
}
218220

219221
delete ev;

core/AmUtils.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ string long2hex(unsigned long val)
164164
/** Convert a double to a string. (from jsoncpp) */
165165
string double2str(double val) {
166166
char buffer[32];
167-
sprintf(buffer, "%#.16g", val);
167+
snprintf(buffer, sizeof(buffer), "%#.16g", val);
168168
char* ch = buffer + strlen(buffer) - 1;
169169
if (*ch != '0') return buffer; // nothing to truncate, so save time
170170
while(ch > buffer && *ch == '0'){
@@ -523,8 +523,8 @@ std::string URL_encode(const std::string &s)
523523
else
524524
{
525525
escaped.append("%");
526-
char buf[3];
527-
sprintf(buf, "%.2X", (unsigned char)s[i]);
526+
char buf[4];
527+
snprintf(buf, sizeof(buf), "%.2X", (unsigned char)s[i]);
528528
escaped.append(buf);
529529
}
530530
}

core/SipCtrlInterface.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
*/
2929
#include "SipCtrlInterface.h"
3030

31+
#include "sems.h"
3132
#include "AmUtils.h"
3233
#include "AmSipMsg.h"
3334
#include "AmMimeBody.h"
@@ -400,7 +401,8 @@ int _SipCtrlInterface::run()
400401
}
401402

402403
while (!stopped.get()) {
403-
stopped.wait_for();
404+
stopped.wait_for_to(500); // wake periodically to process signals
405+
process_pending_signals();
404406
}
405407

406408
DBG("SIP control interface ending\n");

0 commit comments

Comments
 (0)