Skip to content

Commit 45214e5

Browse files
filipecosta90filipecosta90
authored
invalid memory accesses fixes and improvements (#342)
* [fix] removed extra spacings * [fix] removed extra spacings * [fix] _GNU_SOURCE review * [add] checking if enabling caching dependencies breaks ci * [add] added no_output_timeout: 20m to coverage * [add] added no_output_timeout: 20m to coverage
1 parent 76abde1 commit 45214e5

File tree

15 files changed

+129
-113
lines changed

15 files changed

+129
-113
lines changed

‎.circleci/config.yml‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,10 @@ commands:
77
type: string
88
steps:
99
- checkout
10-
# - restore_cache:
11-
# keys:
12-
# - v1-dependencies-{{ checksum "get_deps.sh" }}
13-
# # fallback to using the latest cache if no exact match is found
14-
# # - v1-dependencies-
10+
- restore_cache:
11+
keys:
12+
- v1-dependencies-{{ checksum "get_deps.sh" }}
13+
# If no exact match is found will get dependencies from source
1514
- run:
1615
name: Install dependencies
1716
command: |
@@ -22,7 +21,6 @@ commands:
2221
- save_cache:
2322
paths:
2423
- deps
25-
- /usr/local
2624
key: build-dependencies-{{ checksum "get_deps.sh" }}
2725
- run:
2826
name: Set up workspace
@@ -74,27 +72,32 @@ jobs:
7472
- image: redisfab/rmbuilder:5.0.7-x64-buster
7573
steps:
7674
- checkout
75+
- restore_cache:
76+
keys:
77+
- build-dependencies-{{ checksum "get_deps.sh" }}
78+
# If no exact match is found will get dependencies from source
7779
- run:
7880
name: Install dependencies
7981
command: |
8082
./opt/readies/bin/getpy3
8183
./opt/system-setup.py
8284
# git clone git://github.com/antirez/redis.git --branch 5.0.7; cd redis; make malloc=libc -j $(nproc); make install; redis-server --version
83-
- restore_cache:
84-
keys:
85-
- build-dependencies-{{ checksum "get_deps.sh" }}
86-
# fallback to using the latest cache if no exact match is found
87-
- v1-dependencies-
85+
./get_deps.sh cpu
86+
- run:
87+
name: Set up workspace
88+
command: |
89+
mkdir -p ~/workspace
90+
chown `whoami` ~/workspace
8891
- run:
8992
name: Build
9093
command: |
91-
# make fetch SHOW=1
92-
make -C opt build COV=1 SHOW=1
94+
make -C opt all COV=1 SHOW=1
9395
- run:
9496
name: Test with coverage
9597
command: |
9698
make -C opt test SHOW=1 COV=1
9799
make -C opt cov-upload
100+
no_output_timeout: 20m
98101

99102
build-macos:
100103
macos:
@@ -189,8 +192,6 @@ workflows:
189192
tags:
190193
only: /.*/
191194
- coverage:
192-
requires:
193-
- build
194195
filters:
195196
branches:
196197
only: /.*/

‎opt/Makefile‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,7 @@ SRCDIR=..
8585
BINDIR=$(BINROOT)/src
8686
DEPS_DIR=$(ROOT)/deps/$(OS)-$(ARCH)-$(DEVICE)
8787
INSTALL_DIR=$(BINROOT)/install-$(DEVICE)
88-
COV_EXCLUDE=\
89-
'./rmutil/*'\
90-
'./util/*'
88+
COV_EXCLUDE= "*/rmutil/*" "*src/util/*" "*/deps/*"
9189

9290
TARGET=$(BINDIR)/redisai.so
9391
INSTALLED_TARGET=$(INSTALL_DIR)/redisai.so

‎src/background_workers.c‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,7 @@ int ensureRunQueue(const char *devicestr, RunQueueInfo **run_queue_info) {
7878
void *RedisAI_Run_ThreadMain(void *arg) {
7979
RunQueueInfo *run_queue_info = (RunQueueInfo *)arg;
8080
pthread_t self = pthread_self();
81-
#ifdef __APPLE__
82-
int res = pthread_setname_np("redisai_bthread");
83-
#else
84-
int res = pthread_setname_np(self, "redisai_bthread");
85-
#endif
81+
RAI_PTHREAD_SETNAME("redisai_bthread");
8682
pthread_mutex_lock(&run_queue_info->run_queue_mutex);
8783
while (true) {
8884
int rc = pthread_cond_wait(&run_queue_info->queue_condition_var,

‎src/background_workers.h‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
#if defined(__linux__)
2+
#define _GNU_SOURCE
3+
#endif
4+
15
#ifndef SRC_BACKGROUND_WORKERS_H_
26
#define SRC_BACKGROUND_WORKERS_H_
37

@@ -17,6 +21,24 @@
1721
#include "util/dict.h"
1822
#include "util/queue.h"
1923

24+
/* Define for RedisAI thread name setter */
25+
#ifdef __linux__
26+
#define RAI_PTHREAD_SETNAME(name) pthread_setname_np(pthread_self(), name)
27+
#else
28+
#if (defined __NetBSD__ || defined __FreeBSD__ || defined __OpenBSD__)
29+
#include <pthread_np.h>
30+
#define RAI_PTHREAD_SETNAME(name) pthread_set_name_np(pthread_self(), name)
31+
#else
32+
#if (defined __APPLE__ && defined(MAC_OS_X_VERSION_10_7))
33+
int pthread_setname_np(const char *name);
34+
#include <pthread.h>
35+
#define RAI_PTHREAD_SETNAME(name) pthread_setname_np(name)
36+
#else
37+
#define RAI_PTHREAD_SETNAME(name)
38+
#endif
39+
#endif
40+
#endif
41+
2042
AI_dict *run_queues;
2143
long long perqueueThreadPoolSize;
2244

‎src/err.c‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
#include "err.h"
2-
#include "stdlib.h"
3-
#include "assert.h"
4-
#include "string.h"
52

3+
#include "assert.h"
64
#include "redismodule.h"
5+
#include "stdlib.h"
6+
#include "string.h"
77

88
char *RAI_Chomp(const char *src) {
9-
char* str = RedisModule_Strdup(src);
9+
char *str = RedisModule_Strdup(src);
1010
size_t len = strlen(src);
11-
for (size_t i=0; i<len; i++) {
11+
for (size_t i = 0; i < len; i++) {
1212
if (str[i] == '\n' || str[i] == '\r') {
1313
str[i] = ' ';
1414
}
@@ -17,6 +17,9 @@ char *RAI_Chomp(const char *src) {
1717
}
1818

1919
void RAI_SetError(RAI_Error *err, RAI_ErrorCode code, const char *detail) {
20+
if(!err){
21+
return;
22+
}
2023
if (err->code != RAI_OK) {
2124
return;
2225
}
@@ -37,29 +40,31 @@ void RAI_SetError(RAI_Error *err, RAI_ErrorCode code, const char *detail) {
3740
* @return 0 on success, or 1 if the allocation
3841
* failed.
3942
*/
40-
int RAI_InitError(RAI_Error** result) {
41-
RAI_Error* err;
42-
err = (RAI_Error*)RedisModule_Calloc(1, sizeof(RAI_Error));
43+
int RAI_InitError(RAI_Error **result) {
44+
RAI_Error *err;
45+
err = (RAI_Error *)RedisModule_Calloc(1, sizeof(RAI_Error));
4346
if (!err) {
4447
return 1;
4548
}
46-
err->code=0;
47-
err->detail=NULL;
48-
err->detail_oneline=NULL;
49+
err->code = 0;
50+
err->detail = NULL;
51+
err->detail_oneline = NULL;
4952
*result = err;
5053
return 0;
5154
}
5255

5356
void RAI_ClearError(RAI_Error *err) {
54-
if (err->detail) {
55-
RedisModule_Free(err->detail);
56-
err->detail = NULL;
57-
}
58-
if (err->detail_oneline) {
59-
RedisModule_Free(err->detail_oneline);
60-
err->detail_oneline = NULL;
57+
if (err) {
58+
if (err->detail) {
59+
RedisModule_Free(err->detail);
60+
err->detail = NULL;
61+
}
62+
if (err->detail_oneline) {
63+
RedisModule_Free(err->detail_oneline);
64+
err->detail_oneline = NULL;
65+
}
66+
err->code = RAI_OK;
6167
}
62-
err->code = RAI_OK;
6368
}
6469

6570
void RAI_FreeError(RAI_Error *err) {

‎src/libtorch_c/torch_c.cpp‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,13 @@ extern "C" void* torchLoadModel(const char* graph, size_t graphlen, DLDeviceType
338338
ctx->cu = nullptr;
339339
}
340340
catch(std::exception& e) {
341-
size_t len = strlen(e.what());
341+
const char* err_message = e.what();
342+
const size_t len = strlen(err_message);
342343
*error = (char*)alloc(len * sizeof(char));
343-
strcpy(*error, e.what());
344-
delete ctx;
344+
if(*error){
345+
strcpy(*error, err_message);
346+
}
347+
// delete ctx;
345348
return NULL;
346349
}
347350
return ctx;
@@ -357,9 +360,11 @@ extern "C" void torchRunScript(void* scriptCtx, const char* fnName,
357360
torchRunModule(ctx, fnName, nInputs, inputs, nOutputs, outputs);
358361
}
359362
catch(std::exception& e) {
360-
size_t len = strlen(e.what());
363+
const size_t len = strlen(e.what());
361364
*error = (char*)alloc(len * sizeof(char));
362-
strcpy(*error, e.what());
365+
if(*error!=NULL){
366+
strcpy(*error, e.what());
367+
}
363368
}
364369
}
365370

@@ -373,7 +378,7 @@ extern "C" void torchRunModel(void* modelCtx,
373378
torchRunModule(ctx, "forward", nInputs, inputs, nOutputs, outputs);
374379
}
375380
catch(std::exception& e) {
376-
size_t len = strlen(e.what());
381+
const size_t len = strlen(e.what());
377382
*error = (char*)alloc(len * sizeof(char));
378383
strcpy(*error, e.what());
379384
}
@@ -393,8 +398,8 @@ extern "C" void torchSerializeModel(void* modelCtx, char **buffer, size_t *len,
393398
*len = size;
394399
}
395400
catch(std::exception& e) {
396-
size_t len = strlen(e.what());
397-
*error = (char*)alloc(len * sizeof(char));
401+
const size_t err_len = strlen(e.what());
402+
*error = (char*)alloc(err_len * sizeof(char));
398403
strcpy(*error, e.what());
399404
}
400405
}

‎src/model.c‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ static void* RAI_Model_RdbLoad(struct RedisModuleIO *io, int encver) {
6161
RedisModuleCtx* ctx = RedisModule_GetContextFromIO(io);
6262
int ret = RAI_LoadDefaultBackend(ctx, backend);
6363
if (ret == REDISMODULE_ERR) {
64-
RedisModule_Log(ctx, "error", "Could not load default backend\n");
64+
RedisModule_Log(ctx, "error", "Could not load default backend");
6565
RAI_ClearError(&err);
6666
return NULL;
6767
}
@@ -70,7 +70,8 @@ static void* RAI_Model_RdbLoad(struct RedisModuleIO *io, int encver) {
7070
}
7171

7272
if (err.code != RAI_OK) {
73-
printf("ERR: %s\n", err.detail);
73+
RedisModuleCtx* ctx = RedisModule_GetContextFromIO(io);
74+
RedisModule_Log(ctx, "error", err.detail);
7475
RAI_ClearError(&err);
7576
if (buffer) {
7677
RedisModule_Free(buffer);
@@ -104,6 +105,7 @@ static void RAI_Model_RdbSave(RedisModuleIO *io, void *value) {
104105
int ret = RAI_ModelSerialize(model, &buffer, &len, &err);
105106

106107
if (err.code != RAI_OK) {
108+
RedisModuleCtx* stats_ctx = RedisModule_GetContextFromIO(io);
107109
printf("ERR: %s\n", err.detail);
108110
RAI_ClearError(&err);
109111
if (buffer) {
@@ -142,6 +144,7 @@ static void RAI_Model_AofRewrite(RedisModuleIO *aof, RedisModuleString *key, voi
142144
int ret = RAI_ModelSerialize(model, &buffer, &len, &err);
143145

144146
if (err.code != RAI_OK) {
147+
145148
printf("ERR: %s\n", err.detail);
146149
RAI_ClearError(&err);
147150
if (buffer) {
@@ -274,7 +277,7 @@ RAI_Model *RAI_ModelCreate(RAI_Backend backend, const char* devicestr, const cha
274277
model = RAI_backends.onnx.model_create(backend, devicestr, opts, modeldef, modellen, err);
275278
}
276279
else {
277-
RAI_SetError(err, RAI_EUNSUPPORTEDBACKEND, "ERR Unsupported backend\n");
280+
RAI_SetError(err, RAI_EUNSUPPORTEDBACKEND, "ERR Unsupported backend");
278281
return NULL;
279282
}
280283

@@ -292,7 +295,7 @@ void RAI_ModelFree(RAI_Model* model, RAI_Error* err) {
292295

293296
if (model->backend == RAI_BACKEND_TENSORFLOW) {
294297
if (!RAI_backends.tf.model_free) {
295-
RAI_SetError(err, RAI_EBACKENDNOTLOADED, "ERR Backend not loaded: TF\n");
298+
RAI_SetError(err, RAI_EBACKENDNOTLOADED, "ERR Backend not loaded: TF");
296299
return;
297300
}
298301
RAI_backends.tf.model_free(model, err);
@@ -319,7 +322,7 @@ void RAI_ModelFree(RAI_Model* model, RAI_Error* err) {
319322
RAI_backends.onnx.model_free(model, err);
320323
}
321324
else {
322-
RAI_SetError(err, RAI_EUNSUPPORTEDBACKEND, "Unsupported backend\n");
325+
RAI_SetError(err, RAI_EUNSUPPORTEDBACKEND, "Unsupported backend");
323326
return;
324327
}
325328

‎src/redisai.c‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -248,10 +248,10 @@ int RedisAI_ModelSet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
248248
model = RAI_ModelCreate(backend, devicestr, tag, opts, ninputs, inputs, noutputs, outputs, modeldef, modellen, &err);
249249

250250
if (err.code == RAI_EBACKENDNOTLOADED) {
251-
RedisModule_Log(ctx, "warning", "backend %s not loaded, will try loading default backend\n", bckstr);
251+
RedisModule_Log(ctx, "warning", "backend %s not loaded, will try loading default backend", bckstr);
252252
int ret = RAI_LoadDefaultBackend(ctx, backend);
253253
if (ret == REDISMODULE_ERR) {
254-
RedisModule_Log(ctx, "error", "could not load %s default backend\n", bckstr);
254+
RedisModule_Log(ctx, "error", "could not load %s default backend", bckstr);
255255
int ret = RedisModule_ReplyWithError(ctx, "ERR Could not load backend");
256256
RAI_ClearError(&err);
257257
return ret;
@@ -265,9 +265,7 @@ int RedisAI_ModelSet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
265265
}
266266

267267
if (err.code != RAI_OK) {
268-
#ifdef RAI_PRINT_BACKEND_ERRORS
269-
printf("ERR: %s\n", err.detail);
270-
#endif
268+
RedisModule_Log(ctx, "error", err.detail);
271269
int ret = RedisModule_ReplyWithError(ctx, err.detail_oneline);
272270
RAI_ClearError(&err);
273271
return ret;
@@ -278,9 +276,7 @@ int RedisAI_ModelSet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
278276
if (ensureRunQueue(devicestr,&run_queue_info) != REDISMODULE_OK){
279277
RAI_ModelFree(model, &err);
280278
if (err.code != RAI_OK) {
281-
#ifdef RAI_PRINT_BACKEND_ERRORS
282-
printf("ERR: %s\n", err.detail);
283-
#endif
279+
RedisModule_Log(ctx, "error", err.detail);
284280
int ret = RedisModule_ReplyWithError(ctx, err.detail_oneline);
285281
RAI_ClearError(&err);
286282
return ret;
@@ -297,9 +293,7 @@ int RedisAI_ModelSet_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv,
297293
RedisModule_CloseKey(key);
298294
RAI_ModelFree(model, &err);
299295
if (err.code != RAI_OK) {
300-
#ifdef RAI_PRINT_BACKEND_ERRORS
301-
printf("ERR: %s\n", err.detail);
302-
#endif
296+
RedisModule_Log(ctx, "error", err.detail);
303297
int ret = RedisModule_ReplyWithError(ctx, err.detail_oneline);
304298
RAI_ClearError(&err);
305299
return ret;

0 commit comments

Comments
 (0)