I do agree with your idea of removing the invocation of the package command.
I'm not sure about removing ptr and reusing another pointer. I've seen interesting bugs in different environments from this. And we're talking about making code harder to read to save a whole int-sized pointer.
I'm also not sure on your reasons for changing the output to not include the diagnosed command. Can you explain? Do you think this should be verbose instead?
On the lines of output, not including a package is notable and should be reported so I changed the output from Verbose to normal output.
Revised patch attached.
package.c-buildcmd.patch
Description: Binary data
On Jul 20, 2007, at 5:14 PM, [EMAIL PROTECTED] wrote:
This is a variety of fixes to BuildCommandLine to avoid buffer overflows and the like. Before, the copy functions didn't check whether they were overflowwing CF_BUFFSIZE*2 (the size given to the install string by InstallPackage), and it also would pass off commands to cfpopen that had more than CF_MAXSHELLARGS arguments, meaning that the commands would consistantly fail if you had more than about 15 packages to install with a few package manager flags thrown in.I've also removed the code that attempts to run the package command without the tail string (any characters after %s), as this won't work in all situations, for examlpe: MyPackageInstallCommand = ( "/ bin/sh -c 'realinstallcmd %s | filter'" ). Sure, running it through sh might be a security weakness on the person with that setup, but having the code try to execl "/bin/sh -c 'realinstallcmd" can have strange side effects. There's sufficent warning if the command fails in InstallPackage, I don't think any is needed here.Index: cfengine-trunk/src/package.c =================================================================== --- cfengine-trunk.orig/src/package.c +++ cfengine-trunk/src/package.c @@ -403,13 +403,13 @@ int BuildCommandLine(char *resolvedcmd, { struct Item *package; char *cmd_tail; FILE *pp; - char *ptr, *cmd_ptr; + char *cmd_ptr; int cmd_args = 0; int cmd_tail_len = 0; - int original_len; + int cmd_head_len = 0; /* How many words are there in the package manager invocation? */- for (ptr = rawcmd, cmd_args = 1; NULL != ptr; ptr = strchr(+ +ptr, ' ')) + for (cmd_ptr = rawcmd, cmd_args = 1; NULL != cmd_ptr; cmd_ptr = strchr(++cmd_ptr, ' ')){ ++cmd_args; } @@ -428,39 +428,39 @@ int BuildCommandLine(char *resolvedcmd, /* Copy the initial part of the install command */ strncpy(resolvedcmd, rawcmd, CF_BUFSIZE*2);- snprintf(OUTPUT,CF_BUFSIZE,"Package manager will be invoked as % s\n", resolvedcmd);- CfLog(cfinform,OUTPUT,""); - - if ((pp = cfpopen(resolvedcmd, "r")) == NULL) - { - Verbose("Could not execute package manager\n"); - /* Return that the package is still not installed */ - return 0; - } -/* Iterator through package list until we reach the maximum number that cfpopen can take */- original_len = strlen(resolvedcmd); - cmd_ptr = &resolvedcmd[original_len]; + cmd_head_len = strlen(resolvedcmd); + cmd_ptr = &resolvedcmd[cmd_head_len]; for (package = pkglist; package != NULL; package = package->next) {- Verbose("BuildCommandLine(): Adding package '%s' to the list. \n", package->name);+ if (cmd_args == CF_MAXSHELLARGS) + {+ Verbose("BuildCommandLine(): Skipping package %s (reached CF_MAXSHELLARGS)\n", package->name);+ }+ else if ((int) cmd_ptr - (int) resolvedcmd + strlen(package- >name) + 2 > CF_BUFSIZE*2)+ {+ Verbose("BuildCommandLine(): Skipping package %s (reached 2 * CF_BUFFSIZE)\n", package->name);+ } + else + {+ Verbose("BuildCommandLine(): Adding package '%s' to the list.\n", package->name);- strncpy(cmd_ptr, package->name, &resolvedcmd[CF_BUFSIZE*2] - cmd_ptr);- cmd_ptr += strlen(package->name); - *cmd_ptr++ = ' ';+ strncpy(cmd_ptr, package->name, &resolvedcmd [CF_BUFSIZE*2] - cmd_ptr);+ cmd_ptr += strlen(package->name); + *cmd_ptr++ = ' '; + ++cmd_args; + } } /* Remove trailing space */ *--cmd_ptr = '\0'; - if (cmd_ptr != &resolvedcmd[original_len]) + /* Append tail if necessary */ + if (cmd_tail_len > 0) {- /* Have a full command line, so append the tail if necessary. */- if (cmd_tail_len > 0) - {- strncpy(cmd_ptr, cmd_tail, &resolvedcmd[CF_BUFSIZE*2] - cmd_ptr);- }+ strncpy(cmd_ptr, cmd_tail, &resolvedcmd[CF_BUFSIZE*2] - cmd_ptr);} + return 1; } -- Eric Searcy OSU Open Source Lab _______________________________________________ Bug-cfengine mailing list [email protected] https://cfengine.org/mailman/listinfo/bug-cfengine
-- Jo Rhett senior geek Silicon Valley Colocation Support Phone: 408-400-0550
_______________________________________________ Bug-cfengine mailing list [email protected] https://cfengine.org/mailman/listinfo/bug-cfengine
