Syslinux - Sep 2015 - [PATCH] chrreplace: Don't skip the first character

Erwan, could you shed some light on this?  Was the behavior
intentional?  If not, I think it might prove more useful down the road
to commit this patch.  Original code in commit ID 85d9a1a

-- 
-Gene

On Sat, May 16, 2015 at 3:30 AM, Josh Triplett via Syslinux
<syslinux at zytor.com> wrote:
> Check if the first character matches the character to replace, rather
> than skipping it and starting with the second.
>
> Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> ---
>
> I'm assuming, based on a look at the callers, that this is not
> intentional, and that it just happened that none of the callers happened
> to ever need to replace the first character.
>
>  com32/lib/chrreplace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> index 65786f9..cfbf5d4 100644
> --- a/com32/lib/chrreplace.c
> +++ b/com32/lib/chrreplace.c
> @@ -4,8 +4,8 @@
>  void chrreplace(char *source, char old, char new)
>  {
>      while (*source) {
> -       source++;
>         if (source[0] == old) source[0]=new;
> +       source++;
>      }
>  }
>

Just looking at the code and the description of it, for me the current code
looks wrong and the proposed patch correct for the following reason:

The while loop is made for insuring that *source is valid but just after
shift the pointer by one.
This new position isn't tested and so we are addressing an unknown area.
That could corrupt some memory.

But also as reported, that prevent from changing the first char.

I've been contributing that code and sounds like only the code I produced
is using it (hdt & the pci scan).
So I'd vote for including that patch. If any regression occur, I'd look
at
it.

Thanks for reporting, +1 for the merge.

2015-09-22 12:45 GMT+02:00 Gene Cumm <gene.cumm at gmail.com>:
> Erwan, could you shed some light on this?  Was the behavior
> intentional?  If not, I think it might prove more useful down the road
> to commit this patch.  Original code in commit ID 85d9a1a
>
> --
> -Gene
>
> On Sat, May 16, 2015 at 3:30 AM, Josh Triplett via Syslinux
> <syslinux at zytor.com> wrote:
> > Check if the first character matches the character to replace, rather
> > than skipping it and starting with the second.
> >
> > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> > ---
> >
> > I'm assuming, based on a look at the callers, that this is not
> > intentional, and that it just happened that none of the callers
happened
> > to ever need to replace the first character.
> >
> >  com32/lib/chrreplace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> > index 65786f9..cfbf5d4 100644
> > --- a/com32/lib/chrreplace.c
> > +++ b/com32/lib/chrreplace.c
> > @@ -4,8 +4,8 @@
> >  void chrreplace(char *source, char old, char new)
> >  {
> >      while (*source) {
> > -       source++;
> >         if (source[0] == old) source[0]=new;
> > +       source++;
> >      }
> >  }
> >
>

On Mon, Sep 28, 2015 at 4:29 AM, Erwan Velu <erwanaliasr1 at gmail.com>
wrote:
> Just looking at the code and the description of it, for me the current code
> looks wrong and the proposed patch correct for the following reason:
>
> The while loop is made for insuring that *source is valid but just after
> shift the pointer by one.
> This new position isn't tested and so we are addressing an unknown
area.
> That could corrupt some memory.
>
> But also as reported, that prevent from changing the first char.
>
> I've been contributing that code and sounds like only the code I
produced is
> using it (hdt & the pci scan).
> So I'd vote for including that patch. If any regression occur, I'd
look at
> it.
>
> Thanks for reporting, +1 for the merge.
Thanks.  Commit ID 779a4c8

-- 
-Gene
> 2015-09-22 12:45 GMT+02:00 Gene Cumm <gene.cumm at gmail.com>:
>>
>> Erwan, could you shed some light on this?  Was the behavior
>> intentional?  If not, I think it might prove more useful down the road
>> to commit this patch.  Original code in commit ID 85d9a1a
>>
>> --
>> -Gene
>>
>> On Sat, May 16, 2015 at 3:30 AM, Josh Triplett via Syslinux
>> <syslinux at zytor.com> wrote:
>> > Check if the first character matches the character to replace,
rather
>> > than skipping it and starting with the second.
>> >
>> > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
>> > ---
>> >
>> > I'm assuming, based on a look at the callers, that this is not
>> > intentional, and that it just happened that none of the callers
happened
>> > to ever need to replace the first character.
>> >
>> >  com32/lib/chrreplace.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
>> > index 65786f9..cfbf5d4 100644
>> > --- a/com32/lib/chrreplace.c
>> > +++ b/com32/lib/chrreplace.c
>> > @@ -4,8 +4,8 @@
>> >  void chrreplace(char *source, char old, char new)
>> >  {
>> >      while (*source) {
>> > -       source++;
>> >         if (source[0] == old) source[0]=new;
>> > +       source++;
>> >      }
>> >  }

On Mon, Sep 28, 2015 at 10:29:16AM +0200, Erwan Velu
wrote:
> Just looking at the code and the description of it, for me the current code
> looks wrong and the proposed patch correct for the following reason:
> 
> The while loop is made for insuring that *source is valid but just after
> shift the pointer by one.
> This new position isn't tested and so we are addressing an unknown
area.
> That could corrupt some memory.
Fortunately, as long as the string terminates with a '\0' (and someone
doesn't pass '\0' as "old"), this can't actually
overrun.
> But also as reported, that prevent from changing the first char.
> 
> I've been contributing that code and sounds like only the code I
produced
> is using it (hdt & the pci scan).
> So I'd vote for including that patch. If any regression occur, I'd
look at
> it.
> 
> Thanks for reporting, +1 for the merge.
Thanks.

- Josh Triplett
> 2015-09-22 12:45 GMT+02:00 Gene Cumm <gene.cumm at gmail.com>:
> 
> > Erwan, could you shed some light on this?  Was the behavior
> > intentional?  If not, I think it might prove more useful down the road
> > to commit this patch.  Original code in commit ID 85d9a1a
> >
> > --
> > -Gene
> >
> > On Sat, May 16, 2015 at 3:30 AM, Josh Triplett via Syslinux
> > <syslinux at zytor.com> wrote:
> > > Check if the first character matches the character to replace,
rather
> > > than skipping it and starting with the second.
> > >
> > > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> > > ---
> > >
> > > I'm assuming, based on a look at the callers, that this is
not
> > > intentional, and that it just happened that none of the callers
happened
> > > to ever need to replace the first character.
> > >
> > >  com32/lib/chrreplace.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> > > index 65786f9..cfbf5d4 100644
> > > --- a/com32/lib/chrreplace.c
> > > +++ b/com32/lib/chrreplace.c
> > > @@ -4,8 +4,8 @@
> > >  void chrreplace(char *source, char old, char new)
> > >  {
> > >      while (*source) {
> > > -       source++;
> > >         if (source[0] == old) source[0]=new;
> > > +       source++;
> > >      }
> > >  }
> > >
> >