strncpy is not a safe version of strcpy
Hi All,
As a bit of a background task, over the past few days I've been analysing
the uses of strncpy in the code just to try and validate if it is the right
function to be using. I've already seen quite a few places where their
usage is wrongly assumed.
As many of you will know and maybe some of you have forgotten that strncpy
is not a safe version of strcpy. It is also quite an inefficient way to
copy a string to another buffer as strncpy will 0 out any space that
happens to remain in the buffer. If there is no space left after the copy
then the buffer won't end with a 0.
It is likely far better explained here -->
http://www.courtesan.com/todd/papers/strlcpy.html
For example , the following 2 lines in jsonfuncs.c
memset(name, 0, NAMEDATALEN);
strncpy(name, fname, NAMEDATALEN);
The memset here is redundant as strncpy will null the remaining buffer.
This example is not dangerous, but it does highlight that there's code
that's made the final cut which made this wrong assumption about strncpy.
I was not going to bring this to light until I had done some more analysis,
but there was just a commit which added a usage of strncpy that really
looks like it should be a strlcpy.
I'll continue with my analysis, but perhaps posting this early will bring
something to light which I've not yet realised.
Regards
David Rowley
On 15 Listopad 2013, 0:07, David Rowley wrote:
Hi All,
As a bit of a background task, over the past few days I've been analysing
the uses of strncpy in the code just to try and validate if it is the
right
function to be using. I've already seen quite a few places where their
usage is wrongly assumed.As many of you will know and maybe some of you have forgotten that strncpy
is not a safe version of strcpy. It is also quite an inefficient way to
copy a string to another buffer as strncpy will 0 out any space that
happens to remain in the buffer. If there is no space left after the copy
then the buffer won't end with a 0.It is likely far better explained here -->
http://www.courtesan.com/todd/papers/strlcpy.htmlFor example , the following 2 lines in jsonfuncs.c
memset(name, 0, NAMEDATALEN);
strncpy(name, fname, NAMEDATALEN);
Be careful with 'Name' data type - it's not just a simple string buffer.
AFAIK it needs to work with hashing etc. so the zeroing is actually needed
here to make sure two values produce the same result. At least that's how
I understand the code after a quick check - for example this is from the
same jsonfuncs.c you mentioned:
memset(fname, 0, NAMEDATALEN);
strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);
So the zeroing is on purpose, although if strncpy does that then the
memset is probably superflous. Either people do that because of habit /
copy'n'paste, or maybe there are supported platforms when strncpy does not
behave like this for some reason.
I seriously doubt this inefficiency is going to be measurable in real
world. If the result was a buffer-overflow bug, that'd be a different
story, but maybe we could check the ~120 calls to strncpy in the whole
code base and replace it with strlcpy where appropriate.
That being said, thanks for looking into things like this.
Tomas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
It is likely far better explained here -->
http://www.courtesan.com/todd/papers/strlcpy.htmlFor example , the following 2 lines in jsonfuncs.c
memset(name, 0, NAMEDATALEN);
strncpy(name, fname, NAMEDATALEN);Be careful with 'Name' data type - it's not just a simple string buffer.
AFAIK it needs to work with hashing etc. so the zeroing is actually needed
here to make sure two values produce the same result. At least that's how
I understand the code after a quick check - for example this is from the
same jsonfuncs.c you mentioned:memset(fname, 0, NAMEDATALEN);
strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);So the zeroing is on purpose, although if strncpy does that then the
memset is probably superflous. Either people do that because of habit /
copy'n'paste, or maybe there are supported platforms when strncpy does not
behave like this for some reason.
I had not thought of the fact the some platforms don't properly implement
strncpy(). On quick check http://man.he.net/man3/strncpy seems to indicate
that this behaviour is part of the C89 standard. So does this mean we can
always assume that all supported platforms always 0 out the remaining
buffer?
I seriously doubt this inefficiency is going to be measurable in real
world. If the result was a buffer-overflow bug, that'd be a different
story, but maybe we could check the ~120 calls to strncpy in the whole
code base and replace it with strlcpy where appropriate.
The example was more of a demonstration of wrong assumption rather than
wasted cycles. Though the wasted cycles was on my mind a bit too. I was
more focused on trying to draw a bit of attention to commit
061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and does not
properly set the last byte to 0 afterwards. I think this case could just be
replaced with strlcpy which does all this hard work for us.
Regards
David Rowley
Show quoted text
That being said, thanks for looking into things like this.
Tomas
On 15 Listopad 2013, 1:00, David Rowley wrote:
On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
It is likely far better explained here -->
http://www.courtesan.com/todd/papers/strlcpy.htmlFor example , the following 2 lines in jsonfuncs.c
memset(name, 0, NAMEDATALEN);
strncpy(name, fname, NAMEDATALEN);Be careful with 'Name' data type - it's not just a simple string buffer.
AFAIK it needs to work with hashing etc. so the zeroing is actually
needed
here to make sure two values produce the same result. At least that's
how
I understand the code after a quick check - for example this is from the
same jsonfuncs.c you mentioned:memset(fname, 0, NAMEDATALEN);
strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);So the zeroing is on purpose, although if strncpy does that then the
memset is probably superflous. Either people do that because of habit /
copy'n'paste, or maybe there are supported platforms when strncpy does
not
behave like this for some reason.I had not thought of the fact the some platforms don't properly implement
strncpy(). On quick check http://man.he.net/man3/strncpy seems to indicate
that this behaviour is part of the C89 standard. So does this mean we can
always assume that all supported platforms always 0 out the remaining
buffer?
I don't know about such platform - I was merely speculating about why
people might use such code.
I seriously doubt this inefficiency is going to be measurable in real
world. If the result was a buffer-overflow bug, that'd be a different
story, but maybe we could check the ~120 calls to strncpy in the whole
code base and replace it with strlcpy where appropriate.The example was more of a demonstration of wrong assumption rather than
wasted cycles. Though the wasted cycles was on my mind a bit too. I was
Yeah. To be fair, number of occurrences in the code base is not a
particularly exact measure of the impact - some of those uses might be
used in code paths that are quite busy.
more focused on trying to draw a bit of attention to commit
061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and does not
properly set the last byte to 0 afterwards. I think this case could just
be
replaced with strlcpy which does all this hard work for us.
Hmm, you mean this piece of code?
strncpy(saved_argv0, argv[0], MAXPGPATH);
IMHO you're right that's probably broken, unless there's some checking
happening before the call.
Tomas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tomas Vondra (tv@fuzzy.cz) wrote:
On 15 Listopad 2013, 1:00, David Rowley wrote:
more focused on trying to draw a bit of attention to commit
061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and does not
properly set the last byte to 0 afterwards. I think this case could just
be
replaced with strlcpy which does all this hard work for us.Hmm, you mean this piece of code?
strncpy(saved_argv0, argv[0], MAXPGPATH);
IMHO you're right that's probably broken, unless there's some checking
happening before the call.
Agreed, that looks like a place we should be using strlcpy() instead.
Robert, what do you think?
Thanks,
Stephen
Tomas Vondra <tv@fuzzy.cz> wrote:
On 15 Listopad 2013, 1:00, David Rowley wrote:
more focused on trying to draw a bit of attention to commit
061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and
does not properly set the last byte to 0 afterwards. I think
this case could just be replaced with strlcpy which does all
this hard work for us.Hmm, you mean this piece of code?
strncpy(saved_argv0, argv[0], MAXPGPATH);
IMHO you're right that's probably broken, unless there's some
checking happening before the call.
I agree, and there is no such checking. Fix pushed.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-15 09:24:59 -0500, Stephen Frost wrote:
* Tomas Vondra (tv@fuzzy.cz) wrote:
On 15 Listopad 2013, 1:00, David Rowley wrote:
more focused on trying to draw a bit of attention to commit
061b88c732952c59741374806e1e41c1ec845d50 which uses strncpy and does not
properly set the last byte to 0 afterwards. I think this case could just
be
replaced with strlcpy which does all this hard work for us.Hmm, you mean this piece of code?
strncpy(saved_argv0, argv[0], MAXPGPATH);
IMHO you're right that's probably broken, unless there's some checking
happening before the call.Agreed, that looks like a place we should be using strlcpy() instead.
I don't mind fixing it, but I think anything but s/strncpy/strlcpy/ is
over the top. Translating such strings is just a waste of translator's
time.
If you really worry about paths being longer than MAXPGPATH, there's
lots, and lots of things to do that are, far, far more critical than
this.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-15 04:21:50 +0100, Tomas Vondra wrote:
Hmm, you mean this piece of code?
strncpy(saved_argv0, argv[0], MAXPGPATH);
IMHO you're right that's probably broken, unless there's some checking
happening before the call.
FWIW, argv0 is pretty much guaranteed to be shorter than MAXPGPATH since
MAXPGPATH is the longest a path can be, and argv[0] is either the executable's
name (if executed via PATH) or the path to the executable.
Now, you could probably write a program to exeve() a binary with argv[0]
being longer, but in that case you can also just put garbage in there.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@2ndquadrant.com) wrote:
FWIW, argv0 is pretty much guaranteed to be shorter than MAXPGPATH since
MAXPGPATH is the longest a path can be, and argv[0] is either the executable's
name (if executed via PATH) or the path to the executable.
Err, it's the longest that *we* think the path can be.. That's not the
same as actually being the longest that a path can be, which depends on
the filesystem and OS... It's not hard to get past our 1024 limit:
sfrost@beorn:/really/long/path> echo $PWD | wc -c
1409
Now, you could probably write a program to exeve() a binary with argv[0]
being longer, but in that case you can also just put garbage in there.
We shouldn't blow up in that case either, really.
Thanks,
Stephen
On 2013-11-15 09:53:24 -0500, Stephen Frost wrote:
* Andres Freund (andres@2ndquadrant.com) wrote:
FWIW, argv0 is pretty much guaranteed to be shorter than MAXPGPATH since
MAXPGPATH is the longest a path can be, and argv[0] is either the executable's
name (if executed via PATH) or the path to the executable.Err, it's the longest that *we* think the path can be.. That's not the
same as actually being the longest that a path can be, which depends on
the filesystem and OS... It's not hard to get past our 1024 limit:
Sure, there can be longer paths, but postgres don't support them. In a
*myriad* of places. It's just not worth spending code on it.
Just about any of the places that use MAXPGPATH are "vulnerable" or
produce confusing error messages if it's exceeded. And there are about
zero complaints about it.
Now, you could probably write a program to exeve() a binary with argv[0]
being longer, but in that case you can also just put garbage in there.We shouldn't blow up in that case either, really.
Good luck.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-15 10:04:12 -0500, Stephen Frost wrote:
* Andres Freund (andres@2ndquadrant.com) wrote:
Sure, there can be longer paths, but postgres don't support them. In a
*myriad* of places. It's just not worth spending code on it.Just about any of the places that use MAXPGPATH are "vulnerable" or
produce confusing error messages if it's exceeded. And there are about
zero complaints about it.Confusing error messages are one thing, segfaulting is another.
I didn't argue against s/strncpy/strlcpy/. That's clearly a sensible
fix.
I am arguing about introducing additional code and error messages about
it, that need to be translated. And starting doing so in isolationtester
of all places.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: 20131115150412.GC17272@tamriel.snowman.net
* Andres Freund (andres@2ndquadrant.com) wrote:
Sure, there can be longer paths, but postgres don't support them. In a
*myriad* of places. It's just not worth spending code on it.Just about any of the places that use MAXPGPATH are "vulnerable" or
produce confusing error messages if it's exceeded. And there are about
zero complaints about it.
Confusing error messages are one thing, segfaulting is another.
Thanks,
Stephen
David Rowley escribi�:
On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
Be careful with 'Name' data type - it's not just a simple string buffer.
AFAIK it needs to work with hashing etc. so the zeroing is actually needed
here to make sure two values produce the same result. At least that's how
I understand the code after a quick check - for example this is from the
same jsonfuncs.c you mentioned:memset(fname, 0, NAMEDATALEN);
strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);So the zeroing is on purpose, although if strncpy does that then the
memset is probably superflous.
This code should probably be using namecpy(). Note namecpy() doesn't
memset() after strncpy() and has survived the test of time, which
strongly suggests that the memset is indeed superfluous.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
This code should probably be using namecpy(). Note namecpy()
doesn't memset() after strncpy() and has survived the test of
time, which strongly suggests that the memset is indeed
superfluous.
That argument would be more persuasive if I could find any current
usage of the namecpy() function anywhere in the source code.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
I didn't argue against s/strncpy/strlcpy/. That's clearly a sensible
fix.
I am arguing about introducing additional code and error messages about
it, that need to be translated. And starting doing so in isolationtester
of all places.
I agree with Andres on this. Commit
7cb964acb794078ef033cbf2e3a0e7670c8992a9 is the very definition of
overkill, and I don't want to see us starting to plaster the source
code with things like this. Converting strncpy to strlcpy seems
appropriate --- and sufficient.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner escribi�:
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
This code should probably be using namecpy().� Note namecpy()
doesn't memset() after strncpy() and has survived the test of
time, which strongly suggests that the memset is indeed
superfluous.That argument would be more persuasive if I could find any current
usage of the namecpy() function anywhere in the source code.
Well, its cousin namestrcpy is used in a lot of places. That one uses a
regular C string as source; namecpy uses a Name as source, so they are
slightly different but the coding is pretty much the same.
There is a difference in using the macro StrNCpy instead of the strncpy
library function directly. ISTM this makes sense because Name is known
to be zero-terminated at NAMEDATALEN, which a random C string is not.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Andres Freund <andres@2ndquadrant.com> writes:
I didn't argue against s/strncpy/strlcpy/. That's clearly a sensible
fix.
I am arguing about introducing additional code and error messages about
it, that need to be translated. And starting doing so in isolationtester
of all places.I agree with Andres on this. Commit
7cb964acb794078ef033cbf2e3a0e7670c8992a9 is the very definition of
overkill, and I don't want to see us starting to plaster the source
code with things like this. Converting strncpy to strlcpy seems
appropriate --- and sufficient.
Personally, I'd like to see better handling like this- but done in a way
which minimizes impact to code and translators. A function like
namecpy() (which I agree with Kevin about- curious that it's not used..)
which handled the check, errmsg and exit seems reasonable to me, for the
"userland" binaries (and perhaps the postmaster when doing command-line
checking of, eg, -D) that need it.
Still, I'm not offering to go do it, so take my feelings on it with that
in mind. :)
Thanks,
Stephen
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Kevin Grittner escribió:
That argument would be more persuasive if I could find any current
usage of the namecpy() function anywhere in the source code.Well, its cousin namestrcpy is used in a lot of places. That one uses a
regular C string as source; namecpy uses a Name as source, so they are
slightly different but the coding is pretty much the same.
Fair enough.
There is a difference in using the macro StrNCpy instead of the strncpy
library function directly. ISTM this makes sense because Name is known
to be zero-terminated at NAMEDATALEN, which a random C string is not.
Is the capital T in the second #undef in this pg_locale.c code intended?:
#ifdef WIN32
/*
* This Windows file defines StrNCpy. We don't need it here, so we undefine
* it to keep the compiler quiet, and undefine it again after the file is
* included, so we don't accidentally use theirs.
*/
#undef StrNCpy
#include <shlwapi.h>
#ifdef StrNCpy
#undef STrNCpy
#endif
#endif
--
Kevin GrittnerEDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Nov 16, 2013 at 4:09 AM, Alvaro Herrera <alvherre@2ndquadrant.com>wrote:
David Rowley escribió:
On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <tv@fuzzy.cz> wrote:
Be careful with 'Name' data type - it's not just a simple string
buffer.
AFAIK it needs to work with hashing etc. so the zeroing is actually
needed
here to make sure two values produce the same result. At least that's
how
I understand the code after a quick check - for example this is from
the
same jsonfuncs.c you mentioned:
memset(fname, 0, NAMEDATALEN);
strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);So the zeroing is on purpose, although if strncpy does that then the
memset is probably superflous.This code should probably be using namecpy(). Note namecpy() doesn't
memset() after strncpy() and has survived the test of time, which
strongly suggests that the memset is indeed superfluous.
I went on a bit of a strncpy cleanup rampage this morning and ended up
finding quite a few places where strncpy is used wrongly.
I'm not quite sure if I have got them all in this patch, but I' think I've
got the obvious ones at least.
For the hash_search in jsconfuncs.c after thinking about it a bit more...
Can we not just pass the attname without making a copy of it? I see keyPtr
in hash_search is const void * so it shouldn't get modified in there. I
can't quite see the reason for making the copy.
Attached is a patch with various cleanups where I didn't like the look of
the strncpy. I didn't go overboard with this as I know making this sort of
small changes all over can be a bit scary and I thought maybe it would get
rejected on that basis.
I also cleaned up things like strncpy(dest, src, strlen(src)); which just
seems a bit weird and I'm failing to get my head around why it was done. I
replaced these with memcpy instead, but they could perhaps be a plain old
strcpy.
Regards
David Rowley
Show quoted text
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
strncpy_cleanup_v0.1.patchapplication/octet-stream; name=strncpy_cleanup_v0.1.patchDownload
diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c
index 3db6b84..3d2b06b 100644
--- a/contrib/isn/isn.c
+++ b/contrib/isn/isn.c
@@ -825,18 +825,18 @@ string2ean(const char *str, bool errorOK, ean13 *result,
goto eanwrongtype;
break;
case ISMN:
- strncpy(buf, "9790", 4); /* this isn't for sure yet, for now
+ memcpy(buf, "9790", 4); /* this isn't for sure yet, for now
* ISMN it's only 9790 */
valid = (valid && ((rcheck = checkdig(buf + 3, 10)) == check || magic));
break;
case ISBN:
- strncpy(buf, "978", 3);
+ memcpy(buf, "978", 3);
valid = (valid && ((rcheck = weight_checkdig(buf + 3, 10)) == check || magic));
break;
case ISSN:
- strncpy(buf + 10, "00", 2); /* append 00 as the normal issue
+ memcpy(buf + 10, "00", 2); /* append 00 as the normal issue
* publication code */
- strncpy(buf, "977", 3);
+ memcpy(buf, "977", 3);
valid = (valid && ((rcheck = weight_checkdig(buf + 3, 8)) == check || magic));
break;
case UPC:
diff --git a/contrib/pg_archivecleanup/pg_archivecleanup.c b/contrib/pg_archivecleanup/pg_archivecleanup.c
index f12331a..59793a1 100644
--- a/contrib/pg_archivecleanup/pg_archivecleanup.c
+++ b/contrib/pg_archivecleanup/pg_archivecleanup.c
@@ -119,7 +119,7 @@ CleanupPriorWALFiles(void)
{
while ((xlde = readdir(xldir)) != NULL)
{
- strncpy(walfile, xlde->d_name, MAXPGPATH);
+ strlcpy(walfile, xlde->d_name, MAXPGPATH);
TrimExtension(walfile, additional_ext);
/*
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index fff71e5..65c77c9 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -1779,7 +1779,7 @@ init(bool is_no_vacuum)
{
char buffer[256];
- strncpy(buffer, DDLAFTERs[i], 256);
+ strlcpy(buffer, DDLAFTERs[i], sizeof(buffer));
if (index_tablespace != NULL)
{
@@ -1787,7 +1787,7 @@ init(bool is_no_vacuum)
escape_tablespace = PQescapeIdentifier(con, index_tablespace,
strlen(index_tablespace));
- snprintf(buffer + strlen(buffer), 256 - strlen(buffer),
+ snprintf(buffer + strlen(buffer), sizeof(buffer) - strlen(buffer),
" using index tablespace %s", escape_tablespace);
PQfreemem(escape_tablespace);
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a95149b..5da0d48 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5664,7 +5664,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
recordRestorePointData = (xl_restore_point *) XLogRecGetData(record);
recordXtime = recordRestorePointData->rp_time;
- strncpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
+ strlcpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
}
else
return false;
@@ -5759,7 +5759,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
}
else
{
- strncpy(recoveryStopName, recordRPName, MAXFNAMELEN);
+ strlcpy(recoveryStopName, recordRPName, MAXFNAMELEN);
ereport(LOG,
(errmsg("recovery stopping at restore point \"%s\", time %s",
@@ -6094,7 +6094,7 @@ StartupXLOG(void)
* Save archive_cleanup_command in shared memory so that other processes
* can see it.
*/
- strncpy(XLogCtl->archiveCleanupCommand,
+ strlcpy(XLogCtl->archiveCleanupCommand,
archiveCleanupCommand ? archiveCleanupCommand : "",
sizeof(XLogCtl->archiveCleanupCommand));
@@ -8795,7 +8795,7 @@ XLogRestorePoint(const char *rpName)
xl_restore_point xlrec;
xlrec.rp_time = GetCurrentTimestamp();
- strncpy(xlrec.rp_name, rpName, MAXFNAMELEN);
+ strlcpy(xlrec.rp_name, rpName, MAXFNAMELEN);
rdata.buffer = InvalidBuffer;
rdata.data = (char *) &xlrec;
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 342975c..2502b16 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -458,7 +458,7 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname)
xlogfpath, oldpath)));
}
#else
- strncpy(oldpath, xlogfpath, MAXPGPATH);
+ strlcpy(oldpath, xlogfpath, MAXPGPATH);
#endif
if (unlink(oldpath) != 0)
ereport(FATAL,
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 1b8f109..50c879c 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3824,7 +3824,7 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char
day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday);
tm->tm_wday = j2day(day);
- strncpy(str, days[tm->tm_wday], 3);
+ memcpy(str, days[tm->tm_wday], 3);
strcpy(str + 3, " ");
if (DateOrder == DATEORDER_DMY)
@@ -4307,8 +4307,7 @@ pg_timezone_abbrevs(PG_FUNCTION_ARGS)
* Convert name to text, using upcasing conversion that is the inverse of
* what ParseDateTime() uses.
*/
- strncpy(buffer, timezonetktbl[*pindex].token, TOKMAXLEN);
- buffer[TOKMAXLEN] = '\0'; /* may not be null-terminated */
+ strlcpy(buffer, timezonetktbl[*pindex].token, sizeof(buffer));
for (p = (unsigned char *) buffer; *p; p++)
*p = pg_toupper(*p);
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index bcb9354..9185206 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -1231,7 +1231,6 @@ json_populate_record(PG_FUNCTION_ARGS)
int i;
Datum *values;
bool *nulls;
- char fname[NAMEDATALEN];
JsonHashEntry *hashentry;
use_json_as_text = PG_ARGISNULL(2) ? false : PG_GETARG_BOOL(2);
@@ -1351,9 +1350,7 @@ json_populate_record(PG_FUNCTION_ARGS)
continue;
}
- memset(fname, 0, NAMEDATALEN);
- strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
- hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);
+ hashentry = hash_search(json_hash, &tupdesc->attrs[i]->attname, HASH_FIND, NULL);
/*
* we can't just skip here if the key wasn't found since we might have
@@ -1495,9 +1492,7 @@ hash_object_field_end(void *state, char *fname, bool isnull)
if (_state->lex->lex_level > 2 || strlen(fname) >= NAMEDATALEN)
return;
- memset(name, 0, NAMEDATALEN);
- strncpy(name, fname, NAMEDATALEN);
-
+ strncpy(name, fname, NAMEDATALEN); /* strncpy will 0 any remaining buffer space */
hashentry = hash_search(_state->hash, name, HASH_ENTER, &found);
/*
@@ -1721,7 +1716,6 @@ populate_recordset_object_end(void *state)
HTAB *json_hash = _state->json_hash;
Datum *values;
bool *nulls;
- char fname[NAMEDATALEN];
int i;
RecordIOData *my_extra = _state->my_extra;
int ncolumns = my_extra->ncolumns;
@@ -1771,9 +1765,7 @@ populate_recordset_object_end(void *state)
continue;
}
- memset(fname, 0, NAMEDATALEN);
- strncpy(fname, NameStr(tupdesc->attrs[i]->attname), NAMEDATALEN);
- hashentry = hash_search(json_hash, fname, HASH_FIND, NULL);
+ hashentry = hash_search(json_hash, &tupdesc->attrs[i]->attname, HASH_FIND, NULL);
/*
* we can't just skip here if the key wasn't found since we might have
@@ -1902,9 +1894,7 @@ populate_recordset_object_field_end(void *state, char *fname, bool isnull)
if (_state->lex->lex_level > 2 || strlen(fname) >= NAMEDATALEN)
return;
- memset(name, 0, NAMEDATALEN);
- strncpy(name, fname, NAMEDATALEN);
-
+ strncpy(name, fname, NAMEDATALEN); /* strncpy will 0 any remaining buffer space */
hashentry = hash_search(_state->json_hash, name, HASH_ENTER, &found);
/*
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index e648792..794eeab 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2100,7 +2100,7 @@ setup_formatted_log_time(void)
/* 'paste' milliseconds into place... */
sprintf(msbuf, ".%03d", (int) (tv.tv_usec / 1000));
- strncpy(formatted_log_time + 19, msbuf, 4);
+ memcpy(formatted_log_time + 19, msbuf, 4);
}
/*
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index cd2dded..7a7ad7c 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -399,6 +399,13 @@ ExecuteSqlCommand(ArchiveHandle *AH, const char *qry, const char *desc)
default:
/* trouble */
strncpy(errStmt, qry, DB_MAX_ERR_STMT);
+
+ /*
+ * If qry was too long to fit in errStmt then the string won't
+ * be null terminated. For this case we'll change this to make
+ * the string end in "..." to show the user that the query was
+ * longer but we chopped it down a bit for display.
+ */
if (errStmt[DB_MAX_ERR_STMT - 1] != '\0')
{
errStmt[DB_MAX_ERR_STMT - 4] = '.';
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index b2990ca..6e25f46 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -210,7 +210,7 @@ get_char_item(int lineno, void *var, enum ECPGttype vartype, char *value, int va
(struct ECPGgeneric_varchar *) var;
if (varcharsize == 0)
- strncpy(variable->arr, value, strlen(value));
+ memcpy(variable->arr, value, strlen(value));
else
strncpy(variable->arr, value, varcharsize);
diff --git a/src/interfaces/ecpg/pgtypeslib/datetime.c b/src/interfaces/ecpg/pgtypeslib/datetime.c
index 6600759..6391625 100644
--- a/src/interfaces/ecpg/pgtypeslib/datetime.c
+++ b/src/interfaces/ecpg/pgtypeslib/datetime.c
@@ -277,7 +277,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
return -1;
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
"%u", replace_val.uint_val);
- strncpy(start_pattern, t, strlen(t));
+ memcpy(start_pattern, t, strlen(t));
free(t);
}
break;
@@ -289,7 +289,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
return -1;
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
"%02u", replace_val.uint_val);
- strncpy(start_pattern, t, strlen(t));
+ memcpy(start_pattern, t, strlen(t));
free(t);
}
break;
@@ -301,7 +301,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
return -1;
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
"%04u", replace_val.uint_val);
- strncpy(start_pattern, t, strlen(t));
+ memcpy(start_pattern, t, strlen(t));
free(t);
}
break;
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index d88c752..f39b91f 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -1074,7 +1074,7 @@ initialize_SSL(PGconn *conn)
/* Read the client certificate file */
if (conn->sslcert && strlen(conn->sslcert) > 0)
- strncpy(fnbuf, conn->sslcert, sizeof(fnbuf));
+ strlcpy(fnbuf, conn->sslcert, sizeof(fnbuf));
else if (have_homedir)
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
else
@@ -1265,7 +1265,7 @@ initialize_SSL(PGconn *conn)
#endif /* USE_SSL_ENGINE */
{
/* PGSSLKEY is not an engine, treat it as a filename */
- strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
+ strlcpy(fnbuf, conn->sslkey, sizeof(fnbuf));
}
}
else if (have_homedir)
@@ -1328,7 +1328,7 @@ initialize_SSL(PGconn *conn)
* verification after the connection has been completed.
*/
if (conn->sslrootcert && strlen(conn->sslrootcert) > 0)
- strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
+ strlcpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
else if (have_homedir)
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
else
@@ -1366,7 +1366,7 @@ initialize_SSL(PGconn *conn)
if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
{
if (conn->sslcrl && strlen(conn->sslcrl) > 0)
- strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
+ strlcpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
else if (have_homedir)
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE);
else
On Sat, Nov 16, 2013 at 12:53:10PM +1300, David Rowley wrote:
I went on a bit of a strncpy cleanup rampage this morning and ended up
finding quite a few places where strncpy is used wrongly.
I'm not quite sure if I have got them all in this patch, but I' think I've
got the obvious ones at least.For the hash_search in jsconfuncs.c after thinking about it a bit more...
Can we not just pass the attname without making a copy of it? I see keyPtr
in hash_search is const void * so it shouldn't get modified in there. I
can't quite see the reason for making the copy.
+1 for the goal of this patch. Another commit took care of your jsonfuncs.c
concerns, and the patch for CVE-2014-0065 fixed several of the others. Plenty
remain, though.
Attached is a patch with various cleanups where I didn't like the look of
the strncpy. I didn't go overboard with this as I know making this sort of
small changes all over can be a bit scary and I thought maybe it would get
rejected on that basis.I also cleaned up things like strncpy(dest, src, strlen(src)); which just
seems a bit weird and I'm failing to get my head around why it was done. I
replaced these with memcpy instead, but they could perhaps be a plain old
strcpy.
I suggest preparing one or more patches that focus on the cosmetic-only
changes, such as strncpy() -> memcpy() when strncpy() is guaranteed not to
reach a NUL byte. With that noise out of the way, it will be easier to give
the rest the attention it deserves.
Thanks,
nm
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 13, 2014 at 3:19 PM, Noah Misch <noah@leadboat.com> wrote:
On Sat, Nov 16, 2013 at 12:53:10PM +1300, David Rowley wrote:
I went on a bit of a strncpy cleanup rampage this morning and ended up
finding quite a few places where strncpy is used wrongly.
I'm not quite sure if I have got them all in this patch, but I' thinkI've
got the obvious ones at least.
For the hash_search in jsconfuncs.c after thinking about it a bit more...
Can we not just pass the attname without making a copy of it? I seekeyPtr
in hash_search is const void * so it shouldn't get modified in there. I
can't quite see the reason for making the copy.+1 for the goal of this patch. Another commit took care of your
jsonfuncs.c
concerns, and the patch for CVE-2014-0065 fixed several of the others.
Plenty
remain, though.
Thanks for taking interest in this.
I had a quick look at the usages of strncpy in master tonight and I've
really just picked out the obviously broken ones for now. The other ones,
on first look, either look safe, or require some more analysis to see
what's actually done with the string.
I think this is likely best tackled in small increments anyway.
Does anyone disagree with the 2 changes in the attached?
Regards
David Rowley
Attachments:
strncpy_fixes_pass1.patchapplication/octet-stream; name=strncpy_fixes_pass1.patchDownload
diff --git a/contrib/pg_archivecleanup/pg_archivecleanup.c b/contrib/pg_archivecleanup/pg_archivecleanup.c
index 212b267..c449218 100644
--- a/contrib/pg_archivecleanup/pg_archivecleanup.c
+++ b/contrib/pg_archivecleanup/pg_archivecleanup.c
@@ -108,7 +108,7 @@ CleanupPriorWALFiles(void)
{
while (errno = 0, (xlde = readdir(xldir)) != NULL)
{
- strncpy(walfile, xlde->d_name, MAXPGPATH);
+ strlcpy(walfile, xlde->d_name, MAXPGPATH);
TrimExtension(walfile, additional_ext);
/*
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 37745dc..0c9498a 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -459,7 +459,7 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname)
xlogfpath, oldpath)));
}
#else
- strncpy(oldpath, xlogfpath, MAXPGPATH);
+ strlcpy(oldpath, xlogfpath, MAXPGPATH);
#endif
if (unlink(oldpath) != 0)
ereport(FATAL,
David Rowley <dgrowleyml@gmail.com> wrote:
I had a quick look at the usages of strncpy in master tonight and
I've really just picked out the obviously broken ones for now.
The other ones, on first look, either look safe, or require some
more analysis to see what's actually done with the string.Does anyone disagree with the 2 changes in the attached?
I am concerned that failure to check for truncation could allow
deletion of unexpected files or directories. While this is
probably not as dangerous as *executing* unexpected files, it seems
potentially problematic. At the very least, a code comment
explaining why calling unlink on something which is not what
appears to be expected is not a problem there.
Some might consider it overkill, but I tend to draw a pretty hard
line on deleting or executing random files, even if the odds seem
to be that the mangled name won't find a match. Granted, those
problems exist now, but without checking for truncation it seems to
me that we're just deleting *different* incorrect filenames, not
really fixing the problem.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> writes:
I am concerned that failure to check for truncation could allow
deletion of unexpected files or directories.
I believe that we deal with this by the expedient of checking the lengths
of tablespace paths in advance, when the tablespace is created.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/13/2014 04:31 PM, Kevin Grittner wrote:
David Rowley <dgrowleyml@gmail.com> wrote:
I had a quick look at the usages of strncpy in master tonight and
I've really just picked out the obviously broken ones for now.
The other ones, on first look, either look safe, or require some
more analysis to see what's actually done with the string.Does anyone disagree with the 2 changes in the attached?
I am concerned that failure to check for truncation could allow
deletion of unexpected files or directories. While this is
probably not as dangerous as *executing* unexpected files, it seems
potentially problematic. At the very least, a code comment
explaining why calling unlink on something which is not what
appears to be expected is not a problem there.Some might consider it overkill, but I tend to draw a pretty hard
line on deleting or executing random files, even if the odds seem
to be that the mangled name won't find a match. Granted, those
problems exist now, but without checking for truncation it seems to
me that we're just deleting *different* incorrect filenames, not
really fixing the problem.
strlcpy is clearly better than strncpy here, but I wonder if we should
have yet another string copying function that throws an error instead of
truncating, if the buffer is too small. What you really want in these
cases is a "path too long" error.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kevin Grittner <kgrittn@ymail.com> writes:
I am concerned that failure to check for truncation could allow
deletion of unexpected files or directories.I believe that we deal with this by the expedient of checking the
lengths of tablespace paths in advance, when the tablespace is
created.
As long as it is covered.
I would point out that the when strlcpy is used it returns a size_t
which can be directly compared to one of the arguments passed in
(in this case MAXPGPATH) to detect whether the name was truncated
for the cost of an integer compare (probably in registers). No
additional scan of the data is needed.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote:
Kevin Grittner <kgrittn@ymail.com> writes:
I am concerned that failure to check for truncation could allow
deletion of unexpected files or directories.I believe that we deal with this by the expedient of checking the lengths
of tablespace paths in advance, when the tablespace is created.
The files under scrutiny here are not located in a tablespace. Even if they
were, isn't the length of $PGDATA/pg_tblspc the important factor? $PGDATA can
change between runs if the DBA moves the data directory or reaches it via
different symlinks, so any DDL-time defense would be incomplete.
Some might consider it overkill, but I tend to draw a pretty hard
line on deleting or executing random files, even if the odds seem
to be that the mangled name won't find a match.� Granted, those
problems exist now, but without checking for truncation it seems to
me that we're just deleting *different* incorrect filenames, not
really fixing the problem.
I share your (Kevin's) discomfort with our use of strlcpy(). I wouldn't mind
someone replacing most strlcpy()/snprintf() calls with calls to wrappers that
ereport(ERROR) on truncation. Though as reliability problems go, this one has
been minor.
David's specific patch has no concrete problem:
On Wed, Aug 13, 2014 at 10:26:01PM +1200, David Rowley wrote:
--- a/contrib/pg_archivecleanup/pg_archivecleanup.c +++ b/contrib/pg_archivecleanup/pg_archivecleanup.c @@ -108,7 +108,7 @@ CleanupPriorWALFiles(void) { while (errno = 0, (xlde = readdir(xldir)) != NULL) { - strncpy(walfile, xlde->d_name, MAXPGPATH); + strlcpy(walfile, xlde->d_name, MAXPGPATH);
The code proceeds to check strlen(walfile) == XLOG_DATA_FNAME_LEN, so a long
name can't trick it.
TrimExtension(walfile, additional_ext);
/* diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 37745dc..0c9498a 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -459,7 +459,7 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname) xlogfpath, oldpath))); } #else - strncpy(oldpath, xlogfpath, MAXPGPATH); + strlcpy(oldpath, xlogfpath, MAXPGPATH);
This one never overflows, because it's copying from one MAXPGPATH buffer to
another. Plain strcpy() would be fine, too.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Noah Misch <noah@leadboat.com> writes:
On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote:
I believe that we deal with this by the expedient of checking the lengths
of tablespace paths in advance, when the tablespace is created.
The files under scrutiny here are not located in a tablespace. Even if they
were, isn't the length of $PGDATA/pg_tblspc the important factor?
The length of $PGDATA is of no relevance whatsoever; we chdir into that
directory at startup, and subsequently all paths are implicitly relative
to there. If there is any backend code that's prepending $PGDATA to
something else, it's wrong to start with.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 14, 2014 at 02:50:02AM -0400, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote:
I believe that we deal with this by the expedient of checking the lengths
of tablespace paths in advance, when the tablespace is created.The files under scrutiny here are not located in a tablespace. Even if they
were, isn't the length of $PGDATA/pg_tblspc the important factor?The length of $PGDATA is of no relevance whatsoever; we chdir into that
directory at startup, and subsequently all paths are implicitly relative
to there. If there is any backend code that's prepending $PGDATA to
something else, it's wrong to start with.
Ah; quite right.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 14, 2014 at 4:13 PM, Noah Misch <noah@leadboat.com> wrote:
I share your (Kevin's) discomfort with our use of strlcpy(). I wouldn't
mind
someone replacing most strlcpy()/snprintf() calls with calls to wrappers
that
ereport(ERROR) on truncation. Though as reliability problems go, this one
has
been minor.
Or maybe it would be better to just remove the restriction and just palloc
something of the correct size?
Although, that sounds like a much larger patch. I'd vote that the strlcpy
should be used in the meantime.
Regards
David Rowley
On Sat, Aug 16, 2014 at 10:38:39AM +1200, David Rowley wrote:
On Thu, Aug 14, 2014 at 4:13 PM, Noah Misch <noah@leadboat.com> wrote:
I share your (Kevin's) discomfort with our use of strlcpy(). I wouldn't
mind
someone replacing most strlcpy()/snprintf() calls with calls to wrappers
that
ereport(ERROR) on truncation. Though as reliability problems go, this one
has
been minor.Or maybe it would be better to just remove the restriction and just palloc
something of the correct size?
Although, that sounds like a much larger patch. I'd vote that the strlcpy
should be used in the meantime.
I agree that, in principle, dynamic allocation might be better still. I also
agree that it would impose more code churn, for an awfully-narrow benefit.
Barring objections, I will commit your latest patch with some comments about
why truncation is harmless for those two particular calls.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 15, 2014 at 11:26:55PM -0400, Noah Misch wrote:
Barring objections, I will commit your latest patch with some comments about
why truncation is harmless for those two particular calls.
Done.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers