MIME-Version: 1.0
References: <79692bf9-17d3-41e6-b9c9-fc8c3944222a@eisentraut.org>
In-Reply-To: <79692bf9-17d3-41e6-b9c9-fc8c3944222a@eisentraut.org>
From: Ranier Vilela <ranier.vf@gmail.com>
Date: Tue, 18 Jun 2024 08:43:23 -0300
Message-ID: <CAEudQApMiHHpx_GA96B8RMWpJ42CEYD4MjtvJpfj7cXrjn8i=Q@mail.gmail.com>
Subject: Re: replace strtok()
To: Peter Eisentraut <peter@eisentraut.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000008f2269061b289796"
Archived-At: <https://www.postgresql.org/message-id/CAEudQApMiHHpx_GA96B8RMWpJ42CEYD4MjtvJpfj7cXrjn8i%3DQ%40mail.gmail.com>
Precedence: bulk

--0000000000008f2269061b289796
Content-Type: multipart/alternative; boundary="0000000000008f2268061b289794"

--0000000000008f2268061b289794
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Em ter., 18 de jun. de 2024 =C3=A0s 04:18, Peter Eisentraut <peter@eisentra=
ut.org>
escreveu:

> Under the topic of getting rid of thread-unsafe functions in the backend
> [0], here is a patch series to deal with strtok().
>
> Of course, strtok() is famously not thread-safe and can be replaced by
> strtok_r().  But it also has the wrong semantics in some cases, because
> it considers adjacent delimiters to be one delimiter.  So if you parse
>
>      SCRAM-SHA-256$<iterations>:<salt>$<storedkey>:<serverkey>
>
> with strtok(), then
>
>      SCRAM-SHA-256$$<iterations>::<salt>$$<storedkey>::<serverkey>
>
> parses just the same.  In many cases, this is arguably wrong and could
> hide mistakes.
>
> So I'm suggesting to use strsep() in those places.  strsep() is
> nonstandard but widely available.
>
> There are a few places where strtok() has the right semantics, such as
> parsing tokens separated by whitespace.  For those, I'm using strtok_r().
>
> A reviewer job here would be to check whether I made that distinction
> correctly in each case.
>
> On the portability side, I'm including a port/ replacement for strsep()
> and some workaround to get strtok_r() for Windows.  I have included
> these here as separate patches for clarity.
>
+1 For making the code thread-safe.
But I would like to see more const char * where this is possible.

For example, in pg_locale.c
IMO, the token variable can be const char *.

At least strchr expects a const char * as the first parameter.

I found another implementation of strsep, it seems lighter to me.
I will attach it for consideration, however, I have not done any testing.

best regards,
Ranier Vilela

--0000000000008f2268061b289794
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail=
_attr">Em ter., 18 de jun. de 2024 =C3=A0s 04:18, Peter Eisentraut &lt;<a h=
ref=3D"mailto:peter@eisentraut.org">peter@eisentraut.org</a>&gt; escreveu:<=
br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8e=
x;border-left:1px solid rgb(204,204,204);padding-left:1ex">Under the topic =
of getting rid of thread-unsafe functions in the backend <br>
[0], here is a patch series to deal with strtok().<br>
<br>
Of course, strtok() is famously not thread-safe and can be replaced by <br>
strtok_r().=C2=A0 But it also has the wrong semantics in some cases, becaus=
e <br>
it considers adjacent delimiters to be one delimiter.=C2=A0 So if you parse=
<br>
<br>
=C2=A0 =C2=A0 =C2=A0SCRAM-SHA-256$&lt;iterations&gt;:&lt;salt&gt;$&lt;store=
dkey&gt;:&lt;serverkey&gt;<br>
<br>
with strtok(), then<br>
<br>
=C2=A0 =C2=A0 =C2=A0SCRAM-SHA-256$$&lt;iterations&gt;::&lt;salt&gt;$$&lt;st=
oredkey&gt;::&lt;serverkey&gt;<br>
<br>
parses just the same.=C2=A0 In many cases, this is arguably wrong and could=
 <br>
hide mistakes.<br>
<br>
So I&#39;m suggesting to use strsep() in those places.=C2=A0 strsep() is <b=
r>
nonstandard but widely available.<br>
<br>
There are a few places where strtok() has the right semantics, such as <br>
parsing tokens separated by whitespace.=C2=A0 For those, I&#39;m using strt=
ok_r().<br>
<br>
A reviewer job here would be to check whether I made that distinction <br>
correctly in each case.<br>
<br>
On the portability side, I&#39;m including a port/ replacement for strsep()=
 <br>
and some workaround to get strtok_r() for Windows.=C2=A0 I have included <b=
r>
these here as separate patches for clarity.<br></blockquote><div>+1 For mak=
ing the code thread-safe.<br>But I would like to see more const char * wher=
e this is possible.<br><br>For example, in pg_locale.c<br>IMO, the token va=
riable can be const char *.<br><br>At least strchr expects a const char * a=
s the first parameter.</div><div><br></div><div>
<span class=3D"gmail-HwtZe" lang=3D"en"><span class=3D"gmail-jCAhz gmail-Jx=
Vs2d"><span class=3D"gmail-ryNqvb">
</span></span><span class=3D"gmail-jCAhz gmail-JxVs2d gmail-ChMk0b"><span c=
lass=3D"gmail-ryNqvb">I found another implementation of strsep, it seems li=
ghter to me. <br></span></span></span></div><div><span class=3D"gmail-HwtZe=
" lang=3D"en"><span class=3D"gmail-jCAhz gmail-JxVs2d gmail-ChMk0b"><span c=
lass=3D"gmail-ryNqvb">I will attach it for consideration, however, I have n=
ot done any testing.</span></span></span>=C2=A0 <br></div><div><br></div><d=
iv>best regards,</div><div>Ranier Vilela</div></div></div>

--0000000000008f2268061b289794--
--0000000000008f2269061b289796
Content-Type: text/plain; charset="US-ASCII"; name="strsep.c"
Content-Disposition: attachment; filename="strsep.c"
Content-Transfer-Encoding: base64
Content-ID: <f_lxkc4h4w0>
X-Attachment-Id: f_lxkc4h4w0

Lyogc3Ryc2VwLmgNCiAqDQogKiAgUHJvdmlkZXMgdGhlIDQuNEJTRCBzdHJzZXAoMykgZnVuY3Rp
b24gZm9yIHRob3NlIHRoYXQgZG9uJ3QgaGF2ZSBpdC4NCiAqDQogKiAgQ29weXJpZ2h0IDIwMTEg
TWljaGFlbCBUaG9tYXMgR3JlZXINCiAqICBEaXN0cmlidXRlZCB1bmRlciB0aGUgQm9vc3QgU29m
dHdhcmUgTGljZW5zZSwgVmVyc2lvbiAxLjAuDQogKiAgKCBTZWUgYWNjb21wYW55aW5nIGZpbGUg
TElDRU5TRV8xXzAudHh0IG9yIGNvcHkgYXQNCiAqICAgaHR0cDovL3d3dy5ib29zdC5vcmcvTElD
RU5TRV8xXzAudHh0ICkNCiAqDQogKiAgSW5jbHVkaW5nIHRoaXMgZmlsZSBtb2RpZmllcyB0aGUg
c3RkIG5hbWVzcGFjZSBpbiBDKysuDQogKg0KICogIERvbid0IGluY2x1ZGUgdGhpcyBmaWxlIGlm
IHlvdXIgY29tcGlsZXIgcHJvdmlkZXMgdGhlIHN0cnNlcCBmdW5jdGlvbiBpbiA8c3RyaW5nLmg+
Lg0KICogIE1ha2Ugc3VyZSB5b3VyIGJ1aWxkIHByb2Nlc3MgdGVzdHMgZm9yIHRoaXMgYW5kIGJl
aGF2ZXMgYWNjb3JkaW5nbHkhDQogKg0KICovDQoNCiNpbmNsdWRlIDxzdHJpbmcuaD4NCg0KY2hh
ciAqDQpzdHJzZXAoY2hhciAqKnN0cmluZ3AsIGNvbnN0IGNoYXIgKmRlbGltKQ0Kew0KCWNoYXIg
KnJlc3VsdDsNCg0KCWlmICgoc3RyaW5ncCA9PSBOVUxMKSB8fCAoKnN0cmluZ3AgPT0gTlVMTCkp
DQoJCXJldHVybiBOVUxMOw0KDQoJcmVzdWx0ID0gKnN0cmluZ3A7DQoNCgl3aGlsZSgqKnN0cmlu
Z3AgJiYgIShzdHJjaHIoZGVsaW0sICoqc3RyaW5ncCkpKQ0KCQkrKypzdHJpbmdwOw0KDQoJaWYg
KCoqc3RyaW5ncCkNCgkJKigqc3RyaW5ncCkrKyA9ICdcMCc7DQoJZWxzZQ0KCQkqc3RyaW5ncCA9
IE5VTEw7DQoNCglyZXR1cm4gcmVzdWx0Ow0KfQ0K
--0000000000008f2269061b289796--