Bug 1320593

Small update : 

forget this part : 

"4. In the process of trying this out, i found out, that --password=dbpassword ...."  

was a different error.

Hi,
unfortunately, I wasn't able to reproduce this issue with Python (which I am familiar with) and mariadb-10.0.25 (current version of MariaDB in Fedora 23).
Could You please provide me with the sample Java code (which I am not as much familiar with) for me to be able to reproduce it?

Thank You,
QB

i don't have the time to write it down completly ATM, 
but for now, consult this website : 

http://marius.bloggt-in-braunschweig.de/2016/04/29/solution-mysqldump-no-database-selected-when-selecting-the-database/

It explains it in detail. you may have to use google translate, but it has an english section.

I did my best to create reproducer in Java, however mysqldump works well for me.

I had not time to do this yesterday, so here it is now:


                        System.out.println("MYSQLDUMP gestartet für User "+myJobId+" "+dbname+" am "+ new Date().toString() );

                        String cmd = ".....chrootwrap "+ Username +" /usr/bin/mysqldump  --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password="+dbpass+" --user="+dbuser+" --databases  "+dbname;

                        String r = dos.readPipe( cmd ) +"\n\n";

The above line of code is already the workaround version with --databases .


where DOS contains: 

        public String readPipe(String s) {
                return readPipe(s,null);
        }

        public String readPipe(String s,String stdin) {
                String output = "";

                Process process;
                try {
                        Runtime task = java.lang.Runtime.getRuntime();
                        if ( s.indexOf(" ")>0)  {

                                StringHash params = new StringHash();
                                int c = 0;
                                while ( s.indexOf("\"")>=0 ) {
//                                      System.out.println( "in="+s ) ;
                                        String arg = zwischen( s , "\"", "\"" ); // extracts text between " and "
                                        params.put("<ARG"+c+">", arg );
                                        s = s.replaceAll( "\""+arg+"\"", "<ARG"+c+">" );
//                                      System.out.println( "out="+s ) ;
                                        c++;
                                }

                                String[] cmds = s.split(" ");
                                for(int i=0;i<cmds.length;i++) {
                                        for(int a=0;a<=c;a++) cmds[i] = cmds[i]. replaceAll( "<ARG"+a+">", params.get( "<ARG"+a+">" ) );
//                                      System.out.println(i +". => "+cmds[i] );
                                }
                                process = task.exec(cmds);
                        } else process = task.exec(s);
                        
                        if ( stdin != null ) {
        
                                BufferedWriter out = new BufferedWriter( new OutputStreamWriter(process.getOutputStream()) );
                                out.write(stdin+"\n");
                                out.flush();
                                out.close();
                        }

                        String lines ="";
                        BufferedReader ins = new BufferedReader(new InputStreamReader(process.getInputStream()));
                        while ((lines = ins.readLine())!=null) {
                                output += lines+"\n";
                        }

                        process.waitFor();

                        ins = new BufferedReader(new InputStreamReader(process.getErrorStream()));
                        while ((lines = ins.readLine())!=null) {
                                output += lines+"\n";
                        }
                        
                        while ((lines = ins.readLine())!=null) {
                                output += lines+"\n";
                        }

                        process.destroy();
                } catch (Exception e) {
                        return e.toString();
                }
                return output;
        } 

And chrootwrap does this : 

       chdir("/opt/root/");
       chroot("/opt/root/");
       ...
       execve( argv[2], newargv, environ);
       printf( "EXEC-Error" );
       return 0;

It rearranges the argumentlist into "newargv", drops to a user arg[0] and executes the tool the cmdlines arg[1] is pointing to.

-rwsr-x--- 1 root root


hope that helps now..

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

This message is a reminder that Fedora 23 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 23. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '23'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 23 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Just some thoughts, before I forget them:

  - the version "mariadb-10.0.23-1.fc22.i686" against which was the bug 
    originally reported can't be even installed anymore. In this case, however,
    it is fine to test this against current rawhide

  - the issue with the java exec could be, that it expands empty variable to 
    *nothing*. If it would expand to *nothing in parentheses*, the script would 
    took the argument correctly.
    ^ need tests to prove this ^

"
  - the issue with the java exec could be, that it expands empty variable to 
    *nothing*. If it would expand to *nothing in parentheses*, the script would 
    took the argument correctly.
    ^ need tests to prove this ^
"

That has already been tested, how do you think i got it working :D

There were no "" involved nor were there empty variables:

so the entire string is expanded before sending it to exec and it got checked before:
                         
                        ...

                        if ( secCheck( dbname ) || secCheck( dbpass ) || secCheck( dbuser ) ) {

                        ...

                        System.out.println("MYSQLDUMP gestartet für User "+myJobId+" "+dbname+" am "+ new Date().toString() );

                        String cmd = "/java/rdt/tools/chrootwrap "+myJobId+" /usr/bin/mysqldump  --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password="+dbpass+" --user="+dbuser+" "+dbname;

                        String result = "Ergebnisse des Backups:\n\n";

                        String r = dos.readPipe( cmd ) +"\n\n";

And an empty dbname would have been spotted in the logs easily. I even made a small test app for it.

readPipe() is using these calls:

                Process process;
                try {
                        Runtime task = java.lang.Runtime.getRuntime();
                        if ( s.indexOf(" ")>0)  {
                                ...
                                String[] cmds = s.split(" ");
                                ...
                                for(int i=0;i<cmds.length;i++) {
                                        ...
//                                      System.out.println(i +". => "+cmds[i] );
                                }
                                process = task.exec(cmds);

Actually, that code is much bigger, as it tests for dangerouse regexes and some other stuff. In the end, it gets exec()ed .


As the bug fix solely was made outside of any underlaying routines and chrootcalls etc. etc. by exchanging this:

String cmd = "/java/rdt/tools/chrootwrap "+myJobId+" /usr/bin/mysqldump  --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password="+dbpass+" --user="+dbuser+" "+dbname;

with this:

String cmd = "/java/rdt/tools/chrootwrap "+myJobId+" /usr/bin/mysqldump  --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password="+dbpass+" --user="+dbuser+" --databases "+dbname;

any theory of "the last argument got lost on the way" must be false, because the dbname stayed the last argument and with --databases upfront it works as intended :)

But, I would accept a strange "even/odd" number of arguments reason ;)

After looking on your code, I reproduced what happens.
You parse command by a single space, but in your code your command looks like:
/usr/bin/mysqldump  --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password="+dbpass+" --user="+dbuser+" "+dbname;
------------------^^ two spaces
Therefore your code execute command:
/usr/bin/mysqldump "" --add-locks -e --force -R --triggers --add-drop-table --hex-blob -h localhost --password=pass --user=user dbname
-------------------^^ this is actually first positional argument
According to mysqldump help:
"
Usage: mysqldump [OPTIONS] database [tables]
OR     mysqldump [OPTIONS] --databases [OPTIONS] DB1 [DB2 DB3...]
"
You use first variant and only first positional argument is taken as database, this is why your dbname is ignored, because "" database does not exist and your dbname is actually table name according to syntax.(bugs 1,2)
In your workaround you use second variant and all positional arguments are taken as databases. This is why you see dump of your database and then error about not selecting database. (bug 3)

I impressed, i totally overlooked that space, but isn't it very stupid to parse this as a database name ? And why is it correctly parsed as soon as bash is involved ?

mysqldump looking for first positional argument, and in your example it is "", because you explicitly created it in your Java code.

In Bash it does not matter how many blank spaces you leave between arguments, it will strip them and does not treat two spaces as argument, unless you use "" or ''.

Closing issue. If the above reply did not help, feel free to reopen.

Also, if you feel, that some error messages should be updated, gather a list of them and your proposal.
I can take them to the upstream, although I don't give them much chances to be accepted, because some DB admins in production rely on their specific wording.

Only one question: How can "" be a valid argument aka a valid Databasename ? 

Because of this question, i really like to suggest to change the 

"mysqldump: Got error: 1046: No database selected when selecting the database"

to

"mysqldump: Got error: XXXX: i think we got a space on the commandline, that is not a valid databasename"

People will still be confused, no old string needs to be changed, but people will be pointed into the right direction. If you find this sugestion  	ridiculous, that's because it is :)  I still think, parsing a " " to an argument at all is a parser bug.

Ok. I agree, that could be take as a feature request atleast.

Here's the upstream issue created for you:

https://jira.mariadb.org/browse/MDEV-14449

--

Once some outcome will be made, I'll notify you here, so you don't need to watch it all the time. (But sure, you can create an account there and join the discussion)

Thanks. 

You did a lot to bring light into the matter and i fixed my call. You don't need to report it here, as it's only me and your redhat colleagues. 

I hope, you stay as awesome on all your bugreports, as you did on this one :)
So, thank you.

And so you obtained your answen on the upstream.

In that case, I'd agree with Elena and her arguments. It didn't thought about checking the standard core utilities - which could be totally taken as an example.

--

With your approval, I'll ask closing of the upstream issue.

If you'd like to talk about it with the upstream in a semi-formal way, there is always their IRC channels and mailing list :)

TBH: "" was a bad example to start with, because thats actually empty where "mysqldump  --option" is just a simple space infront of a real option.

After thinking about it, I came to the conclusion, that '  --option' is correctly parsed, under the rule, that a space is a seperator char and there for, 2 spaces seperate an empty argument. But, it's still a so common mistake, which is why bash is normalizing it away. 

If they won't change it, maybe we could go with the error message plan, but formulate it a bit accurate for the actual problem ?

One thing i don't get: 

even IF the first " --" is parsed as an empty dbname, there was a second name at the end of the string, why does it stop ? It could process the second name and give out "database '' given,but not found." and continue.

Does make way more sense than the original message and behaviour ;)

Whatever the mariadb team decides, my blog is one of 5 pages covering the errormessage. I win anyway :D

(In reply to customercare from comment #19)
> After thinking about it, I came to the conclusion, that '  --option' is
> correctly parsed, under the rule, that a space is a seperator char and there
> for, 2 spaces seperate an empty argument. But, it's still a so common
> mistake, which is why bash is normalizing it away. 

Usually this does not happen in Java, problem is that you tokenize with your own code. See [1],
you can pass whole string into Runtime.exec and it will be properly parsed by Java StringTokenizer.

[1]  https://docs.oracle.com/javase/7/docs/api/java/lang/Runtime.html#exec(java.lang.String,%20java.lang.String[],%20java.io.File)