Hi, Nathan.
On 11 May 2014 15:04, Nathan Hintz <nlhi...@hotmail.com> wrote:
> Hi Yousong:
>
>>
>>> ++ }
>>> ++ } else if (opt->flags & OPT_A2LIST) {
>>> + struct option_value *ovp, *pp;
>>> +
>>> + ovp = malloc(sizeof(*ovp) + strlen(*argv));
>>> +@@ -994,7 +1003,7 @@ print_option(opt, mainopt, printer, arg)
>>> + p = (char *) opt->addr2;
>>> + if ((opt->flags & OPT_STATIC) == 0)
>>> + p = *(char **)p;
>>> +- printer("%q", p);
>>> ++ printer(arg, "%q", p);
>>> + } else if (opt->flags & OPT_A2LIST) {
>>> + struct option_value *ovp;
>>> +
>>> --
>>> 1.9.0
>>> _______________________________________________
>>> openwrt-devel mailing list
>>> openwrt-devel@lists.openwrt.org
>>> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>
> ----------------------------------------
>> From: yszhou4t...@gmail.com
>> Date: Wed, 7 May 2014 14:40:20 +0800
>> Subject: Re: [OpenWrt-Devel] [PATCH 2/2] ppp: fix o_special option printing
>> To: nlhi...@hotmail.com
>> CC: openwrt-devel@lists.openwrt.org
>>
>> Hi, Nathan.
>>
>> On 7 May 2014 13:25, Nathan Hintz <nlhi...@hotmail.com> wrote:
>>>
>>> PPPD crashes (SEGV) when the 'dump' or 'dryrun' options are specified and
>>> an option defined internally as "o_special" with an option flag of
>>> "OPT_A2STRVAL" is used. The crash occurs because the option value is not
>>> saved when the parameter is processed, but is then referenced when printed.
>>> Additionally, the "arg" parameter is missing from the call to the "printer"
>>> utility. This was encountered using xl2tpd and the l2tp_ppp kernel module;
>>> the option PPPD crashes on is "pppol2tp 8".
>>
>> I think the handling of OPT_A2STRVAL should be done in pppol2tp.c
>> option handling function. Take `domain' option in options.c and
>> `callback' option in cbcp.c as examples, the value of addr2 are set by
>> handler of each.
>>
>
> I got a chance to look at this this weekend, and I agree with your comment and
> will update the patch. I was contemplating a null pointer check in the
> "print_option"
> function in options.c to eliminate the possibility of crashing (if "*(char
> **)addr2" was
> NULL); but that might not be necessary anymore. Thoughts?
I checked several options with the OPT_A2STRVAL attribute and w/o
OPT_STATIC. Looks like they all novm(errmsg) if pppd failed
allocating the needed memory. I tend to think the current
implementation is fine although a few lines of comment there will be
better. :)
Regards.
yousong
>
>>>
>>> Modify pppd to correctly save the option value, and to call the printer
>>> utility with the correct parameters.
>>>
>>> Signed-off-by: Nathan Hintz <nlhi...@hotmail.com>
>>> ---
>>>
>>> Supersedes this patch: http://patchwork.openwrt.org/patch/3245/
>>>
>>> package/network/services/ppp/Makefile | 2 +-
>>> .../501-fix-o_special-option-printing.patch | 29 ++++++++++++++++++++++
>>> 2 files changed, 30 insertions(+), 1 deletion(-)
>>> create mode 100644
>>> package/network/services/ppp/patches/501-fix-o_special-option-printing.patch
>>>
>>> diff --git a/package/network/services/ppp/Makefile
>>> b/package/network/services/ppp/Makefile
>>> index 9bf9616..a707985 100644
>>> --- a/package/network/services/ppp/Makefile
>>> +++ b/package/network/services/ppp/Makefile
>>> @@ -10,7 +10,7 @@ include $(INCLUDE_DIR)/kernel.mk
>>>
>>> PKG_NAME:=ppp
>>> PKG_VERSION:=2.4.5
>>> -PKG_RELEASE:=10
>>> +PKG_RELEASE:=11
>>>
>>> PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
>>> PKG_SOURCE_URL:=ftp://ftp.samba.org/pub/ppp/
>>> diff --git
>>> a/package/network/services/ppp/patches/501-fix-o_special-option-printing.patch
>>>
>>> b/package/network/services/ppp/patches/501-fix-o_special-option-printing.patch
>>> new file mode 100644
>>> index 0000000..99b933f
>>> --- /dev/null
>>> +++
>>> b/package/network/services/ppp/patches/501-fix-o_special-option-printing.patch
>>> @@ -0,0 +1,29 @@
>>> +--- a/pppd/options.c
>>> ++++ b/pppd/options.c
>>> +@@ -809,7 +809,16 @@ process_option(opt, cmd, argv)
>>> + parser = (int (*) __P((char **))) opt->addr;
>>> + if (!(*parser)(argv))
>>> + return 0;
>>> +- if (opt->flags & OPT_A2LIST) {
>>> ++ if (opt->flags & OPT_A2STRVAL) {
>>> ++ if (opt->flags & OPT_STATIC) {
>>> ++ strlcpy((char *)(opt->addr2), *argv, opt->upper_limit);
>>> ++ } else {
>>> ++ sv = strdup(*argv);
>>> ++ if (sv == NULL)
>>> ++ novm("option argument");
>>> ++ *(char **)(opt->addr2) = sv;
>>
>> This reminds me of memory leak. :)
>>
>
> This particular instance will be gone due to incorporation of your previous
> comment
>
>>
>> Regards.
>>
>> yousong
>>
>
> Thanks for your input!
>
> Nathan
>
>>> ++ }
>>> ++ } else if (opt->flags & OPT_A2LIST) {
>>> + struct option_value *ovp, *pp;
>>> +
>>> + ovp = malloc(sizeof(*ovp) + strlen(*argv));
>>> +@@ -994,7 +1003,7 @@ print_option(opt, mainopt, printer, arg)
>>> + p = (char *) opt->addr2;
>>> + if ((opt->flags & OPT_STATIC) == 0)
>>> + p = *(char **)p;
>>> +- printer("%q", p);
>>> ++ printer(arg, "%q", p);
>>> + } else if (opt->flags & OPT_A2LIST) {
>>> + struct option_value *ovp;
>>> +
>>> --
>>> 1.9.0
>>> _______________________________________________
>>> openwrt-devel mailing list
>>> openwrt-devel@lists.openwrt.org
>>> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
