Обсуждение: BUG #7811: strlen(NULL) cause psql crash
The following bug has been logged on the website:
Bug reference: 7811
Logged by: Meng Qingzhong
Email address: 1584171677@qq.com
PostgreSQL version: 9.2.2
Operating system: Windows 7
Description: =
I find a bug this time really and I have fixed it. The bug is in psql, my
oprating system is Windows 7, and the IDE is Visual Studio 2010. The version
of postgreSQL is 9.2.2
I give you a description about how to trigger this bug first=EF=BC=9A
(1) start the server with the command "postgres -D pgdata"
(2) start the client with the command "psql"
(3) close the server
(4) execute a query from the client "slect *from t; ". At this
time, the client detected that it lost the connection with the server.
(5) execute the following command from the client "\?", then the
client will crash.
I have found the reason which caused that.
(1) When the client execute "slect *from t; ", it execute the
function "ResetCancelConn()" at line 364 in src\bin\psql\common.c ,and the
function set pset.db to NULL.
(2) When the client execute "\?", it execute the function fprintf
at line 254 in help.c. The value returned by PQdb(pset.db) is an argument of
fprintf, and at this time PQdb returned NULL.
(3) This NULL was finally passed to strlen at line 779 in
snprintf.c through several simple fuction calls, so psql crashed.
=
I hava fixed the bug in the following way which may be not the best:
(1) add a string named strofnull, and in the function "dopr" in
file src\port\snprintf.c
char *strofnull=3D"(null)";
(2) add an if statment before calling fmtstr at about line 720 in
file src\port\snprintf.c
if (strvalue=3D=3DNULL)
{
strvalue=3Dstrofnull;
}
On 15.01.2013 16:18, 1584171677@qq.com wrote:
> I give you a description about how to trigger this bug first=EF=BC=
=9A
> (1) start the server with the command "postgres -D pgdata"
> (2) start the client with the command "psql"
> (3) close the server
> (4) execute a query from the client "slect *from t; ". At th=
is
> time, the client detected that it lost the connection with the server.
> (5) execute the following command from the client "\?", then=
the
> client will crash.
>
> I have found the reason which caused that.
>
> (1) When the client execute "slect *from t; ", it execute th=
e
> function "ResetCancelConn()" at line 364 in src\bin\psql\common.c ,and =
the
> function set pset.db to NULL.
> (2) When the client execute "\?", it execute the function fp=
rintf
> at line 254 in help.c. The value returned by PQdb(pset.db) is an argume=
nt of
> fprintf, and at this time PQdb returned NULL.
> (3) This NULL was finally passed to strlen at line 779 in
> snprintf.c through several simple fuction calls, so psql crashed.
Thanks for the report and debugging!
> I hava fixed the bug in the following way which may be not the b=
est:
>
> (1) add a string named strofnull, and in the function "dopr"=
in
> file src\port\snprintf.c
> char *strofnull=3D"(null)";
>
> (2) add an if statment before calling fmtstr at about line 7=
20 in
> file src\port\snprintf.c
>
> if (strvalue=3D=3DNULL)
> {
> strvalue=3Dstrofnull;
> }
That'd change the behavior of all sprintf calls, not sure we want to go=20
there. Might not be a bad idea to avoid crashes if there are more bugs=20
like this, but one really should not pass NULL to sprintf to begin with.=20
I committed a local fix to help.c to print "none" as the database name=20
when not connected.
- Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> That'd change the behavior of all sprintf calls, not sure we want to go
> there.
I don't think so; especially since the proposed fix doesn't help on
systems where we are using the native printf.
> I committed a local fix to help.c to print "none" as the database name
> when not connected.
I think that patch could use more thought. As is, it will print
connect to new database (currently "none")
which to me is claiming that we are connected to a database whose name
is "none". The quotes should not be used in this situation, and in
fact it would be better for translatability if the whole message text
were specialized to this case. I'd like to see it reading more like
connect to new database (currently no connection)
regards, tom lane
On 15.01.2013 20:10, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >> I committed a local fix to help.c to print "none" as the database name >> when not connected. > > I think that patch could use more thought. As is, it will print > > connect to new database (currently "none") > > which to me is claiming that we are connected to a database whose name > is "none". The quotes should not be used in this situation, and in > fact it would be better for translatability if the whole message text > were specialized to this case. I'd like to see it reading more like > > connect to new database (currently no connection) Hmm, that'd introduce a new quite visible string into back-branches that needs to be translated, which I think we try to avoid. I'll change that in master, but back-branches? - Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 15.01.2013 20:10, Tom Lane wrote:
>> I think that patch could use more thought. As is, it will print
>> connect to new database (currently "none")
>> which to me is claiming that we are connected to a database whose name
>> is "none". The quotes should not be used in this situation, and in
>> fact it would be better for translatability if the whole message text
>> were specialized to this case. I'd like to see it reading more like
>> connect to new database (currently no connection)
> Hmm, that'd introduce a new quite visible string into back-branches that
> needs to be translated, which I think we try to avoid.
But you already introduced "none" as a stand-alone (and probably almost
untranslatable without context) string. That's better?
regards, tom lane
On 15.01.2013 20:29, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >> On 15.01.2013 20:10, Tom Lane wrote: >>> I think that patch could use more thought. As is, it will print >>> connect to new database (currently "none") >>> which to me is claiming that we are connected to a database whose name >>> is "none". The quotes should not be used in this situation, and in >>> fact it would be better for translatability if the whole message text >>> were specialized to this case. I'd like to see it reading more like >>> connect to new database (currently no connection) > >> Hmm, that'd introduce a new quite visible string into back-branches that >> needs to be translated, which I think we try to avoid. > > But you already introduced "none" as a stand-alone (and probably almost > untranslatable without context) string. That's better? I figured it would be. One little untranslated string in parens, versus the whole is untranslated. I'm happy to change it if you feel otherwise, though, I don't feel strongly about it myself. - Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 15.01.2013 20:29, Tom Lane wrote:
>> But you already introduced "none" as a stand-alone (and probably almost
>> untranslatable without context) string. That's better?
> I figured it would be. One little untranslated string in parens, versus
> the whole is untranslated. I'm happy to change it if you feel otherwise,
> though, I don't feel strongly about it myself.
Well, I shouldn't be opining too strongly on translatability issues.
Other people would have much-better-qualified opinions as to how well
it'll read if "none" has to be translated by itself.
But as to the behavior when the new message hasn't been translated yet:
the only case where anyone would see the untranslated message is if they
were in fact not connected, which we know is a seldom-exercised corner
case (else this bug would've been noticed long ago). So it might not be
worth arguing about. But ISTM that somebody whose English was weak
might not grasp that "none" (untranslated) wasn't meant to be a name.
regards, tom lane
On 15.01.2013 21:13, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >> On 15.01.2013 20:29, Tom Lane wrote: >>> But you already introduced "none" as a stand-alone (and probably almost >>> untranslatable without context) string. That's better? > >> I figured it would be. One little untranslated string in parens, versus >> the whole is untranslated. I'm happy to change it if you feel otherwise, >> though, I don't feel strongly about it myself. > > Well, I shouldn't be opining too strongly on translatability issues. > Other people would have much-better-qualified opinions as to how well > it'll read if "none" has to be translated by itself. > > But as to the behavior when the new message hasn't been translated yet: > the only case where anyone would see the untranslated message is if they > were in fact not connected, which we know is a seldom-exercised corner > case (else this bug would've been noticed long ago). So it might not be > worth arguing about. But ISTM that somebody whose English was weak > might not grasp that "none" (untranslated) wasn't meant to be a name. Hmm, I wonder if an empty string would be better? It'd look a bit odd, but at least it would not mislead you to think you're connected to a database called "none". - Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> Hmm, I wonder if an empty string would be better? It'd look a bit odd,
> but at least it would not mislead you to think you're connected to a
> database called "none".
Works for me as a back-branch fix, especially since then we're adding
zero new strings to the back branches.
regards, tom lane