Thread: [HACKERS][PATCH] adding simple sock check for windows
Hi, I found some missing check for windows int init_slot function in vacuumdb.c in windows SOCKET is unsigned type. so slot->sock < 0 never can be under 0. so this patch just check using slot->sock == PGINVALID_SOCKET Thanks.
Attachment
On Fri, Mar 30, 2018 at 8:10 PM, CharSyam <charsyam@gmail.com> wrote: > Hi, I found some missing check for windows int init_slot function in vacuumdb.c > > in windows > SOCKET is unsigned type. so > > slot->sock < 0 never can be under 0. > > so this patch just check using slot->sock == PGINVALID_SOCKET > - if (slot->sock < 0) + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0) If you are checking for PGINVALID_SOCKET, why do you need the second part of check (slot->sock < 0)? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Hi, Amit, It's good question. I also thought about it. But, I want to leave original code intention. Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET and slot->sock < 0) but if original code want to check (sock < -1) I think it is better to leave that condition. but I think slot->sock == PGINVALID_SOCKET is enough 2018-03-31 14:38 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>: > On Fri, Mar 30, 2018 at 8:10 PM, CharSyam <charsyam@gmail.com> wrote: >> Hi, I found some missing check for windows int init_slot function in vacuumdb.c >> >> in windows >> SOCKET is unsigned type. so >> >> slot->sock < 0 never can be under 0. >> >> so this patch just check using slot->sock == PGINVALID_SOCKET >> > > - if (slot->sock < 0) > + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0) > > If you are checking for PGINVALID_SOCKET, why do you need the second > part of check (slot->sock < 0)? > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com
On Sat, Mar 31, 2018 at 11:42 AM, CharSyam <charsyam@gmail.com> wrote: > Hi, Amit, It's good question. I also thought about it. > > But, I want to leave original code intention. > > Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET > and slot->sock < 0) > > but if original code want to check (sock < -1) > If you see the code of PQsocket, then that won't be possible. > I think it is better to leave that condition. > > but I think slot->sock == PGINVALID_SOCKET is enough > It is up to you, but I don't see any reason to retain the old check. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit, I agree with you. I changed my patch :) to remove old check. 2018-03-31 15:17 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>: > On Sat, Mar 31, 2018 at 11:42 AM, CharSyam <charsyam@gmail.com> wrote: >> Hi, Amit, It's good question. I also thought about it. >> >> But, I want to leave original code intention. >> >> Actually I think there is no case ( slot->sock is not PGINVALID_SOCKET >> and slot->sock < 0) >> >> but if original code want to check (sock < -1) >> > > If you see the code of PQsocket, then that won't be possible. > > >> I think it is better to leave that condition. >> >> but I think slot->sock == PGINVALID_SOCKET is enough >> > > It is up to you, but I don't see any reason to retain the old check. > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com
Attachment
On Sat, Mar 31, 2018 at 12:05 PM, CharSyam <charsyam@gmail.com> wrote: > Amit, I agree with you. > > I changed my patch :) to remove old check. > - if (slot->sock < 0) + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0) I still see the same check. I think by mistake you have attached old patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Thanks Amit. I had a mistake. Thank you again to point it out :) 2018-03-31 19:33 GMT+09:00 Amit Kapila <amit.kapila16@gmail.com>: > On Sat, Mar 31, 2018 at 12:05 PM, CharSyam <charsyam@gmail.com> wrote: >> Amit, I agree with you. >> >> I changed my patch :) to remove old check. >> > > - if (slot->sock < 0) > + if (slot->sock == PGINVALID_SOCKET || slot->sock < 0) > > I still see the same check. I think by mistake you have attached old patch. > > -- > With Regards, > Amit Kapila. > EnterpriseDB: http://www.enterprisedb.com
Attachment
On Sat, Mar 31, 2018 at 6:08 PM, CharSyam <charsyam@gmail.com> wrote: > Thanks Amit. > I had a mistake. Thank you again to point it out :) > Thanks, your new patch looks good. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
CharSyam <charsyam@gmail.com> writes:
> [ simple_check.patch ]
This is a good catch. However, it looks to me like the reason nobody
has noticed a problem here is that actually, this error test is useless;
we can never get here with a bad connection, because connectDatabase
would have failed. I'm inclined to think we should just delete the whole
if-statement, instead.
BTW, I tried cross-checking for other errors of this ilk by doing
diff --git a/src/include/port.h b/src/include/port.h
index a514ab758b..9ba6a0df79 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -28,9 +28,9 @@
/* socket has a different definition on WIN32 */
#ifndef WIN32
-typedef int pgsocket;
+typedef unsigned int pgsocket;
-#define PGINVALID_SOCKET (-1)
+#define PGINVALID_SOCKET ((pgsocket) -1)
#else
typedef SOCKET pgsocket;
and then compiling with a compiler that would warn about "unsigned int < 0"
tests (I used Apple's current compiler, but probably any recent clang
would do as well). It found this case and no others, which is good.
This is not a 100% cross-check, because I don't think it would notice
"unsigned int == -1" which is another way somebody might misspell the
test, and it definitely would not catch cases where somebody stored a
socket identifier in plain int instead of pgsocket. But it's better
than nothing...
regards, tom lane
Hi, Tom, Thank you for your review.
so Do you think it is better to remove if statement like below
diff --git src/bin/scripts/vacuumdb.c src/bin/scripts/vacuumdb.c
index 887fa48fbd..243d842d06 100644
--- src/bin/scripts/vacuumdb.c
+++ src/bin/scripts/vacuumdb.c
@@ -947,13 +947,6 @@ init_slot(ParallelSlot *slot, PGconn *conn, const
char *progname)
slot->connection = conn;
slot->isFree = true;
slot->sock = PQsocket(conn);
-
- if (slot->sock < 0)
- {
- fprintf(stderr, _("%s: invalid socket: %s"), progname,
- PQerrorMessage(conn));
- exit(1);
- }
}
static void
2018-04-01 2:16 GMT+09:00 Tom Lane <tgl@sss.pgh.pa.us>:
> CharSyam <charsyam@gmail.com> writes:
>> [ simple_check.patch ]
>
> This is a good catch. However, it looks to me like the reason nobody
> has noticed a problem here is that actually, this error test is useless;
> we can never get here with a bad connection, because connectDatabase
> would have failed. I'm inclined to think we should just delete the whole
> if-statement, instead.
>
> BTW, I tried cross-checking for other errors of this ilk by doing
>
> diff --git a/src/include/port.h b/src/include/port.h
> index a514ab758b..9ba6a0df79 100644
> --- a/src/include/port.h
> +++ b/src/include/port.h
> @@ -28,9 +28,9 @@
>
> /* socket has a different definition on WIN32 */
> #ifndef WIN32
> -typedef int pgsocket;
> +typedef unsigned int pgsocket;
>
> -#define PGINVALID_SOCKET (-1)
> +#define PGINVALID_SOCKET ((pgsocket) -1)
> #else
> typedef SOCKET pgsocket;
>
> and then compiling with a compiler that would warn about "unsigned int < 0"
> tests (I used Apple's current compiler, but probably any recent clang
> would do as well). It found this case and no others, which is good.
> This is not a 100% cross-check, because I don't think it would notice
> "unsigned int == -1" which is another way somebody might misspell the
> test, and it definitely would not catch cases where somebody stored a
> socket identifier in plain int instead of pgsocket. But it's better
> than nothing...
>
> regards, tom lane
Attachment
... Oh, just to make things even more fun, PQsocket() returns int, not
pgsocket; see its header comment. Therefore, that test is correctly
coded as-is (though it's still useless), and the real problem is that
ParallelSlot.sock ought to be declared int not pgsocket. If you look
around at our other places that read PQsocket() and use its result in
select() masks, they uniformly use int variables, and I think they're
right.
Actually, the more I look at this code, the more problems I'm finding.
The wait-for-a-free-connection code is just broken on its face, because
it supposes it can retry select() waits without reinitializing the
FD_SET. The loop in vacuum_one_database() uses "conn" to call
prepare_vacuum_command() without any thought as to whether that is a
free connection --- which, typically, it isn't. It looks to me like
libpq silently copes with this silliness, just waiting for the active
command to finish and then doing what it's told, but the net result
is that supposedly-concurrent vacuums get serialized.
regards, tom lane
On Sat, Mar 31, 2018 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > ... Oh, just to make things even more fun, PQsocket() returns int, not > pgsocket; see its header comment. Therefore, that test is correctly > coded as-is (though it's still useless), and the real problem is that > ParallelSlot.sock ought to be declared int not pgsocket. If you look > around at our other places that read PQsocket() and use its result in > select() masks, they uniformly use int variables, and I think they're > right. > There is one other place where we are using pgsocket, see walrcv_receive, but we are using Assert in that place. I think it would be better if the output of PQsocket() can be consistently used everywhere. If there is no particular restriction, then it will be good to make it as 'int' everywhere. > Actually, the more I look at this code, the more problems I'm finding. > The wait-for-a-free-connection code is just broken on its face, because > it supposes it can retry select() waits without reinitializing the > FD_SET. The loop in vacuum_one_database() uses "conn" to call > prepare_vacuum_command() without any thought as to whether that is a > free connection --- which, typically, it isn't. It looks to me like > libpq silently copes with this silliness, just waiting for the active > command to finish and then doing what it's told, but the net result > is that supposedly-concurrent vacuums get serialized. > I think it would have been better if this code would have found the free_slot before preparing the command and then used the connection from free slot. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sun, Apr 1, 2018 at 8:59 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sat, Mar 31, 2018 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> ... Oh, just to make things even more fun, PQsocket() returns int, not >> pgsocket; see its header comment. Therefore, that test is correctly >> coded as-is (though it's still useless), and the real problem is that >> ParallelSlot.sock ought to be declared int not pgsocket. If you look >> around at our other places that read PQsocket() and use its result in >> select() masks, they uniformly use int variables, and I think they're >> right. >> > > There is one other place where we are using pgsocket, see > walrcv_receive, but we are using Assert in that place. I think it > would be better if the output of PQsocket() can be consistently used > everywhere. If there is no particular restriction, then it will be > good to make it as 'int' everywhere. > > >> Actually, the more I look at this code, the more problems I'm finding. >> The wait-for-a-free-connection code is just broken on its face, because >> it supposes it can retry select() waits without reinitializing the >> FD_SET. The loop in vacuum_one_database() uses "conn" to call >> prepare_vacuum_command() without any thought as to whether that is a >> free connection --- which, typically, it isn't. It looks to me like >> libpq silently copes with this silliness, just waiting for the active >> command to finish and then doing what it's told, but the net result >> is that supposedly-concurrent vacuums get serialized. >> > > I think it would have been better if this code would have found the > free_slot before preparing the command and then used the connection > from free slot. > oops, I just saw that you have already pushed a fix for it. I am not sure if we should try to do anything about walrcv_receive's output still uses pgsocket instead of int as the usage in itself doesn't have any problem. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
> oops, I just saw that you have already pushed a fix for it. I am not
> sure if we should try to do anything about walrcv_receive's output
> still uses pgsocket instead of int as the usage in itself doesn't have
> any problem.
I see a few places where we're still assigning PQsocket's result to a
pgsocket variable, but none of them are worrying about the possibility
that it's not a valid socket, and I'm not sure they need to.
regards, tom lane