Project

General

Profile

Bug #900

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

Added by Rich Lowe over 8 years ago. Updated over 8 years ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
cmd - userland programs
Start date:
2011-04-12
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

% 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);
                        break;

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.).

History

#1

Updated by Yuri Pankov over 8 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);
                        break;

working correctly in this case.

#2

Updated by Yuri Pankov over 8 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?

#3

Updated by Rich Lowe over 8 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 :)

#4

Updated by Yuri Pankov over 8 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.

#5

Updated by Gary Mills over 8 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.

#6

Updated by Gordon Ross over 8 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");

#7

Updated by Rich Lowe over 8 years ago

Resolved in r13358 commit:f7ba8ec46a21

#8

Updated by Rich Lowe over 8 years ago

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

Also available in: Atom PDF