Re: Initdb failure
On Tue, Jul 30, 2019 at 03:30:12PM -0400, Tom Lane wrote: > On the whole though, I don't have a problem with the "do nothing" > answer. There's no security risk here, and no issue that seems > likely to arise in actual use cases rather than try-to-break-it > test scripts. +1. -- Michael signature.asc Description: PGP signature
Re: Initdb failure
Robert Haas writes: > Agreed, but I think we should just do nothing. To actually fix this > in general, we'd have to get rid of every instance of MAXPGPATH in the > source tree: > [rhaas pgsql]$ git grep MAXPGPATH | wc -l > 611 I don't think it'd really be necessary to go that far. One of the reasons we chdir to the data directory at postmaster start is so that (pretty nearly) all filenames that backends deal with are relative pathnames of very predictable, short lengths. So a lot of those MAXPGPATH uses are probably perfectly safe, indeed likely overkill. However, identifying which ones are not safe would still take looking at every use case, so I agree there'd be a lot of work here. Would there be any value in teaching initdb to do something similar, ie chdir to the parent of the target datadir location? Then the set of places in initdb that have to deal with long pathnames would be pretty well constrained. On the whole though, I don't have a problem with the "do nothing" answer. There's no security risk here, and no issue that seems likely to arise in actual use cases rather than try-to-break-it test scripts. regards, tom lane
Re: Initdb failure
On Sat, Jul 27, 2019 at 2:22 AM Peter Eisentraut wrote: > I think if you want to make this more robust, get rid of the fixed-size > array, use dynamic allocation with PQExpBuffer, and let the operating > system complain if it doesn't like the directory name length. Agreed, but I think we should just do nothing. To actually fix this in general, we'd have to get rid of every instance of MAXPGPATH in the source tree: [rhaas pgsql]$ git grep MAXPGPATH | wc -l 611 If somebody feels motivated to spend that amount of effort improving this, I will stand back and cheer from the sidelines, but that's gonna be a LOT of work for a problem that, as Tom says, is probably not really affecting very many people. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Initdb failure
On 2019-07-25 17:09, Rafia Sabih wrote: > But on the other hand emitting the right error message atleast would > be good for the sake of correctness if nothing else. But yes that > definitely should be weighed against what is the effort required for > this. I think if you want to make this more robust, get rid of the fixed-size array, use dynamic allocation with PQExpBuffer, and let the operating system complain if it doesn't like the directory name length. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Initdb failure
On Thu, Jul 25, 2019 at 8:39 PM Rafia Sabih wrote: > > On Thu, 25 Jul 2019 at 16:44, Tom Lane wrote: > > > > Rafia Sabih writes: > > > On Thu, 25 Jul 2019 at 13:50, vignesh C wrote: > > >>> Initdb fails when following path is provided as input: > > >>> datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ > > > > > Now that you say this, it does make sense to atleast inform about the > > > correct error and that too earlier. Something like the attached patch > > > would make sense. > > > > I am not terribly excited about putting effort into this at all, because > > I don't think that any actual user anywhere will ever get any benefit. > > The proposed test case is just silly. > > That I totally agree upon! > But on the other hand emitting the right error message atleast would > be good for the sake of correctness if nothing else. But yes that > definitely should be weighed against what is the effort required for > this. > Thanks Tom for your opinion. Thanks Rafia for your thoughts and effort in making the patch. I'm not sure if we are planning to fix this. If we are planning to fix, one suggestion from my side we can choose a safe length which would include the subdirectories and file paths. I think one of these will be the longest: base/database_oid/tables pg_wal/archive_status/ pg_wal/archive_file Fix can be something like: MAXPGPATH - (LONGEST_PATH_FROM_ABOVE) Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Re: Initdb failure
On Thu, 25 Jul 2019 at 16:44, Tom Lane wrote: > > Rafia Sabih writes: > > On Thu, 25 Jul 2019 at 13:50, vignesh C wrote: > >>> Initdb fails when following path is provided as input: > >>> datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ > > > Now that you say this, it does make sense to atleast inform about the > > correct error and that too earlier. Something like the attached patch > > would make sense. > > I am not terribly excited about putting effort into this at all, because > I don't think that any actual user anywhere will ever get any benefit. > The proposed test case is just silly. That I totally agree upon! But on the other hand emitting the right error message atleast would be good for the sake of correctness if nothing else. But yes that definitely should be weighed against what is the effort required for this. -- Regards, Rafia Sabih
Re: Initdb failure
Rafia Sabih writes: > On Thu, 25 Jul 2019 at 13:50, vignesh C wrote: >>> Initdb fails when following path is provided as input: >>> datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ > Now that you say this, it does make sense to atleast inform about the > correct error and that too earlier. Something like the attached patch > would make sense. I am not terribly excited about putting effort into this at all, because I don't think that any actual user anywhere will ever get any benefit. The proposed test case is just silly. However, if we are going to put effort into it, it needs to be more than this. First off, what is the actual failure point? (It's surely less than MAXPGPATH, because we tend to append subdirectory/file names onto whatever is given.) Second, what of absolute versus relative paths? If converting the given path to absolute makes it exceed MAXPGPATH, is that a problem? regards, tom lane
Re: Initdb failure
On Thu, 25 Jul 2019 at 13:50, vignesh C wrote:
>
> On Thu, Jul 25, 2019 at 4:52 PM Rafia Sabih wrote:
> >
> > On Thu, 25 Jul 2019 at 07:39, vignesh C wrote:
> > >
> > > Hi,
> > >
> > > Initdb fails when following path is provided as input:
> > > datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/
> > >
> > > Also the cleanup also tends to fail in the cleanup path.
> > >
> > > Could be something to do with path handling.
> > This is because the value of MAXPGPATH is 1024 and the path you are
> > providing is more than that. Hence, when it is trying to read
> > PG_VERSION in ValidatePgVersion it is going to a wrong path with just
> > 1024 characters.
> >
>
> The error occurs at a very later point after performing the initial
> work like creating directory. I'm thinking we should check this in
> the beginning and throw the error message at the beginning and exit
> cleanly.
>
Now that you say this, it does make sense to atleast inform about the
correct error and that too earlier. Something like the attached patch
would make sense.
--
Regards,
Rafia Sabih
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 04d77ad700..3af11b365b 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2726,6 +2726,12 @@ create_data_directory(void)
{
int ret;
+ if (strlen(pg_data) > MAXPGPATH)
+ {
+ pg_log_error("too long directory name\"%s\": %m", pg_data);
+ exit(1);
+ }
+
switch ((ret = pg_check_dir(pg_data)))
{
case 0:
Re: Initdb failure
On Thu, Jul 25, 2019 at 4:52 PM Rafia Sabih wrote: > > On Thu, 25 Jul 2019 at 07:39, vignesh C wrote: > > > > Hi, > > > > Initdb fails when following path is provided as input: > > datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ > > > > Also the cleanup also tends to fail in the cleanup path. > > > > Could be something to do with path handling. > This is because the value of MAXPGPATH is 1024 and the path you are > providing is more than that. Hence, when it is trying to read > PG_VERSION in ValidatePgVersion it is going to a wrong path with just > 1024 characters. > The error occurs at a very later point after performing the initial work like creating directory. I'm thinking we should check this in the beginning and throw the error message at the beginning and exit cleanly. > > > I'm not sure if this is already known. > I am also not sure if it is known or intentional. On the other hand I > also don't know if it is practical to give such long names for > database directory anyway. > Usually they will not be using such long path, but this is one of the odd scenarios. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Re: Initdb failure
On Thu, 25 Jul 2019 at 07:39, vignesh C wrote: > > Hi, > > Initdb fails when following path is provided as input: > datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ > > Also the cleanup also tends to fail in the cleanup path. > > Could be something to do with path handling. This is because the value of MAXPGPATH is 1024 and the path you are providing is more than that. Hence, when it is trying to read PG_VERSION in ValidatePgVersion it is going to a wrong path with just 1024 characters. > I'm not sure if this is already known. I am also not sure if it is known or intentional. On the other hand I also don't know if it is practical to give such long names for database directory anyway. -- Regards, Rafia Sabih
Initdb failure
Hi, Initdb fails when following path is provided as input: datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/datasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafddsdatasadfasfdsafdds/ Also the cleanup also tends to fail in the cleanup path. Could be something to do with path handling. I'm not sure if this is already known. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
