Bug #900


sed errors when -e is given a 0-length argument

Added by Rich Lowe about 11 years ago. Updated about 11 years ago.

cmd - userland programs
Start date:
Due date:
% Done:


Estimated time:
Gerrit CR:


% echo "x" | /bin/sed -e ''
sed: asprintf: Error 0

                case 'e':
                        eflag = 1;
                        if (asprintf(&temp_arg, "%s\n", optarg) <= 1)
                                err(1, "asprintf");
                        add_compunit(CU_STRING, temp_arg);

This is wrong. It's entirely reasonable to end up passing a 0-length command to sed (I hit this during a build of gcc, which this bug causes to fail). sed should check the length and avoid calling add_compunit if it is 0 (probably even strlen(optarg), and skip the asprintf, too.).

Actions #1

Updated by Yuri Pankov about 11 years ago

Porting issue, the original code has:

                case 'e':
                        eflag = 1;
                        if ((temp_arg = malloc(strlen(optarg) + 2)) == NULL)
                                err(1, "malloc");
                        strcpy(temp_arg, optarg);
                        strcat(temp_arg, "\n");
                        add_compunit(CU_STRING, temp_arg);

working correctly in this case.

Actions #2

Updated by Yuri Pankov about 11 years ago

As there's no comments, the reason I brought up original code is that asprintf, while widely implemented, is still non-standard GNU extension and the change, given the problem, doesn't look nice. Can we use the original code instead?

Actions #3

Updated by Rich Lowe about 11 years ago

It would be easier to just change '<= 1' to '< 1'. (there should always be one character, the \n, if asprintf() really did not fail).
asprintf() also sets its first argument to NULL on failure, so the condition could be based on that, instead.

While asprintf is non-standard, it's too convenient to ignore out of hand for that reason, especially for systems software where we control the C library :)

Actions #4

Updated by Yuri Pankov about 11 years ago

But is introducing incompatibility worth it? As I see it, it'll just make harder to sync once bsd sed is got updated.

Actions #5

Updated by Gary Mills about 11 years ago

I have a fix for this one that eliminates the problem
under OI 148b. I've decided to revert to the freebsd
code for two reasons. `asprintf' is only used in that
one place in the sed code, leaving no reason to retain
it. As well, there's no reason to alter the freebsd code
in this region. `sed' performs nicely without the change.
Look for a webrev soon.

Actions #6

Updated by Gordon Ross about 11 years ago

The BSD code was changed to asprintf to avoid the separate
malloc and copy. In general, that's a good thing.

I suggest simply correcting the check of the return value, i.e.

        if (asprintf(&temp_arg, "%s\n", optarg) < 0)
            err(1, "asprintf");

Actions #7

Updated by Rich Lowe about 11 years ago

Resolved in r13358 commit:f7ba8ec46a21

Actions #8

Updated by Rich Lowe about 11 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100
  • Difficulty set to Medium

Also available in: Atom PDF