On Thu, Jul 19, 2012 at 02:24:36AM -0400, Bruce Momjian wrote:
> On Wed, Jul 18, 2012 at 01:30:34AM -0400, Tom Lane wrote:
> > I wrote:
> > > ... I'm wondering if maybe
> > > Solaris has a weird definition of struct dirent that breaks the
> > > calculation used here:
> >
> > > entrysize = sizeof(struct dirent) - sizeof(direntry->d_name) +
> > > strlen(direntry->d_name) + 1;
> >
> > Some googling suggests that on Solaris, this calculation would actually
> > give a result that's a little too *large* not too small, because of
> > padding in the declaration of struct dirent. That would not be a
> > problem so far as callers were concerned, but it's conceivable that the
> > memcpy in load_directory would run off the end of memory and crash.
> > Could that explain Mike's problem? You'd have to assume that the stack
> > trace he showed us had omitted to mention load_directory, but I don't
> > have a huge problem with making such an assumption. Anyway the offsetof
> > formulation should avoid this hazard, so I'm still interested to know
> > if that helps.
>
> Great to hear this fixed the problem. The only way I can see this
> causing a crash is:
>
> * the padding of direntry->d_name is less than the padding of
> struct dirent
> * there is a non-mapped memory range after the struct
> * the file name is near the full length of d_name
>
> Those are pretty rare odds. The only other issue is that this code
> assumes direntry->d_name is the last element of the struct, and I am not
> sure how we can assume that is true.
>
> Also, this code tries to be tricky by reducing the allocated memory
> instead of allocating the maximum path. Shouldn't there a comment
> about the propose of this code?
This bug report was fixed mostly via private email because a private
schema dump was sent to Tom and me. Tom fixed the problem by changing
the way we interact with struct dirent, but neither of us is sure
exactly why the fix worked. It is something about Solaris.
Tom suggested that load_directory() return a (char *) array, rather than
a struct dirent array, greatly simplifying the code.
I have done this in the attached patch, and because of the uncertainty
of ths fix, I would like to apply this to 9.2 as well.
--
Bruce Momjian <br...@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/file.c b/contrib/pg_upgrade/file.c
new file mode 100644
index 962cbac..047c262
*** a/contrib/pg_upgrade/file.c
--- b/contrib/pg_upgrade/file.c
*************** copy_file(const char *srcfile, const cha
*** 232,247 ****
* namelist array itself.
*/
int
! load_directory(const char *dirname, struct dirent *** namelist)
{
DIR *dirdesc;
struct dirent *direntry;
int count = 0;
! int allocsize = 64;
! size_t entrysize;
! *namelist = (struct dirent **)
! pg_malloc(allocsize * sizeof(struct dirent *));
if ((dirdesc = opendir(dirname)) == NULL)
pg_log(PG_FATAL, "could not open directory \"%s\": %s\n",
--- 232,245 ----
* namelist array itself.
*/
int
! load_directory(const char *dirname, char ***namelist)
{
DIR *dirdesc;
struct dirent *direntry;
int count = 0;
! int allocsize = 64; /* initial array size */
! *namelist = (char **) pg_malloc(allocsize * sizeof(char *));
if ((dirdesc = opendir(dirname)) == NULL)
pg_log(PG_FATAL, "could not open directory \"%s\": %s\n",
*************** load_directory(const char *dirname, stru
*** 252,269 ****
if (count >= allocsize)
{
allocsize *= 2;
! *namelist = (struct dirent **)
! pg_realloc(*namelist, allocsize * sizeof(struct dirent *));
}
! entrysize = offsetof(struct dirent, d_name) +
! strlen(direntry->d_name) + 1;
!
! (*namelist)[count] = (struct dirent *) pg_malloc(entrysize);
!
! memcpy((*namelist)[count], direntry, entrysize);
!
! count++;
}
#ifdef WIN32
--- 250,260 ----
if (count >= allocsize)
{
allocsize *= 2;
! *namelist = (char **)
! pg_realloc(*namelist, allocsize * sizeof(char *));
}
! (*namelist)[count++] = pg_strdup(direntry->d_name);
}
#ifdef WIN32
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index f0d84a0..9a23176
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** const char *setupPageConverter(pageCnvCt
*** 356,362 ****
typedef void *pageCnvCtx;
#endif
! int load_directory(const char *dirname, struct dirent *** namelist);
const char *copyAndUpdateFile(pageCnvCtx *pageConverter, const char *src,
const char *dst, bool force);
const char *linkAndUpdateFile(pageCnvCtx *pageConverter, const char *src,
--- 356,362 ----
typedef void *pageCnvCtx;
#endif
! int load_directory(const char *dirname, char ***namelist);
const char *copyAndUpdateFile(pageCnvCtx *pageConverter, const char *src,
const char *dst, bool force);
const char *linkAndUpdateFile(pageCnvCtx *pageConverter, const char *src,
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 659b614..7688914
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*************** transfer_single_new_db(pageCnvCtx *pageC
*** 133,139 ****
{
char old_dir[MAXPGPATH];
char file_pattern[MAXPGPATH];
! struct dirent **namelist = NULL;
int numFiles = 0;
int mapnum;
int fileno;
--- 133,139 ----
{
char old_dir[MAXPGPATH];
char file_pattern[MAXPGPATH];
! char **namelist = NULL;
int numFiles = 0;
int mapnum;
int fileno;
*************** transfer_single_new_db(pageCnvCtx *pageC
*** 192,212 ****
for (fileno = 0; fileno < numFiles; fileno++)
{
! char *vm_offset = strstr(namelist[fileno]->d_name, "_vm");
bool is_vm_file = false;
/* Is a visibility map file? (name ends with _vm) */
if (vm_offset && strlen(vm_offset) == strlen("_vm"))
is_vm_file = true;
! if (strncmp(namelist[fileno]->d_name, file_pattern,
strlen(file_pattern)) == 0 &&
(!is_vm_file || !vm_crashsafe_change))
{
snprintf(old_file, sizeof(old_file), "%s/%s", maps[mapnum].old_dir,
! namelist[fileno]->d_name);
snprintf(new_file, sizeof(new_file), "%s/%u%s", maps[mapnum].new_dir,
! maps[mapnum].new_relfilenode, strchr(namelist[fileno]->d_name, '_'));
unlink(new_file);
transfer_relfile(pageConverter, old_file, new_file,
--- 192,212 ----
for (fileno = 0; fileno < numFiles; fileno++)
{
! char *vm_offset = strstr(namelist[fileno], "_vm");
bool is_vm_file = false;
/* Is a visibility map file? (name ends with _vm) */
if (vm_offset && strlen(vm_offset) == strlen("_vm"))
is_vm_file = true;
! if (strncmp(namelist[fileno], file_pattern,
strlen(file_pattern)) == 0 &&
(!is_vm_file || !vm_crashsafe_change))
{
snprintf(old_file, sizeof(old_file), "%s/%s", maps[mapnum].old_dir,
! namelist[fileno]);
snprintf(new_file, sizeof(new_file), "%s/%u%s", maps[mapnum].new_dir,
! maps[mapnum].new_relfilenode, strchr(namelist[fileno], '_'));
unlink(new_file);
transfer_relfile(pageConverter, old_file, new_file,
*************** transfer_single_new_db(pageCnvCtx *pageC
*** 227,239 ****
for (fileno = 0; fileno < numFiles; fileno++)
{
! if (strncmp(namelist[fileno]->d_name, file_pattern,
strlen(file_pattern)) == 0)
{
snprintf(old_file, sizeof(old_file), "%s/%s", maps[mapnum].old_dir,
! namelist[fileno]->d_name);
snprintf(new_file, sizeof(new_file), "%s/%u%s", maps[mapnum].new_dir,
! maps[mapnum].new_relfilenode, strchr(namelist[fileno]->d_name, '.'));
unlink(new_file);
transfer_relfile(pageConverter, old_file, new_file,
--- 227,239 ----
for (fileno = 0; fileno < numFiles; fileno++)
{
! if (strncmp(namelist[fileno], file_pattern,
strlen(file_pattern)) == 0)
{
snprintf(old_file, sizeof(old_file), "%s/%s", maps[mapnum].old_dir,
! namelist[fileno]);
snprintf(new_file, sizeof(new_file), "%s/%u%s", maps[mapnum].new_dir,
! maps[mapnum].new_relfilenode, strchr(namelist[fileno], '.'));
unlink(new_file);
transfer_relfile(pageConverter, old_file, new_file,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
