Re: pgsql: Exclude unlogged tables from base backups

From: David Steele <david(at)pgmasters(dot)net>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Teodor Sigaev <teodor(at)sigaev(dot)ru>
Cc: pgsql-committers(at)lists(dot)postgresql(dot)org
Subject: Re: pgsql: Exclude unlogged tables from base backups
Date: 2018-03-25 19:10:03
Message-ID: 8d57c3ba-d44b-a8eb-7a91-c0b6156240fd@pgmasters.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

On 3/25/18 2:16 PM, Tom Lane wrote:
> Teodor Sigaev <teodor(at)sigaev(dot)ru> writes:
>> Exclude unlogged tables from base backups
>
> Buildfarm member skink (valgrind) has reported this during its last couple
> of runs:
>
> 2018-03-24 03:18:23.409 UTC [17302] 010_pg_basebackup.pl LOG: received replication command: BASE_BACKUP LABEL 'pg_basebackup base backup' NOWAIT TABLESPACE_MAP
> ==17302== VALGRINDERROR-BEGIN
> ==17302== Conditional jump or move depends on uninitialised value(s)
> ==17302== at 0x4C2FD88: strlen (vg_replace_strmem.c:458)
> ==17302== by 0x6FBA3AB: vfprintf (vfprintf.c:1643)
> ==17302== by 0x6FE19AF: vsnprintf (vsnprintf.c:114)
> ==17302== by 0x6FC0F6E: snprintf (snprintf.c:33)
> ==17302== by 0x4A3C97: sendDir (basebackup.c:1060)
> ==17302== by 0x4A40C6: sendDir (basebackup.c:1221)
> ==17302== by 0x4A52B2: sendTablespace (basebackup.c:937)
> ==17302== by 0x4A5740: perform_base_backup (basebackup.c:313)
> ==17302== by 0x4A635C: SendBaseBackup (basebackup.c:710)
> ==17302== by 0x49FCF2: exec_replication_command (walsender.c:1520)
> ==17302== by 0x4F61CE: PostgresMain (postgres.c:4143)
> ==17302== by 0x4712ED: BackendRun (postmaster.c:4409)
> ==17302== Uninitialised value was created by a stack allocation
> ==17302== at 0x4A38EA: sendDir (basebackup.c:957)
> ==17302==
> ==17302== VALGRINDERROR-END
> 2018-03-24 03:18:26.127 UTC [17302] 010_pg_basebackup.pl DEBUG: contents of directory "pg_serial" excluded from backup
>
> I might be wrong to blame that on this patch, but nothing else has
> touched basebackup.c lately.
Sure looks like it deserves the blame -- the error is occurring right in
the new code.

I think skink is using large values for rel oids and that has exposed a
bug. The strncpy doesn't zero terminate the string if the oid has the
max number of characters. At least, I was able to reproduce under those
circumstances.

The attached should fix it.

Regards,
--
-David
david(at)pgmasters(dot)net

Attachment Content-Type Size
basebackup.patch text/plain 619 bytes

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Tom Lane 2018-03-25 19:15:49 pgsql: Fix unsafe extraction of the OID part of a relation filename.
Previous Message Peter Eisentraut 2018-03-25 19:01:00 pgsql: pg_resetwal: Allow users to change the WAL segment size