Hi Michael,
On 11/15/17 10:09 PM, Michael Paquier wrote:
> On Thu, Nov 16, 2017 at 9:20 AM, David Steele <david@pgmasters.net> wrote:
>> For this patch at least, I think we should do #1. Getting rid of the order
>> dependency is attractive but there may be other programs that are depending
>> on the order. I know you are not proposing to change the order now, but it
>> *could* be changed with #2.
>
> read_backup_label() is a static function in the backend code. With #2
> I do not imply to change the order of the elements written in the
> backup_label file, just to make the way they are parsed smarter.
My point is that the order *could* be changed and it wouldn't be noticed
if the read function were improved as you propose.
I'm not against the idea, just think we should continue to enforce the
order unless we decide an interface break is OK.
>> Also, I think DEBUG1 would be a more appropriate log level if any logging is
>> done.
>
> OK, the label and time strings can include spaces. The label string is
> assumed to not be bigger than 1028 bytes, and the timestamp is assumed
> that it can get up to 128. So it is possible to use stuff like
> fscanf("%127[^\n]\n") to avoid any buffer overflow problems on the
> backend. If a backup_label is generated with strings longer than that,
> then read_backup_label would fail to parse the next entries but that's
> not really something to worry about IMO as those are only minor sanity
> checks. Having a safer coding in the backend is more important, and
> the new checks should not trigger any hard failures.
I have tested and get an error as expected when I munge the backup_label
file:
FATAL: invalid data in file "backup_label"
DETAIL: Timeline ID parsed is 2, but expected 1
Everything else looks good so I will mark it ready for committer.
Regards,
--
-David
david@pgmasters.net