agora inbox for [email protected]help / color / mirror / Atom feed
strncpy is not a safe version of strcpy 32+ messages / 10 participants [nested] [flat]
* strncpy is not a safe version of strcpy @ 2013-11-14 23:07 David Rowley <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: David Rowley @ 2013-11-14 23:07 UTC (permalink / raw) To: pgsql-hackers 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 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-14 23:33 Tomas Vondra <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Tomas Vondra @ 2013-11-14 23:33 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: pgsql-hackers 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.html > > For 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 00:00 David Rowley <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: David Rowley @ 2013-11-15 00:00 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: pgsql-hackers On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <[email protected]> wrote: > > 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); > > 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 > That being said, thanks for looking into things like this. > > Tomas > > ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 03:21 Tomas Vondra <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 3 replies; 32+ messages in thread From: Tomas Vondra @ 2013-11-15 03:21 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers On 15 Listopad 2013, 1:00, David Rowley wrote: > On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <[email protected]> wrote: > >> > 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); >> >> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:24 Stephen Frost <[email protected]> parent: Tomas Vondra <[email protected]> 2 siblings, 1 reply; 32+ messages in thread From: Stephen Frost @ 2013-11-15 14:24 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; Robert Haas <[email protected]>; +Cc: David Rowley <[email protected]>; pgsql-hackers * Tomas Vondra ([email protected]) 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 Attachments: [application/pgp-signature] signature.asc (836B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:32 Kevin Grittner <[email protected]> parent: Tomas Vondra <[email protected]> 2 siblings, 0 replies; 32+ messages in thread From: Kevin Grittner @ 2013-11-15 14:32 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; +Cc: pgsql-hackers Tomas Vondra <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:35 Andres Freund <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Andres Freund @ 2013-11-15 14:35 UTC (permalink / raw) To: Stephen Frost <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Robert Haas <[email protected]>; David Rowley <[email protected]>; pgsql-hackers On 2013-11-15 09:24:59 -0500, Stephen Frost wrote: > * Tomas Vondra ([email protected]) 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:41 Andres Freund <[email protected]> parent: Tomas Vondra <[email protected]> 2 siblings, 1 reply; 32+ messages in thread From: Andres Freund @ 2013-11-15 14:41 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: David Rowley <[email protected]>; 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:53 Stephen Frost <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Stephen Frost @ 2013-11-15 14:53 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; pgsql-hackers * Andres Freund ([email protected]) 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 Attachments: [application/pgp-signature] signature.asc (836B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 14:56 Andres Freund <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Andres Freund @ 2013-11-15 14:56 UTC (permalink / raw) To: Stephen Frost <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; pgsql-hackers On 2013-11-15 09:53:24 -0500, Stephen Frost wrote: > * Andres Freund ([email protected]) 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:01 Andres Freund <[email protected]> parent: Stephen Frost <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Andres Freund @ 2013-11-15 15:01 UTC (permalink / raw) To: Stephen Frost <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; pgsql-hackers On 2013-11-15 10:04:12 -0500, Stephen Frost wrote: > * Andres Freund ([email protected]) 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:04 Stephen Frost <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Stephen Frost @ 2013-11-15 15:04 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; pgsql-hackers * Andres Freund ([email protected]) 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 Attachments: [application/pgp-signature] signature.asc (836B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:09 Alvaro Herrera <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 2 replies; 32+ messages in thread From: Alvaro Herrera @ 2013-11-15 15:09 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers David Rowley escribió: > On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:18 Kevin Grittner <[email protected]> parent: Alvaro Herrera <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Kevin Grittner @ 2013-11-15 15:18 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers Alvaro Herrera <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:25 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Tom Lane @ 2013-11-15 15:25 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Stephen Frost <[email protected]>; Tomas Vondra <[email protected]>; David Rowley <[email protected]>; Kevin Grittner <[email protected]>; pgsql-hackers Andres Freund <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:27 Alvaro Herrera <[email protected]> parent: Kevin Grittner <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Alvaro Herrera @ 2013-11-15 15:27 UTC (permalink / raw) To: Kevin Grittner <[email protected]>; +Cc: David Rowley <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers Kevin Grittner escribió: > Alvaro Herrera <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:48 Stephen Frost <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Stephen Frost @ 2013-11-15 15:48 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; David Rowley <[email protected]>; Kevin Grittner <[email protected]>; pgsql-hackers * Tom Lane ([email protected]) wrote: > Andres Freund <[email protected]> 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 Attachments: [application/pgp-signature] signature.asc (836B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 15:56 Kevin Grittner <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Kevin Grittner @ 2013-11-15 15:56 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: David Rowley <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers Alvaro Herrera <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2013-11-15 23:53 David Rowley <[email protected]> parent: Alvaro Herrera <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: David Rowley @ 2013-11-15 23:53 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tomas Vondra <[email protected]>; pgsql-hackers On Sat, Nov 16, 2013 at 4:09 AM, Alvaro Herrera <[email protected]>wrote: > David Rowley escribió: > > On Fri, Nov 15, 2013 at 12:33 PM, Tomas Vondra <[email protected]> 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 > -- > Álvaro Herrera http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments: [application/octet-stream] strncpy_cleanup_v0.1.patch (11.7K, ../../CAApHDvrCrKSrx3WJSxN2-9FQJGYZMWtz-Nz9c4PWEMTYW8sdQg@mail.gmail.com/3-strncpy_cleanup_v0.1.patch) download | inline diff: 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 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 03:19 Noah Misch <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Noah Misch @ 2014-08-13 03:19 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 10:26 David Rowley <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: David Rowley @ 2014-08-13 10:26 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On Wed, Aug 13, 2014 at 3:19 PM, Noah Misch <[email protected]> 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' 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. > > 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 -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments: [application/octet-stream] strncpy_fixes_pass1.patch (981B, ../../CAApHDvp6YM9tSBCLZOrA-P5oAWjoN-dDU7200O7wERsiG=9EYA@mail.gmail.com/3-strncpy_fixes_pass1.patch) download | inline diff: 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, ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 13:31 Kevin Grittner <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: Kevin Grittner @ 2014-08-13 13:31 UTC (permalink / raw) To: David Rowley <[email protected]>; Noah Misch <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers David Rowley <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 14:21 Tom Lane <[email protected]> parent: Kevin Grittner <[email protected]> 1 sibling, 2 replies; 32+ messages in thread From: Tom Lane @ 2014-08-13 14:21 UTC (permalink / raw) To: Kevin Grittner <[email protected]>; +Cc: David Rowley <[email protected]>; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers Kevin Grittner <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 14:23 Heikki Linnakangas <[email protected]> parent: Kevin Grittner <[email protected]> 1 sibling, 0 replies; 32+ messages in thread From: Heikki Linnakangas @ 2014-08-13 14:23 UTC (permalink / raw) To: Kevin Grittner <[email protected]>; David Rowley <[email protected]>; Noah Misch <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On 08/13/2014 04:31 PM, Kevin Grittner wrote: > David Rowley <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-13 14:55 Kevin Grittner <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 32+ messages in thread From: Kevin Grittner @ 2014-08-13 14:55 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: David Rowley <[email protected]>; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers Tom Lane <[email protected]> wrote: > Kevin Grittner <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-14 04:13 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 2 replies; 32+ messages in thread From: Noah Misch @ 2014-08-14 04:13 UTC (permalink / raw) To: Tom Lane <[email protected]>; Kevin Grittner <[email protected]>; +Cc: David Rowley <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote: > Kevin Grittner <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-14 06:50 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Tom Lane @ 2014-08-14 06:50 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Kevin Grittner <[email protected]>; David Rowley <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers Noah Misch <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-15 06:09 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Noah Misch @ 2014-08-15 06:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Kevin Grittner <[email protected]>; David Rowley <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On Thu, Aug 14, 2014 at 02:50:02AM -0400, Tom Lane wrote: > Noah Misch <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-15 22:38 David Rowley <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: David Rowley @ 2014-08-15 22:38 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Kevin Grittner <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On Thu, Aug 14, 2014 at 4:13 PM, Noah Misch <[email protected]> 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 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-16 03:26 Noah Misch <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Noah Misch @ 2014-08-16 03:26 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Tom Lane <[email protected]>; Kevin Grittner <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; pgsql-hackers On Sat, Aug 16, 2014 at 10:38:39AM +1200, David Rowley wrote: > On Thu, Aug 14, 2014 at 4:13 PM, Noah Misch <[email protected]> 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: strncpy is not a safe version of strcpy @ 2014-08-19 03:15 Noah Misch <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Noah Misch @ 2014-08-19 03:15 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Tom Lane <[email protected]>; Kevin Grittner <[email protected]>; Alvaro Herrera <[email protected]>; Tomas Vondra <[email protected]>; 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 ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH 2/3] one more stress test for repack concurrently @ 2025-12-13 17:46 Mikhail Nikalayeu <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Mikhail Nikalayeu @ 2025-12-13 17:46 UTC (permalink / raw) --- contrib/amcheck/meson.build | 1 + contrib/amcheck/t/008_repack_2.pl | 111 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 contrib/amcheck/t/008_repack_2.pl diff --git a/contrib/amcheck/meson.build b/contrib/amcheck/meson.build index cb4bc32e98a..6799df214f9 100644 --- a/contrib/amcheck/meson.build +++ b/contrib/amcheck/meson.build @@ -51,6 +51,7 @@ tests += { 't/005_pitr.pl', 't/006_verify_gin.pl', 't/007_repack_1.pl', + 't/008_repack_2.pl', ], }, } diff --git a/contrib/amcheck/t/008_repack_2.pl b/contrib/amcheck/t/008_repack_2.pl new file mode 100644 index 00000000000..a287ee2bb71 --- /dev/null +++ b/contrib/amcheck/t/008_repack_2.pl @@ -0,0 +1,111 @@ +# Copyright (c) 2021-2026, PostgreSQL Global Development Group + +# Test REPACK CONCURRENTLY with concurrent modifications +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; + +use Test::More; + +# PG_TEST_EXTRA carries a numerical stress value; 0 disables the test, +# 1 means the test takes about 6 seconds, and the increase should be +# roughly linear. +my $matched = ($ENV{PG_TEST_EXTRA} // '') =~ /\bstress_concurrently(?:=(?<stressval>[0-9]*))?\b/; +my $stressval = $+{stressval} // 1; +if (!$matched or $stressval == 0) +{ + plan skip_all => 'skipping disabled REPACK CONCURRENTLY stress test'; +} + +my $node; + +# +# Test set-up +# +$node = PostgreSQL::Test::Cluster->new('CIC_test'); +$node->init; +$node->append_conf('postgresql.conf', + 'lock_timeout = ' . (1000 * $PostgreSQL::Test::Utils::timeout_default)); +$node->append_conf( + 'postgresql.conf', qq( +wal_level = logical +)); + +my $duration = 6 * $stressval; +my $no_hot = int(rand(2)); + +$node->start; +$node->safe_psql('postgres', q(CREATE TABLE tbl(id SERIAL PRIMARY KEY, val int))); +if ($no_hot) +{ + $node->safe_psql('postgres', q(CREATE INDEX test_idx ON tbl(val);)); +} +else +{ + $node->safe_psql('postgres', q(CREATE INDEX test_idx ON tbl(id);)); +} + +# Load amcheck +$node->safe_psql('postgres', q(CREATE EXTENSION amcheck)); + +my $sum = $node->safe_psql('postgres', q( + SELECT SUM(val) AS sum FROM tbl +)); + +$node->safe_psql('postgres', q(CREATE UNLOGGED SEQUENCE last_j START 1 INCREMENT 1;)); + + +$node->pgbench( +"--no-vacuum --client=30 --jobs=4 --exit-on-abort -T $duration", +0, +[qr{actually processed}], +[qr{^$}], +'concurrent operations with REINDEX/CREATE INDEX CONCURRENTLY', +{ + 'concurrent_ops' => qq( + SELECT pg_try_advisory_lock(42)::integer AS gotlock \\gset + \\if :gotlock + REPACK (CONCURRENTLY) tbl USING INDEX tbl_pkey; + SELECT bt_index_parent_check('tbl_pkey', heapallindexed => true); + SELECT bt_index_parent_check('test_idx', heapallindexed => true); + \\sleep 10 ms + + REPACK (CONCURRENTLY) tbl USING INDEX test_idx; + SELECT bt_index_parent_check('tbl_pkey', heapallindexed => true); + SELECT bt_index_parent_check('test_idx', heapallindexed => true); + \\sleep 10 ms + + REPACK (CONCURRENTLY) tbl; + SELECT bt_index_parent_check('tbl_pkey', heapallindexed => true); + SELECT bt_index_parent_check('test_idx', heapallindexed => true); + \\sleep 10 ms + + SELECT pg_advisory_unlock(42); + \\else + SELECT pg_advisory_lock(43); + BEGIN; + INSERT INTO tbl(val) VALUES (nextval('last_j')) RETURNING val \\gset p_ + COMMIT; + SELECT pg_advisory_unlock(43); + \\sleep 1 ms + + BEGIN + --TRANSACTION ISOLATION LEVEL REPEATABLE READ + ; + SELECT 1; + \\sleep 1 ms + SELECT COUNT(*) AS count FROM tbl WHERE val <= :p_j \\gset p_ + \\if :p_count != :p_j + COMMIT; + SELECT (:p_count) / 0; + \\endif + + COMMIT; + \\endif + ) +}); + +$node->stop; +done_testing(); -- 2.47.3 --tbdehtstrqwksfdh Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="0003-Distinguish-properly-when-database-specific-transact.patch" ^ permalink raw reply [nested|flat] 32+ messages in thread
end of thread, other threads:[~2025-12-13 17:46 UTC | newest] Thread overview: 32+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2013-11-14 23:07 strncpy is not a safe version of strcpy David Rowley <[email protected]> 2013-11-14 23:33 ` Tomas Vondra <[email protected]> 2013-11-15 00:00 ` David Rowley <[email protected]> 2013-11-15 03:21 ` Tomas Vondra <[email protected]> 2013-11-15 14:24 ` Stephen Frost <[email protected]> 2013-11-15 14:35 ` Andres Freund <[email protected]> 2013-11-15 14:32 ` Kevin Grittner <[email protected]> 2013-11-15 14:41 ` Andres Freund <[email protected]> 2013-11-15 14:53 ` Stephen Frost <[email protected]> 2013-11-15 14:56 ` Andres Freund <[email protected]> 2013-11-15 15:04 ` Stephen Frost <[email protected]> 2013-11-15 15:01 ` Andres Freund <[email protected]> 2013-11-15 15:25 ` Tom Lane <[email protected]> 2013-11-15 15:48 ` Stephen Frost <[email protected]> 2013-11-15 15:09 ` Alvaro Herrera <[email protected]> 2013-11-15 15:18 ` Kevin Grittner <[email protected]> 2013-11-15 15:27 ` Alvaro Herrera <[email protected]> 2013-11-15 15:56 ` Kevin Grittner <[email protected]> 2013-11-15 23:53 ` David Rowley <[email protected]> 2014-08-13 03:19 ` Noah Misch <[email protected]> 2014-08-13 10:26 ` David Rowley <[email protected]> 2014-08-13 13:31 ` Kevin Grittner <[email protected]> 2014-08-13 14:21 ` Tom Lane <[email protected]> 2014-08-13 14:55 ` Kevin Grittner <[email protected]> 2014-08-14 04:13 ` Noah Misch <[email protected]> 2014-08-14 06:50 ` Tom Lane <[email protected]> 2014-08-15 06:09 ` Noah Misch <[email protected]> 2014-08-15 22:38 ` David Rowley <[email protected]> 2014-08-16 03:26 ` Noah Misch <[email protected]> 2014-08-19 03:15 ` Noah Misch <[email protected]> 2014-08-13 14:23 ` Heikki Linnakangas <[email protected]> 2025-12-13 17:46 [PATCH 2/3] one more stress test for repack concurrently Mikhail Nikalayeu <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox