MIME-Version: 1.0
References: <CAC+AXB26a4EmxM2suXxPpJaGrqAdxracd7hskLg-zxtPB50h7A@mail.gmail.com>
 <ZBFQOCHQqKY6X69b@paquier.xyz>
In-Reply-To: <ZBFQOCHQqKY6X69b@paquier.xyz>
From: =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com>
Date: Wed, 15 Mar 2023 12:18:25 +0100
Message-ID: <CAC+AXB3BgqAQ4Fmj2h2DhUXyNL1TqH6mi_+nGh+aGun-grF-kQ@mail.gmail.com>
Subject: Re: Fix fseek() detection of unseekable files on WIN32
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000005120ee05f6ee81e3"
Archived-At: <https://www.postgresql.org/message-id/CAC%2BAXB3BgqAQ4Fmj2h2DhUXyNL1TqH6mi_%2BnGh%2BaGun-grF-kQ%40mail.gmail.com>
Precedence: bulk

--0000000000005120ee05f6ee81e3
Content-Type: multipart/alternative; boundary="0000000000005120ec05f6ee81e1"

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

On Wed, Mar 15, 2023 at 5:57=E2=80=AFAM Michael Paquier <michael@paquier.xy=
z> wrote:

> On Tue, Mar 14, 2023 at 01:26:27PM +0100, Juan Jos=C3=A9 Santamar=C3=ADa =
Flecha
> wrote:
> > As highlighted in [1] fseek() might fail to error even when accessing
> > unseekable streams.
> >
> > PFA a patch that checks the file type before the actual fseek(), so onl=
y
> > supported calls are made.
>
> + * streams, so harden that funcion with our version.
> s/funcion/function/.
>

Done.

+extern int     pgfseek64(FILE *stream, pgoff_t offset, int origin);
> +extern pgoff_t pgftell64(FILE *stream);
> +#define fseeko(stream, offset, origin) pgfseek64(stream, offset, origin)
> +#define ftello(stream) pgftell64(stream)
>
> What about naming the internal wrappers _pgfseeko64() and
> _pgftello64(), located in a new file named win32fseek.c?  It may be
> possible that we would need a similar treatment for fseek(), in the
> future, though I don't see an issue why this would be needed now.
>

Done.


> +   if (GetFileType((HANDLE) _get_osfhandle(_fileno(stream))) !=3D
> FILE_TYPE_DISK)
> +   {
> +       errno =3D ESPIPE;
> +       return -1;
> +   }
> Shouldn't there be cases where we should return EINVAL for some of the
> other types, like FILE_TYPE_REMOTE or FILE_TYPE_UNKNOWN?  We should
> return ESPIPE only for FILE_TYPE_PIPE and FILE_TYPE_CHAR, then?
>

Done.

PFA a new version of the patch.

Regards,

Juan Jos=C3=A9 Santamar=C3=ADa Flecha

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Wed, Mar 15, 2023 at 5:57=E2=80=AFAM M=
ichael Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">michael@paquier.x=
yz</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margi=
n:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex=
">On Tue, Mar 14, 2023 at 01:26:27PM +0100, Juan Jos=C3=A9 Santamar=C3=ADa =
Flecha wrote:<br>
&gt; As highlighted in [1] fseek() might fail to error even when accessing<=
br>
&gt; unseekable streams.<br>
&gt; <br>
&gt; PFA a patch that checks the file type before the actual fseek(), so on=
ly<br>
&gt; supported calls are made.<br>
<br>
+ * streams, so harden that funcion with our version.<br>
s/funcion/function/.<br></blockquote><div><br></div><div>Done.</div><div><b=
r></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex=
;border-left:1px solid rgb(204,204,204);padding-left:1ex">+extern int=C2=A0=
 =C2=A0 =C2=A0pgfseek64(FILE *stream, pgoff_t offset, int origin);<br>
+extern pgoff_t pgftell64(FILE *stream);<br>
+#define fseeko(stream, offset, origin) pgfseek64(stream, offset, origin)<b=
r>
+#define ftello(stream) pgftell64(stream)<br>
<br>
What about naming the internal wrappers _pgfseeko64() and<br>
_pgftello64(), located in a new file named win32fseek.c?=C2=A0 It may be<br=
>
possible that we would need a similar treatment for fseek(), in the<br>
future, though I don&#39;t see an issue why this would be needed now.<br></=
blockquote><div><br></div><div>Done.</div><div>=C2=A0</div><blockquote clas=
s=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid r=
gb(204,204,204);padding-left:1ex">+=C2=A0 =C2=A0if (GetFileType((HANDLE) _g=
et_osfhandle(_fileno(stream))) !=3D FILE_TYPE_DISK)<br>
+=C2=A0 =C2=A0{<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D ESPIPE;<br>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;<br>
+=C2=A0 =C2=A0}<br>
Shouldn&#39;t there be cases where we should return EINVAL for some of the<=
br>
other types, like FILE_TYPE_REMOTE or FILE_TYPE_UNKNOWN?=C2=A0 We should<br=
>
return ESPIPE only for FILE_TYPE_PIPE and FILE_TYPE_CHAR, then?<br></blockq=
uote><div><br></div><div>Done.</div><div><br></div><div>PFA a new version o=
f the patch.</div><div><br></div><div>Regards,</div><div><br></div><div>Jua=
n Jos=C3=A9 Santamar=C3=ADa Flecha</div></div></div>

--0000000000005120ec05f6ee81e1--
--0000000000005120ee05f6ee81e3
Content-Type: application/octet-stream; 
	name="v2-0001-fix-fseek-detection-of-unseekable-files-for-WIN32.patch"
Content-Disposition: attachment; 
	filename="v2-0001-fix-fseek-detection-of-unseekable-files-for-WIN32.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lf9l98yu0>
X-Attachment-Id: f_lf9l98yu0

RnJvbSA0NWVhMzMzOGUxMGM2YjI5NmRkYzFjZWRiODBjZThlNWYxMDNlNTFkIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBKdWFuIEpvc2UgU2FudGFtYXJpYSBGbGVjaGEgPGp1YW5qby5z
YW50YW1hcmlhQGdtYWlsLmNvbT4KRGF0ZTogV2VkLCAxNSBNYXIgMjAyMyAwNzowNDo0NCAtMDQw
MApTdWJqZWN0OiBbUEFUQ0hdIGZpeCBmc2VlayBkZXRlY3Rpb24gb2YgdW5zZWVrYWJsZSBmaWxl
cyBmb3IgV0lOMzIKCkNhbGxpbmcgZnNlZWsoKSBvbiBhIGhhbmRsZSB0byBhIG5vbi1zZWVraW5n
IGRldmljZSBzdWNoIGFzIGEgcGlwZSBvcgphIGNvbW11bmljYXRpb25zIGRldmljZSBpcyBub3Qg
c3VwcG9ydGVkLCBldmVuIHRob3VnaCB0aGUgZnNlZWsoKSBtYXkKbm90IHJldHVybiBhbiBlcnJv
ciwgc28gaGFyZGVuIHRoYXQgZnVuY2lvbiB3aXRoIG91ciB2ZXJzaW9uLgotLS0KIHNyYy9pbmNs
dWRlL3BvcnQvd2luMzJfcG9ydC5oIHwgIDkgKysrKy0tCiBzcmMvcG9ydC9tZXNvbi5idWlsZCAg
ICAgICAgICB8ICAxICsKIHNyYy9wb3J0L3dpbjMyZnNlZWsuYyAgICAgICAgIHwgNjggKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKwogc3JjL3Rvb2xzL21zdmMvTWt2
Y2J1aWxkLnBtICAgfCAgMSArCiA0IGZpbGVzIGNoYW5nZWQsIDc2IGluc2VydGlvbnMoKyksIDMg
ZGVsZXRpb25zKC0pCiBjcmVhdGUgbW9kZSAxMDA2NDQgc3JjL3BvcnQvd2luMzJmc2Vlay5jCgpk
aWZmIC0tZ2l0IGEvc3JjL2luY2x1ZGUvcG9ydC93aW4zMl9wb3J0LmggYi9zcmMvaW5jbHVkZS9w
b3J0L3dpbjMyX3BvcnQuaAppbmRleCA5NDg4MTk1Li42NjU1M2ZlIDEwMDY0NAotLS0gYS9zcmMv
aW5jbHVkZS9wb3J0L3dpbjMyX3BvcnQuaAorKysgYi9zcmMvaW5jbHVkZS9wb3J0L3dpbjMyX3Bv
cnQuaApAQCAtMjA2LDEzICsyMDYsMTYgQEAgaW50CQkJc2V0aXRpbWVyKGludCB3aGljaCwgY29u
c3Qgc3RydWN0IGl0aW1lcnZhbCAqdmFsdWUsIHN0cnVjdCBpdGltZXJ2YWwgKm92YWwKIAogLyoK
ICAqIFdJTjMyIGRvZXMgbm90IHByb3ZpZGUgNjQtYml0IG9mZl90LCBidXQgZG9lcyBwcm92aWRl
IHRoZSBmdW5jdGlvbnMgb3BlcmF0aW5nCi0gKiB3aXRoIDY0LWJpdCBvZmZzZXRzLgorICogd2l0
aCA2NC1iaXQgb2Zmc2V0cy4gQWxzbywgZnNlZWsoKSBtaWdodCBub3QgZ2l2ZSBhbiBlcnJvciBm
b3IgdW5zZWVrYWJsZQorICogc3RyZWFtcywgc28gaGFyZGVuIHRoYXQgZnVuY3Rpb24gd2l0aCBv
dXIgdmVyc2lvbi4KICAqLwogI2RlZmluZSBwZ29mZl90IF9faW50NjQKIAogI2lmZGVmIF9NU0Nf
VkVSCi0jZGVmaW5lIGZzZWVrbyhzdHJlYW0sIG9mZnNldCwgb3JpZ2luKSBfZnNlZWtpNjQoc3Ry
ZWFtLCBvZmZzZXQsIG9yaWdpbikKLSNkZWZpbmUgZnRlbGxvKHN0cmVhbSkgX2Z0ZWxsaTY0KHN0
cmVhbSkKK2V4dGVybiBpbnQgIAlfcGdmc2Vla282NChGSUxFICpzdHJlYW0sIHBnb2ZmX3Qgb2Zm
c2V0LCBpbnQgb3JpZ2luKTsKK2V4dGVybiBwZ29mZl90CV9wZ2Z0ZWxsbzY0KEZJTEUgKnN0cmVh
bSk7CisjZGVmaW5lIGZzZWVrbyhzdHJlYW0sIG9mZnNldCwgb3JpZ2luKSBfcGdmc2Vla282NChz
dHJlYW0sIG9mZnNldCwgb3JpZ2luKQorI2RlZmluZSBmdGVsbG8oc3RyZWFtKSBfcGdmdGVsbG82
NChzdHJlYW0pCiAjZWxzZQogI2lmbmRlZiBmc2Vla28KICNkZWZpbmUgZnNlZWtvKHN0cmVhbSwg
b2Zmc2V0LCBvcmlnaW4pIGZzZWVrbzY0KHN0cmVhbSwgb2Zmc2V0LCBvcmlnaW4pCmRpZmYgLS1n
aXQgYS9zcmMvcG9ydC9tZXNvbi5idWlsZCBiL3NyYy9wb3J0L21lc29uLmJ1aWxkCmluZGV4IGIx
NzRiMjUuLjNkOGVlYjQgMTAwNjQ0Ci0tLSBhL3NyYy9wb3J0L21lc29uLmJ1aWxkCisrKyBiL3Ny
Yy9wb3J0L21lc29uLmJ1aWxkCkBAIC0zMyw2ICszMyw3IEBAIGlmIGhvc3Rfc3lzdGVtID09ICd3
aW5kb3dzJwogICAgICd3aW4zMmVudi5jJywKICAgICAnd2luMzJlcnJvci5jJywKICAgICAnd2lu
MzJmZGF0YXN5bmMuYycsCisgICAgJ3dpbjMyZnNlZWsuYycsCiAgICAgJ3dpbjMyZ2V0cnVzYWdl
LmMnLAogICAgICd3aW4zMmxpbmsuYycsCiAgICAgJ3dpbjMybnRkbGwuYycsCmRpZmYgLS1naXQg
YS9zcmMvcG9ydC93aW4zMmZzZWVrLmMgYi9zcmMvcG9ydC93aW4zMmZzZWVrLmMKbmV3IGZpbGUg
bW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMC4uNmNmNjJkYgotLS0gL2Rldi9udWxsCisrKyBiL3Ny
Yy9wb3J0L3dpbjMyZnNlZWsuYwpAQCAtMCwwICsxLDY4IEBACisvKi0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0K
KyAqCisgKiB3aW4zMmZzZWVrLmMKKyAqCSAgUmVwbGFjZW1lbnRzIGZvciBmc2Vla28oKSBhbmQg
ZnRlbGxvKCkuCisgKgorICogUG9ydGlvbnMgQ29weXJpZ2h0IChjKSAxOTk2LTIwMjMsIFBvc3Rn
cmVTUUwgR2xvYmFsIERldmVsb3BtZW50IEdyb3VwCisgKgorICogSURFTlRJRklDQVRJT04KKyAq
CSAgc3JjL3BvcnQvd2luMzJmc2Vlay5jCisgKgorICotLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCisgKi8KKwor
I2lmZGVmIEZST05URU5ECisjaW5jbHVkZSAicG9zdGdyZXNfZmUuaCIKKyNlbHNlCisjaW5jbHVk
ZSAicG9zdGdyZXMuaCIKKyNlbmRpZgorCisjaWYgZGVmaW5lZChXSU4zMikgJiYgZGVmaW5lZChf
TVNDX1ZFUikKKworLyoKKyAqIF9wZ2ZzZWVrbzY0CisgKgorICogQ2FsbGluZyBmc2VlaygpIG9u
IGEgaGFuZGxlIHRvIGEgbm9uLXNlZWtpbmcgZGV2aWNlIHN1Y2ggYXMgYSBwaXBlIG9yCisgKiBh
IGNvbW11bmljYXRpb25zIGRldmljZSBpcyBub3Qgc3VwcG9ydGVkLCBldmVuIHRob3VnaCB0aGUg
ZnNlZWsoKSBtYXkKKyAqIG5vdCByZXR1cm4gYW4gZXJyb3IuCisgKi8KK2ludAorX3BnZnNlZWtv
NjQoRklMRSAqc3RyZWFtLCBwZ29mZl90IG9mZnNldCwgaW50IG9yaWdpbikKK3sKKwlEV09SRAkJ
CWZpbGVUeXBlOworCisJZmlsZVR5cGUgPSBHZXRGaWxlVHlwZSgoSEFORExFKSBfZ2V0X29zZmhh
bmRsZShfZmlsZW5vKHN0cmVhbSkpKTsKKworCWlmIChmaWxlVHlwZSA9PSBGSUxFX1RZUEVfRElT
SykKKwkJcmV0dXJuIF9mc2Vla2k2NChzdHJlYW0sIG9mZnNldCwgb3JpZ2luKTsKKwllbHNlIGlm
IChmaWxlVHlwZSA9PSBGSUxFX1RZUEVfQ0hBUiB8fCBmaWxlVHlwZSA9PSBGSUxFX1RZUEVfUElQ
RSkKKwkJZXJybm8gPSBFU1BJUEU7CisJZWxzZQorCQllcnJubyA9IEVJTlZBTDsKKworCXJldHVy
biAtMTsKK30KKworLyoKKyAqIF9wZ2Z0ZWxsbzY0CisgKgorICogU2FtZSBhcyBfcGdmc2Vla282
NCgpLgorICovCitwZ29mZl90CitfcGdmdGVsbG82NChGSUxFICpzdHJlYW0pCit7CisJRFdPUkQJ
CQlmaWxlVHlwZTsKKworCWZpbGVUeXBlID0gR2V0RmlsZVR5cGUoKEhBTkRMRSkgX2dldF9vc2Zo
YW5kbGUoX2ZpbGVubyhzdHJlYW0pKSk7CisKKwlpZiAoZmlsZVR5cGUgPT0gRklMRV9UWVBFX0RJ
U0spCisJCXJldHVybiBfZnRlbGxpNjQoc3RyZWFtKTsKKwllbHNlIGlmIChmaWxlVHlwZSA9PSBG
SUxFX1RZUEVfQ0hBUiB8fCBmaWxlVHlwZSA9PSBGSUxFX1RZUEVfUElQRSkKKwkJZXJybm8gPSBF
U1BJUEU7CisJZWxzZQorCQllcnJubyA9IEVJTlZBTDsKKworCXJldHVybiAtMTsKK30KKworI2Vu
ZGlmCQkJCQkJCS8qIGRlZmluZWQoV0lOMzIpICYmIGRlZmluZWQoX01TQ19WRVIpICovCmRpZmYg
LS1naXQgYS9zcmMvdG9vbHMvbXN2Yy9Na3ZjYnVpbGQucG0gYi9zcmMvdG9vbHMvbXN2Yy9Na3Zj
YnVpbGQucG0KaW5kZXggZjFjOWRkZi4uNjFhY2U1NyAxMDA2NDQKLS0tIGEvc3JjL3Rvb2xzL21z
dmMvTWt2Y2J1aWxkLnBtCisrKyBiL3NyYy90b29scy9tc3ZjL01rdmNidWlsZC5wbQpAQCAtMTEx
LDYgKzExMSw3IEBAIHN1YiBta3ZjYnVpbGQKIAkgIHdpbjMyZGxvcGVuLmMKIAkgIHdpbjMyZW52
LmMgd2luMzJlcnJvci5jCiAJICB3aW4zMmZkYXRhc3luYy5jCisJICB3aW4zMmZzZWVrLmMKIAkg
IHdpbjMyZ2V0cnVzYWdlLmMKIAkgIHdpbjMyZ2V0dGltZW9mZGF5LmMKIAkgIHdpbjMybGluay5j
Ci0tIAoyLjExLjAKCg==
--0000000000005120ee05f6ee81e3--