Re: [PATCH 0/3] Additional ist functions
Hi Tim, On Thu, Apr 08, 2021 at 07:32:10PM +0200, Tim Düsterhus wrote: > > No emergency but since I guess you're using them in your code, it would > > be nice that your first caller uses either a secured or explicit version. > > I'll opt for the explicit version, because for a secured version I'd need a > proper return value to indicate whether it succeeded or not and I really > don't want istappend() to take a pointer. I suspect that no one would have > checked that return value anyway, because it is not really actionable. OK! > I'd preferred if you would not have taken the patch yet. A follow-up feels > ugly and writing a good commit message is hard :-) Hey don't worry. If I merged it it's because it was correct. Matters of taste and naming are what the current cool down period is for and I'm perfectly fine with stacking patch iterations to clean up some stuff until we're perfectly happy. I've already done quite some renaming and some still remains so that's pretty fine. Applied now, thank you! Willy
Re: [PATCH 0/3] Additional ist functions
Willy,
On 4/7/21 7:56 PM, Willy Tarreau wrote:
Overall it all looks good so I've merged it. I'd just have one small
request regarding istappend(), it's the first really unsafe function
we have in this collection that could be used inside a loop and cause
buffer overflows, especially since ist strings are designed to be
easier to use than plain strings (i.e. users care less). I'm prefectly
fine with having unsafe functions but not with a default name, so I'd
rather have __istappend() that the caller knows he wants to use and
takes the responsibility for, and istappend() that adds the length
check against an extra argument "size" as a few other functions do
in this case (e.g. istcat() uses a count argument for this).
No emergency but since I guess you're using them in your code, it would
be nice that your first caller uses either a secured or explicit version.
I'll opt for the explicit version, because for a secured version I'd
need a proper return value to indicate whether it succeeded or not and I
really don't want istappend() to take a pointer. I suspect that no one
would have checked that return value anyway, because it is not really
actionable.
I'd preferred if you would not have taken the patch yet. A follow-up
feels ugly and writing a good commit message is hard :-)
Patch atteched.
Best regards
Tim Düsterhus
>From e0bfbc39ca443ca07dc4676b78ea56d131adb311 Mon Sep 17 00:00:00 2001
From: Tim Duesterhus
Date: Thu, 8 Apr 2021 19:28:16 +0200
Subject: [PATCH] MINOR: ist: Rename istappend() to __istappend()
To: haproxy@formilux.org
Cc: w...@1wt.eu
Indicate that this function is not inherently safe by adding two underscores as
a prefix.
---
include/import/ist.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/import/ist.h b/include/import/ist.h
index daf23d552..a19e22802 100644
--- a/include/import/ist.h
+++ b/include/import/ist.h
@@ -425,7 +425,7 @@ static inline int istneq(const struct ist ist1, const struct ist ist2, size_t co
/* appends after . The caller must ensure that the underlying buffer
* is large enough to fit the character.
*/
-static inline struct ist istappend(struct ist dst, const char src)
+static inline struct ist __istappend(struct ist dst, const char src)
{
dst.ptr[dst.len++] = src;
--
2.31.1
Re: [PATCH 0/3] Additional ist functions
On Wed, Apr 07, 2021 at 07:56:58PM +0200, Willy Tarreau wrote: > No emergency but since I guess you're using them in your code, it would > be nice that your first caller uses either a secured or explicit version. And by the way I forgot to say, I like the idea of the istsplit(), it could make string tokenizing easier in many cases :-) Willy
Re: [PATCH 0/3] Additional ist functions
Hi Tim, On Mon, Apr 05, 2021 at 05:53:53PM +0200, Tim Duesterhus wrote: > Willy, > > some more `ist` helper functions that allows consumers to avoid directly > operating on the raw pointer, instead using safe high level functions. > > These will be used in a future series of mine. I'm sending them for early > review and integration, because I believe their existence is useful on its > own. Overall it all looks good so I've merged it. I'd just have one small request regarding istappend(), it's the first really unsafe function we have in this collection that could be used inside a loop and cause buffer overflows, especially since ist strings are designed to be easier to use than plain strings (i.e. users care less). I'm prefectly fine with having unsafe functions but not with a default name, so I'd rather have __istappend() that the caller knows he wants to use and takes the responsibility for, and istappend() that adds the length check against an extra argument "size" as a few other functions do in this case (e.g. istcat() uses a count argument for this). No emergency but since I guess you're using them in your code, it would be nice that your first caller uses either a secured or explicit version. Thanks! Willy
[PATCH 0/3] Additional ist functions
Willy, some more `ist` helper functions that allows consumers to avoid directly operating on the raw pointer, instead using safe high level functions. These will be used in a future series of mine. I'm sending them for early review and integration, because I believe their existence is useful on its own. Best regards Tim Düsterhus (3): MINOR: ist: Add `istappend(struct ist, char)` MINOR: ist: Add `istshift(struct ist*)` MINOR: ist: Add `istsplit(struct ist*, char)` include/import/ist.h | 39 +++ 1 file changed, 39 insertions(+) -- 2.31.1
