From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Mergen Imeev <imeevma@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Date: Tue, 30 Nov 2021 23:02:40 +0100 [thread overview] Message-ID: <819eff36-2d59-3328-e442-10703dbbda99@tarantool.org> (raw) In-Reply-To: <20211125083336.GA56448@tarantool.org> Thanks for the fixes! >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >>> index eb169aeb8..74a98c550 100644 >>> --- a/src/box/sql/expr.c >>> +++ b/src/box/sql/expr.c >>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) >>> } >>> } >>> >>> +struct Expr * >>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id) >> >> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable(). >> To be consistent with our naming policy for constructors allocating memory >> and for consistency with with sql_expr_new_column(), sql_expr_new(), >> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon(). >> > Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_' > prefix for functions that only accessible in SQL. It would work for static functions. But if a function is visible in other modules as a symbol, then you would get a conflict during linking if we ever introduce another 'struct expr' somewhere. Even if they do not interest anywhere in the code. However I don't mind leaving it as is. It can be fixed later if ever needed. See 5 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index eb169aeb8..8ff01dd33 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -1314,6 +1314,59 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) > } > } > > +struct Expr * > +expr_new_variable(struct Parse *parse, const struct Token *spec, > + const struct Token *id) > +{ > + assert(spec != 0 && spec->n == 1); > + uint32_t len = 1; > + if (parse->parse_only) { > + diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, > + parse->line_count, parse->line_pos, > + "bindings are not allowed in DDL"); > + parse->is_aborted = true; > + return NULL; > + } > + if (id != NULL) { > + assert(spec->z[0] != '?'); > + if (id->z - spec->z != 1) { > + diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, > + parse->line_count, spec->z - parse->zTail + 1, > + spec->n, spec->z); > + parse->is_aborted = true; > + return NULL; > + } > + if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) { 1. You already checked for 'id != NULL' several lines above. Also you don't need () around the second &&. > + diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, > + parse->line_count, spec->n, spec->z); > + parse->is_aborted = true; > + return NULL; > + } > + len += id->n; > + } > + struct Expr *expr = sqlDbMallocRawNN(parse->db, > + sizeof(*expr) + len + 1); 2. sql_expr_new_empty(). > + if (expr == NULL) { > + parse->is_aborted = true; > + return NULL; > + } > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index ee319d5ad..15f6223b0 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). { > p->flags = EP_Leaf; > p->iAgg = -1; > p->u.zToken = (char*)&p[1]; > - if (op != TK_VARIABLE) { > - int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n); > - if (rc > name_sz) { > - name_sz = rc; > - p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz); > - if (p == NULL) > - goto tarantool_error; > - p->u.zToken = (char *) &p[1]; > - if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz) > - unreachable(); > + int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n); > + if (rc > name_sz) { > + name_sz = rc; > + p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz); > + if (p == NULL) { > + sqlDbFree(pParse->db, p); 3. From the name sqlDbReallocOrFree() it looks like 'p' is already freed. Also it is NULL here anyway, what makes sqlDbFree() nop. > + pParse->is_aborted = true; 4. You will get a crash below, because p is NULL but you dereference it right afterwards. > } > - } else { > - memcpy(p->u.zToken, t.z, t.n); > - p->u.zToken[t.n] = 0; > + p->u.zToken = (char *) &p[1]; 5. Don't need whitespace after cast operator. > + sql_normalize_name(p->u.zToken, name_sz, t.z, t.n); > }
next prev parent reply other threads:[~2021-11-30 22:02 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches 2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches 2021-11-20 0:45 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-25 8:33 ` Mergen Imeev via Tarantool-patches 2021-11-30 22:02 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-12-02 8:32 ` Mergen Imeev via Tarantool-patches 2021-12-09 0:31 ` Vladislav Shpilevoy via Tarantool-patches 2021-12-13 7:34 ` Mergen Imeev via Tarantool-patches 2021-12-13 21:47 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches 2021-11-20 0:46 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-25 8:55 ` Mergen Imeev via Tarantool-patches 2021-11-30 22:04 ` Vladislav Shpilevoy via Tarantool-patches 2021-12-02 8:38 ` Mergen Imeev via Tarantool-patches 2021-12-09 0:31 ` Vladislav Shpilevoy via Tarantool-patches 2021-12-13 7:42 ` Mergen Imeev via Tarantool-patches 2021-12-13 21:48 ` Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=819eff36-2d59-3328-e442-10703dbbda99@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox