percona-xtrabackup icon indicating copy to clipboard operation
percona-xtrabackup copied to clipboard

Bug fix for https://bugs.launchpad.net/percona-xtrabackup/2.3/+bug/1642329

Open elshadaghazade opened this issue 7 years ago • 12 comments

The reason of the bug was that, strcend function puts the pointer to the end of the string in argv: optend = strcend(argv[i], '='); If we send only -- -- string at the parameter than we enter the first if: if (strncmp(argv[i], "--defaults-group", optend - argv[i]) == 0) { defaults_group = optend + 1; append_defaults_group(defaults_group); }

As you see the value of the defaults_group is optend + 1. This makes a problem, because after the argv massive in the RAM comes some system ENV strings like TERM_PROGRAM=Apple_Terminal, SHELL=/bin/bash and etc. If you print defaults_group you can see this result. It seems OS keeps argv massive near the system environment strings in the RAM. For this reason I copied the argv strings to the temporary string object to keep them in the secure place of RAM, then I gave this string to the strcend function like first parameter. After that, program worked correctly.

elshadaghazade avatar Nov 18 '16 13:11 elshadaghazade

The bug analysis is correct, but the fix is not correct. It doesn't fix described problem. It just breaks handling of --defaults-group entirely, so (strncmp(argv[i], "--defaults-group", optend - argv[i]) == 0) will never be true.

gl-sergei avatar Nov 18 '16 16:11 gl-sergei

Yes, you are right Sergei. It is not a completed fix. I was so tired when I wrote this :) But I updated it now. I guess It will be correct now. Thanks a lot

elshadaghazade avatar Nov 18 '16 21:11 elshadaghazade

And now you came back to what you started with. If you would try to run xtrabackup with your patch, you would see that it is failing the same way as without your patch...

xtrabackup --backup --target-dir=./ -- -- -- -- --
2016-11-19 04:12:04 7fffa6fac3c0  InnoDB: Assertion failure in thread 140735994840000 in file xtrabackup.cc line 6692
InnoDB: Failing assertion: appended
InnoDB: We intentionally generate a memory trap.
InnoDB: Submit a detailed bug report to http://bugs.mysql.com.
InnoDB: If you get repeated assertion failures or crashes, even
InnoDB: immediately after the mysqld startup, there may be
InnoDB: corruption in the InnoDB tablespace. Please refer to
InnoDB: http://dev.mysql.com/doc/refman/5.6/en/forcing-innodb-recovery.html
InnoDB: about forcing recovery.
21:12:04 UTC - xtrabackup got signal 6 ;
This could be because you hit a bug or data is corrupted.
This error can also be caused by malfunctioning hardware.
We will try our best to scrape up some info that will hopefully help
diagnose the problem, but since we have already crashed, 
something is definitely wrong and this may fail.

Thread pointer: 0x0
Attempting backtrace. You can use the following information to find out
where mysqld died. If you see no messages after this, something went
terribly wrong...
stack_bottom = 0 thread_stack 0x10000
0   xtrabackup                          0x000000010eeaf90d my_print_stacktrace + 61
1   xtrabackup                          0x000000010f227d1a handle_fatal_signal + 458
2   libsystem_platform.dylib            0x00007fff9e499bba _sigtramp + 26
3   ???                                 0x000000011f6a4291 0x0 + 4822024849
4   libsystem_c.dylib                   0x00007fff9e320420 abort + 129
5   xtrabackup                          0x000000010ee4de0b main + 859
6   libdyld.dylib                       0x00007fff9e28c255 start + 1
7   ???                                 0x0000000000000008 0x0 + 8

Please report a bug at https://bugs.launchpad.net/percona-xtrabackup

gl-sergei avatar Nov 18 '16 21:11 gl-sergei

Sergei, But when I run xtrabackup I get these results:

[eagayev@localhost bin]$ ./xtrabackup -- -- xtrabackup: Error: --defaults-file must be specified first on the command line

[eagayev@localhost bin]$ --backup --target-dir=./ -- -- -- -- -- bash: --backup: command not found...

elshadaghazade avatar Nov 18 '16 21:11 elshadaghazade

Elshad,

Just run it as I suggested:

./xtrabackup --backup --target-dir=./ -- -- -- -- --

Here is an explanation:

  1. Look at the condition if (strncmp(tmp_argv.c_str(), "--defaults-group", optend - tmp_argv.c_str()) == 0). This condition is true for argv[i]=="--".
  2. Your point about defaults_group being out of tmp_argv bounds is also true.
  3. Each occurrence of -- appended to the xb_load_default_groups which triggers assertion appended in the append_defaults_group.

These tree points valid both for xtrabackup with your patch and without your patch. There are three issues to address.

Your patch makes things even worse. It adds fourth issue when it passing pointer to deallocated memory buffer (tmp_argv->c_str() + N) to load_defaults.

Can you please test your work better before your submit it next time.

gl-sergei avatar Nov 18 '16 22:11 gl-sergei

You are right Sergei. Thank you for your usefull advices. It is pleasure for me to work on this project. I'm trying to find better way to fix it now.

elshadaghazade avatar Nov 18 '16 22:11 elshadaghazade

Sergei, please check again. I tried to fix it with a new way. But I'm not sure it is a right way. I wait for your answer and advice. Thanks

elshadaghazade avatar Nov 18 '16 23:11 elshadaghazade

Sergei, I tested it. It seems everythink is ok.

elshadaghazade avatar Nov 20 '16 14:11 elshadaghazade

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 21 '19 17:10 CLAassistant

Hello, I've signed CLA. You can accept my contribution.

Best regards, Elshad

On Mon, Oct 21, 2019, 9:52 PM CLAassistant [email protected] wrote:

[image: CLA assistant check] https://cla-assistant.io/percona/percona-xtrabackup?pullRequest=273 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement https://cla-assistant.io/percona/percona-xtrabackup?pullRequest=273 before we can accept your contribution. You have signed the CLA already but the status is still pending? Let us recheck https://cla-assistant.io/check/percona/percona-xtrabackup?pullRequest=273 it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/percona/percona-xtrabackup/pull/273?email_source=notifications&email_token=AA3QETCJS7NRZB464CFFLBTQPXT6NA5CNFSM4CW2ALF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB3GSPA#issuecomment-544631100, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3QETBCNREXW5XG3T6SECDQPXT6NANCNFSM4CW2ALFQ .

elshadaghazade avatar Oct 23 '19 04:10 elshadaghazade

Hi @elshadaghazade . I'm picking up this PR. Would you be interested in providing a patch based on the 8.0 branch? 2.3 is already EOL and we will be happy to accept this patch based on 8.0 version.

Thanks in advance for your contribution.

altmannmarcelo avatar Jul 29 '22 13:07 altmannmarcelo

Hi @altmannmarcelo , sure. With great pleasure!

elshadaghazade avatar Jul 30 '22 05:07 elshadaghazade

Hi @elshadaghazade , closing this PR due to inactivity. Please open a new PR if you wish to contribute further.

altmannmarcelo avatar Jul 31 '23 19:07 altmannmarcelo