Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#60 pngcrush: crash on invalid arguments

None
closed
None
1
2014-06-27
2013-09-26
Zack Weinberg
No

(Originally reported as http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=716149 .)

Various command line arguments do not check properly for running into the end of the argument list, thus for instance

$ pngcrush -bkgd
Segmentation fault

Code inspection suggests that the BUMP_I macro is supposed to be used throughout argument parsing to detect end-of-argv, but several options blindly use argv[++i] instead: at least -bkgd, -fil, -lev, -iccp, -trns_a, -trns, -w, -zm, and -z. (The -text-and-friends block appears to be correct despite containing several instances of argv[++i], as there is an initial check.)

I also observe that use of atoi instead of strtol causes errors to go undetected (consider -already 123cheesesandwich) but perhaps you would prefer to put effort into switching to some less ad-hoc command line parser than in fixing non-catastrophic bugs in this one.

Discussion

  • Thanks. I revised pngcrush to use BUMP_I everywhere.

    Also I changed atoi to strtol, but found that at least on my platform,
    strtol does not set errno to a nonzero value even if there is an error:

    After replacing atoi(argv[i])
    with
    (errno=0,strtol(argv[i],endptr,10))
    if (errno != 0) {fprintf(STDERR, "malformed or missing argument\n");
    exit(1);}

    I don't ever see an error report.

    gcc --version
    gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3

    For example, if I use "-bkgd 1 2 QQQ" I get a bKGD chunk
    with values 0001 0002 0000 and no error report. If I use
    "-bkgd 1 2 3QQQ" I get a bKGD chunk with values 0001 0002 0003.

    It didn't help to move "errno = 0" to the top of the
    for (i = 1; i < argc; i++) loop.

     
  • Zack Weinberg
    Zack Weinberg
    2013-09-27

    Testing for errors from strtol is a little involved, yah. It only sets errno for numeric overflow; it doesn't consider junk after the number to be a hard error, because you might be using it to extract numbers from a longer string. For this use case, you need to check that endptr has been advanced to the end of the argument:

    char *endptr;
    errno = 0;
    long x = strtol(argv[i], &endptr, 10);
    if (errno || endptr == argv[i] || *endptr != '\0') {
        fprintf(stderr, "malformed or missing argument '%s'\n",
                argv[i]);
        exit(1);
    }
    

    Anyhow, the BUMP_I fix is much more important. Thanks for the quick reaction.

     
    Last edit: Zack Weinberg 2013-09-27
  • Thanks, that works for me. It'll be in pngcrush-1.7.68 along with
    the BUMP_I fix.

     
    • assigned_to: Glenn Randers-Pehrson
    • Group: -->
     
  • Please test pngcrush-1.7.68 which is now in the GIT repository.

     
  • Zack Weinberg
    Zack Weinberg
    2013-09-29

    I get a compilation failure:

    pngcrush.c: In function main:
    pngcrush.c:3346:52: error: called object is not a function or function pointer
                     specified_gamma = pngcrush_get_long(number);
                                                        ^
    pngcrush.c:3564:58: error: called object is not a function or function pointer
                     force_specified_gamma = pngcrush_get_long(number);
    

    It appears that pngcrush_get_long is not supposed to be invoked with arguments (and thus can only be applied to argv[i]). I changed both instances to strtol(number,&endptr,10) and was able to compile. I haven't exhaustively tested the options, but a few spot-checks all work for me.

     
  • Thanks; I expanded the two instances of pngcrush_get_long(number) to strtol() calls and pushed to the GIT repository.

     
    • status: open --> closed
     
  • Fixed in pngcrush-1.7.69