Re: "current directory" in a server error message

2023-03-16 Thread Kyotaro Horiguchi
At Thu, 16 Mar 2023 12:05:32 -0400, Tom Lane  wrote in 
> Kyotaro Horiguchi  writes:
> > At Thu, 16 Mar 2023 09:32:05 +0530, Bharath Rupireddy 
> >  wrote in 
> >> On Thu, Mar 16, 2023 at 7:47 AM Kyotaro Horiguchi
> >>  wrote:
> >>> Thus I think that the message should read "path must be in or below
> >>> the data directory" instead.
> 
> >> BTW, adminpack too has the same error message.
> 
> > I somehow dropped them. Thanks for pointing.
> 
> Agreed, this is an improvement.  I fixed adminpack too and pushed it.

Oh, thanks for committing this.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Re: "current directory" in a server error message

2023-03-16 Thread Tom Lane
Kyotaro Horiguchi  writes:
> At Thu, 16 Mar 2023 09:32:05 +0530, Bharath Rupireddy 
>  wrote in 
>> On Thu, Mar 16, 2023 at 7:47 AM Kyotaro Horiguchi
>>  wrote:
>>> Thus I think that the message should read "path must be in or below
>>> the data directory" instead.

>> BTW, adminpack too has the same error message.

> I somehow dropped them. Thanks for pointing.

Agreed, this is an improvement.  I fixed adminpack too and pushed it.

regards, tom lane




Re: "current directory" in a server error message

2023-03-16 Thread Kyotaro Horiguchi
At Thu, 16 Mar 2023 09:32:05 +0530, Bharath Rupireddy 
 wrote in 
> On Thu, Mar 16, 2023 at 7:47 AM Kyotaro Horiguchi
>  wrote:
> > Thus I think that the message should read "path must be in or below
> > the data directory" instead.
> >
> > What do you think about making this change?
> 
> Well yes. As far as postgres processes are concerned their working
> directory is set to data directory by the postmaster in
> ChangeToDataDir() and all the children will inherit that setting. So,
> I see nothing wrong in being explicit about it in the error messages.

Yeah, you're right.

> BTW, adminpack too has the same error message.

I somehow dropped them. Thanks for pointing.

> FWIW, here are the steps to generate the error:
> create role foo with nosuperuser;
> grant execute on function pg_ls_dir(text) to foo;
> set role foo;
> select * from pg_ls_dir('..');

Oh, thank you for the clarification about the reproduction method.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


Re: "current directory" in a server error message

2023-03-15 Thread Bharath Rupireddy
On Thu, Mar 16, 2023 at 7:47 AM Kyotaro Horiguchi
 wrote:
>
> Hello.
>
> When I ran pg_ls_dir('..'), the error message I received was somewhat
> difficult to understand.
>
> postgres=> select * from pg_ls_dir('..');
> ERROR:  path must be in or below the current directory
>
> As far as I know the concept of a "current directory" doesn't apply to
> the server side. In fact, the function comment for
> convert_and_check_filename explicitly states that:
>
> > * Filename may be absolute or relative to the DataDir
>
> Thus I think that the message should read "path must be in or below
> the data directory" instead.
>
> What do you think about making this change?

Well yes. As far as postgres processes are concerned their working
directory is set to data directory by the postmaster in
ChangeToDataDir() and all the children will inherit that setting. So,
I see nothing wrong in being explicit about it in the error messages.

BTW, adminpack too has the same error message.

FWIW, here are the steps to generate the error:
create role foo with nosuperuser;
grant execute on function pg_ls_dir(text) to foo;
set role foo;
select * from pg_ls_dir('..');

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com




"current directory" in a server error message

2023-03-15 Thread Kyotaro Horiguchi
Hello.

When I ran pg_ls_dir('..'), the error message I received was somewhat
difficult to understand.

postgres=> select * from pg_ls_dir('..');
ERROR:  path must be in or below the current directory

As far as I know the concept of a "current directory" doesn't apply to
the server side. In fact, the function comment for
convert_and_check_filename explicitly states that:

> * Filename may be absolute or relative to the DataDir

Thus I think that the message should read "path must be in or below
the data directory" instead.

What do you think about making this change?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 75588eebb3..f281ce9806 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -86,7 +86,7 @@ convert_and_check_filename(text *arg)
 	else if (!path_is_relative_and_below_cwd(filename))
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("path must be in or below the current directory")));
+ errmsg("path must be in or below the data directory")));
 
 	return filename;
 }