Removing the fixed-size buffer restriction in hba.c
We got a complaint at [1]/messages/by-id/PH0PR04MB8294A4C5A65D9D492CBBD349C002A@PH0PR04MB8294.namprd04.prod.outlook.com about how a not-so-unreasonable LDAP
configuration can hit the "authentication file token too long,
skipping" error case in hba.c's next_token(). I think we've
seen similar complaints before, although a desultory archives
search didn't turn one up.
A minimum-change response would be to increase the MAX_TOKEN
constant from 256 to (say) 1K or 10K. But it wouldn't be all
that hard to replace the fixed-size buffer with a StringInfo,
as attached.
Given the infrequency of complaints, I'm inclined to apply
the more thorough fix only in HEAD, and to just raise MAX_TOKEN
in the back branches. Thoughts?
regards, tom lane
[1]: /messages/by-id/PH0PR04MB8294A4C5A65D9D492CBBD349C002A@PH0PR04MB8294.namprd04.prod.outlook.com
Attachments:
v1-remove-hba-token-length-limit.patchtext/x-diff; charset=us-ascii; name=v1-remove-hba-token-length-limit.patchDownload+23-41
On Mon, Jul 24, 2023 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We got a complaint at [1] about how a not-so-unreasonable LDAP
configuration can hit the "authentication file token too long,
skipping" error case in hba.c's next_token(). I think we've
seen similar complaints before, although a desultory archives
search didn't turn one up.A minimum-change response would be to increase the MAX_TOKEN
constant from 256 to (say) 1K or 10K. But it wouldn't be all
that hard to replace the fixed-size buffer with a StringInfo,
as attached.
+1 for replacing it with StringInfo. And the patch LGTM!
Given the infrequency of complaints, I'm inclined to apply
the more thorough fix only in HEAD, and to just raise MAX_TOKEN
in the back branches. Thoughts?
It makes sense to change it only in HEAD.
Regards,
--
Fabrízio de Royes Mello
On Mon, Jul 24, 2023 at 03:07:07PM -0300, Fabrízio de Royes Mello wrote:
On Mon, Jul 24, 2023 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We got a complaint at [1] about how a not-so-unreasonable LDAP
configuration can hit the "authentication file token too long,
skipping" error case in hba.c's next_token(). I think we've
seen similar complaints before, although a desultory archives
search didn't turn one up.A minimum-change response would be to increase the MAX_TOKEN
constant from 256 to (say) 1K or 10K. But it wouldn't be all
that hard to replace the fixed-size buffer with a StringInfo,
as attached.+1 for replacing it with StringInfo. And the patch LGTM!
I'd suggest noting that next_token() clears buf, but otherwise it looks
reasonable to me, too.
Given the infrequency of complaints, I'm inclined to apply
the more thorough fix only in HEAD, and to just raise MAX_TOKEN
in the back branches. Thoughts?It makes sense to change it only in HEAD.
I wouldn't be opposed to back-patching the more thorough fix, but I don't
feel too strongly about it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Mon, Jul 24, 2023 at 03:58:31PM -0700, Nathan Bossart wrote:
On Mon, Jul 24, 2023 at 03:07:07PM -0300, Fabrízio de Royes Mello wrote:
Given the infrequency of complaints, I'm inclined to apply
the more thorough fix only in HEAD, and to just raise MAX_TOKEN
in the back branches. Thoughts?It makes sense to change it only in HEAD.
I wouldn't be opposed to back-patching the more thorough fix, but I don't
feel too strongly about it.
- * The token, if any, is returned at *buf (a buffer of size bufsz), and
+ * The token, if any, is returned into *buf, and
* *lineptr is advanced past the token.The comment indentation is a bit off here.
* In event of an error, log a message at ereport level elevel, and also
- * set *err_msg to a string describing the error. Currently the only
- * possible error is token too long for buf.
+ * set *err_msg to a string describing the error. Currently no error
+ * conditions are defined.I find the choice to keep err_msg in next_token() a bit confusing in
next_field_expand(). If no errors are possible, why not just get rid
of it?
FWIW, I don't feel strongly about backpatching that either, so for the
back branches I'd choose to bump up the token size limit and call it a
day.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
I find the choice to keep err_msg in next_token() a bit confusing in
next_field_expand(). If no errors are possible, why not just get rid
of it?
Yeah, I dithered about that. I felt like removing it only to put it
back later would be silly, but then again maybe there won't ever be
a need to put it back. I'm OK with removing it if there's not
objections.
regards, tom lane
On Mon, Jul 24, 2023 at 08:21:54PM -0400, Tom Lane wrote:
Yeah, I dithered about that. I felt like removing it only to put it
back later would be silly, but then again maybe there won't ever be
a need to put it back. I'm OK with removing it if there's not
objections.
Seeing what this code does, the odds of needing an err_msg seem rather
low to me, so I would just remove it for now for the sake of clarity.
It can always be added later if need be.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
Seeing what this code does, the odds of needing an err_msg seem rather
low to me, so I would just remove it for now for the sake of clarity.
It can always be added later if need be.
Done that way. Thanks for looking at it!
regards, tom lane