Skip to content

Commit

Permalink
Convert _str() from inline to a macro and make it return
Browse files Browse the repository at this point in the history 
(const str *), not (str *). This is to limit potential
damage from its (mis)use in non-testing modules code by
first making its value stored locally at point of use
(which should allow compiler to throw it away when used
properly) and also provide at least some protection from
any function from messing with its fields.

Fix any fallout where str * has been passed around.
  • Loading branch information
@sobomax
sobomax committed Jan 28, 2021
1 parent d85d0b5 commit 0147baa
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 19 deletions.
4 changes: 2 additions & 2 deletions modules/dialog/dlg_handlers.c
View file
Expand Up @@ -2707,8 +2707,8 @@ int dlg_validate_dialog( struct sip_msg* req, struct dlg_cell *dlg)
return 0;
}

int terminate_dlg(str *callid, unsigned int h_entry, unsigned int h_id,
str *reason)
int terminate_dlg(const str *callid, unsigned int h_entry, unsigned int h_id,
const str *reason)
{
struct dlg_cell * dlg = NULL;
int ret = 0;
Expand Down
8 changes: 4 additions & 4 deletions modules/dialog/dlg_handlers.h
View file
Expand Up @@ -101,8 +101,8 @@ typedef int (*validate_dialog_f) (struct sip_msg* req, struct dlg_cell *dlg);
typedef int (*fix_route_dialog_f) (struct sip_msg *req,struct dlg_cell *dlg);
/* the dialog is identified by callid if provided,
* otherwise by h_entry and h_id */
typedef int (*terminate_dlg_f)(str *callid, unsigned int h_entry,
unsigned int h_id, str *reason);
typedef int (*terminate_dlg_f)(const str *callid, unsigned int h_entry,
unsigned int h_id, const str *reason);
typedef int (*indialog_reply_f) (struct sip_msg *msg, int statuscode,
void *param);
typedef int (*send_indialog_req_f)(struct dlg_cell *dlg, str *method,
Expand All @@ -128,8 +128,8 @@ int dlg_validate_dialog( struct sip_msg* req, struct dlg_cell *dlg);

int fix_route_dialog(struct sip_msg *req,struct dlg_cell *dlg);

int terminate_dlg(str *callid, unsigned int h_entry, unsigned int h_id,
str *reason);
int terminate_dlg(const str *callid, unsigned int h_entry, unsigned int h_id,
const str *reason);

int send_indialog_request(struct dlg_cell *dlg, str *method,
int leg, str *body, str *ct, str *hdrs, indialog_reply_f func,
Expand Down
2 changes: 1 addition & 1 deletion modules/dialog/dlg_hash.c
View file
Expand Up @@ -867,7 +867,7 @@ struct dlg_cell* get_dlg_by_val(str *attr, str *val)
}


struct dlg_cell* get_dlg_by_callid( str *callid, int active_only)
struct dlg_cell* get_dlg_by_callid(const str *callid, int active_only)
{
struct dlg_cell *dlg;
struct dlg_entry *d_entry;
Expand Down
2 changes: 1 addition & 1 deletion modules/dialog/dlg_hash.h
View file
Expand Up @@ -397,7 +397,7 @@ struct dlg_cell* get_dlg(str *callid, str *ftag, str *ttag,

struct dlg_cell* get_dlg_by_val(str *attr, str *val);

struct dlg_cell* get_dlg_by_callid( str *callid, int active_only);
struct dlg_cell* get_dlg_by_callid(const str *callid, int active_only);

struct dlg_cell* get_dlg_by_did(str *did, int active_only);

Expand Down
2 changes: 1 addition & 1 deletion modules/dialog/dlg_load.h
View file
Expand Up @@ -31,7 +31,7 @@

typedef struct dlg_cell *(*get_dlg_f) (void);
typedef str *(*get_dlg_did_f) (struct dlg_cell *dlg);
typedef struct dlg_cell *(*get_dlg_by_callid_f) (str *, int);
typedef struct dlg_cell *(*get_dlg_by_callid_f) (const str *, int);
typedef struct dlg_cell *(*get_dlg_by_did_f) (str *, int);
typedef int (*match_dialog_f) (struct sip_msg *msg, int _seq_match_mode);
typedef int (*get_direction_f) (void);
Expand Down
2 changes: 1 addition & 1 deletion modules/dialplan/dialplan.c
View file
Expand Up @@ -172,7 +172,7 @@ struct module_exports exports= {


/*Inserts table_name/db url into the list of heads*/
static int dp_head_insert(int dp_insert_type, str *content,
static int dp_head_insert(int dp_insert_type, const str *content,
str *partition)
{
#define h_insert(type, url_str, table_str, ins_str ) \
Expand Down
19 changes: 10 additions & 9 deletions str.h
View file
Expand Up @@ -97,16 +97,17 @@ static inline str *str_cpy(str *dest, const str *src)
/**
* Handy function for writing unit tests which compare str's
*
* WARNING: _only_ use when passing (str *) to _basic_ functions,
* since it is not re-entrant and may cause ugly bugs!
* WARNING: _only_ use when passing (const str *) to _basic_
* functions, since while poiter is stable for the
* lifetime of the application its value is mutable
* and bad code messing it around may cause ugly bugs!
*/
static inline str *_str(const char *s)
{
static str st;

init_str(&st, s);
return &st;
}
#define _str(s) ( \
{ \
static str _st; \
init_str(&_st, s); \
/* return */ (const str *)&_st; \
})

/**
* Initialize private static str_const given the static buffer
Expand Down

2 comments on commit 0147baa

@liviuchircu
Copy link
Member

@liviuchircu liviuchircu commented on 0147baa Jan 29, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "which should allow compiler to throw it away when used properly"? What's a proper way to use a simple string casting macro, after all?

I agree that it's helpful to prevent programmer mistakes going in the future, where someone may abuse _str() and run into tough-to-debug, weird bugs. However, note how this comes with a cost, too. By re-defining a static holder on each invocation of the macro, we are increasing the size of various binaries. For example, this commit immediately causes a 0.06% increase in the size of the usrloc.so binary (from 1565672 bytes to 1566720).

All in all, I think it's better to do the commit and live with the size penalty, than to keep the current, hyper-optimized, error-prone version of it. Long-live OpenSIPS development! :)

@sobomax
Copy link
Contributor Author

@sobomax sobomax commented on 0147baa Jan 29, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @liviuchircu for a feedback! I will probably retract my comment on "when used properly", it does not hold water. The compiler is still storing that pointer even if it clearly not used anywhere. :(

With regards to the size increase, don't worry about it much. It will actually go down once those _str() calls are converted into str_const. Consider the following code blocks for example:

int foo(const str *in)
{
    int a;
    a = str_strcmp(in, _str("abcdefg1"));
    return a;
}

int foo_const(const str *in)
{
    int a;
    a = str_strcmp(in, const_str("abcdefg1"));
    return a;
}

It compiles into almost identical code, except foo_const() does not have any memory operarions to store reference to "abcdedf1" or its length! That's what I am saying as "compiler throws it away". So by changing your usrloc code to use const_str() instead you would not only produce smaller code but it also would run considerably faster!

foo:                                    # @foo
        mov     qword ptr [rip + foo._st], offset .L.str # <- (char *)"abcdefg1"
        mov     dword ptr [rip + foo._st+8], 8 # <-strlen("abcdefg1")
        mov     eax, -2
        test    rdi, rdi
        je      .LBB0_9
        mov     rsi, qword ptr [rdi]
        test    rsi, rsi
        je      .LBB0_9
[...]
foo_const:                              # @foo_const
        mov     eax, -2
        test    rdi, rdi
        je      .LBB1_9
        mov     rsi, qword ptr [rdi]
        test    rsi, rsi
        je      .LBB1_9
[...]

Please sign in to comment.