Fix the problem of potential memory leakage. - #409

@hwoarang need a maintainer to approve running workflows

@zx2c4 @rossburton @sfionov review please, thanks.

@zx2c4 @rossburton @hwoarang @sfionov need review, thanks.

@zx2c4 @rossburton @hwoarang @sfionov need review, thanks.

I'm not a maintainer, so I can't approve workflows.

However, that patch changes behaviour: if I pass -p twice then the second value is ignored. Conventionally, the second value will replace the first.

I'm not a maintainer, so I can't approve workflows.

However, that patch changes behaviour: if I pass -p twice then the second value is ignored. Conventionally, the second value will replace the first.

Yes. you are right. I will fixed this problem.

@zx2c4 @hwoarang @sfionov

@ggvl FYI not approved workflow is not blocking this, lack of my time block it. iputils have practically no functional tests, thus I need to look carefully into the code (there have been many regressions in the past in ping).
BTW I wonder why you don't have allowed GitHub Actions in your fork: https://github.com/ggvl/iputils/actions

However, that patch changes behaviour: if I pass -p twice then the second value is ignored. Conventionally, the second value will replace the first.

@rossburton IMHO last value is always used (in master, and after this change). Or do I miss something?
FYI ping from Busybox behaves the same.

@ggvl LGTM, but let me have second look later.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

@pevik

@ggvl Thinking it twice Could we merge just fixing the leak first:

--- ping/ping.c
+++ ping/ping.c
@@ -449,6 +449,7 @@ main(int argc, char **argv)
                        break;
                case 'p':
                        rts.opt_pingfilled = 1;
+                       free(outpack_fill);
                        outpack_fill = strdup(optarg);
                        if (!outpack_fill)
                                error(2, errno, _("memory allocation failed"));

1. check for max 16 bytes is a separate issue, which should be in a separate commit (you haven't even mention it in the commit message, but separate commit would be better).
2. I'm not sure if strlen(outpack_fill) > 16 is a correct check. Because pattern is in hex, busybox implementation simply checks if number is in 0..255 range.

@pevik Ok. Thanks for you advice.

1. check for max 16 bytes is a separate issue, which should be in a separate commit (you haven't even mention it in the commit message, but separate commit would be better).

Fine. I will separate it.

2. I'm not sure if strlen(outpack_fill) > 16 is a correct check. Because pattern is in hex, busybox implementation simply checks if number is in 0..255 range.

I read the fill() function and find it use isxdigit to check each byte in the readbuf, so the maxlen should be 32.

@pevik This on is ok and I will commit the second patch later.

I read the fill() function and find it use isxdigit to check each byte in the readbuf, so the maxlen should be 32.

Agree, but it'd be worth to check why busybox implementation allows only 0..255 decimal value.
#412