Re: [GNC-dev] gnucash maint: Bug 796982 - Import Bills & Invoices: change in un_escape() routine...

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Bug 796982 - Import Bills & Invoices: change in un_escape() routine...

Geert Janssens-4
Op woensdag 19 december 2018 06:32:16 CET schreef John Ralls:

> Updated via  https://github.com/Gnucash/gnucash/commit/f2976420 (commit)
> from  https://github.com/Gnucash/gnucash/commit/2524482b (commit)
>
>
>
> commit f29764202ec9f7ace6eb726c246799cb19ad5c1a
> Author: John Ralls <[hidden email]>
> Date:   Tue Dec 18 21:29:53 2018 -0800
>
>     Bug 796982 - Import Bills & Invoices: change in un_escape() routine...
>
>     causes description and notes fields to be mangled.
>
>     Simple error, but rewrote the function to be more idiomatic, resisting
>     temptation to abuse the ternary operator.
>
> diff --git a/gnucash/import-export/bi-import/dialog-bi-import.c
> b/gnucash/import-export/bi-import/dialog-bi-import.c index 5b46e84..6aabcc3
> 100644
> --- a/gnucash/import-export/bi-import/dialog-bi-import.c
> +++ b/gnucash/import-export/bi-import/dialog-bi-import.c
> @@ -876,30 +876,26 @@ gnc_bi_import_create_bis (GtkListStore * store,
> QofBook * book, * @param char* String to be modified
>   * @return char* Modified string.
>  */
> -static char * un_escape(char *str)
> +static char*
> +un_escape(char *str)
>  {
>      gchar quote = '"';
>      gchar *newStr = NULL, *tmpstr = str;
>      int n = 0;
> +
>      newStr = g_malloc(strlen(str)+1); // This must be freed in the calling
> code. while(*tmpstr != '\0')
>      {
>          if(*tmpstr == quote)
> -        {
> -            tmpstr++;
> -            if(*tmpstr == quote)
> -            {
> -                newStr[n] = quote;
> -            }
> -        }
> +            // We always want the character after a quote.
> +            newStr[n] = *(++tmpstr);
>          else
> -        {
> -            newStr[n] = *str;
> -        }
> -            tmpstr++;
> -            n++;
> +            newStr[n] = *tmpstr;
> +        ++tmpstr;
> +        ++n;
>      }
> +
>      g_free (str);
> - newStr[n] = '\0'; //ending the character array
> +    newStr[n] = '\0'; //ending the character array
>      return newStr;
>  }

Won't this introduce a read-past-end-of-string in case the last character of
the string is a quote ?

As far as I understand that would make
newStr[n] = '\0'
and tmpstr will be increased twice before it's reevaluated, effectively moving
it past the final '\0':
Once in
newStr[n] = *(++tmpstr);
and a second time in
++tmpstr;

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Bug 796982 - Import Bills & Invoices: change in un_escape() routine...

John Ralls-2


> On Dec 19, 2018, at 2:04 AM, Geert Janssens <[hidden email]> wrote:
>
> Op woensdag 19 december 2018 06:32:16 CET schreef John Ralls:
>> Updated via  https://github.com/Gnucash/gnucash/commit/f2976420 (commit)
>> from  https://github.com/Gnucash/gnucash/commit/2524482b (commit)
>>
>>
>>
>> commit f29764202ec9f7ace6eb726c246799cb19ad5c1a
>> Author: John Ralls <[hidden email]>
>> Date:   Tue Dec 18 21:29:53 2018 -0800
>>
>>    Bug 796982 - Import Bills & Invoices: change in un_escape() routine...
>>
>>    causes description and notes fields to be mangled.
>>
>>    Simple error, but rewrote the function to be more idiomatic, resisting
>>    temptation to abuse the ternary operator.
>>
>> diff --git a/gnucash/import-export/bi-import/dialog-bi-import.c
>> b/gnucash/import-export/bi-import/dialog-bi-import.c index 5b46e84..6aabcc3
>> 100644
>> --- a/gnucash/import-export/bi-import/dialog-bi-import.c
>> +++ b/gnucash/import-export/bi-import/dialog-bi-import.c
>> @@ -876,30 +876,26 @@ gnc_bi_import_create_bis (GtkListStore * store,
>> QofBook * book, * @param char* String to be modified
>>  * @return char* Modified string.
>> */
>> -static char * un_escape(char *str)
>> +static char*
>> +un_escape(char *str)
>> {
>>     gchar quote = '"';
>>     gchar *newStr = NULL, *tmpstr = str;
>>     int n = 0;
>> +
>>     newStr = g_malloc(strlen(str)+1); // This must be freed in the calling
>> code. while(*tmpstr != '\0')
>>     {
>>         if(*tmpstr == quote)
>> -        {
>> -            tmpstr++;
>> -            if(*tmpstr == quote)
>> -            {
>> -                newStr[n] = quote;
>> -            }
>> -        }
>> +            // We always want the character after a quote.
>> +            newStr[n] = *(++tmpstr);
>>         else
>> -        {
>> -            newStr[n] = *str;
>> -        }
>> -            tmpstr++;
>> -            n++;
>> +            newStr[n] = *tmpstr;
>> +        ++tmpstr;
>> +        ++n;
>>     }
>> +
>>     g_free (str);
>> - newStr[n] = '\0'; //ending the character array
>> +    newStr[n] = '\0'; //ending the character array
>>     return newStr;
>> }
>
> Won't this introduce a read-past-end-of-string in case the last character of
> the string is a quote ?
>
> As far as I understand that would make
> newStr[n] = '\0'
> and tmpstr will be increased twice before it's reevaluated, effectively moving
> it past the final '\0':
> Once in
> newStr[n] = *(++tmpstr);
> and a second time in
> ++tmpstr;

Good point, though it’s also true of the original. I considered a for-loop instead, I’ll revisit that as less ugly than checking tmpstr twice.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Bug 796982 - Import Bills & Invoices: change in un_escape() routine...

Geert Janssens-4
Op woensdag 19 december 2018 15:55:17 CET schreef John Ralls:
> > On Dec 19, 2018, at 2:04 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op woensdag 19 december 2018 06:32:16 CET schreef John Ralls:
> >> Updated via  https://github.com/Gnucash/gnucash/commit/f2976420 
(commit)

> >>
> >> from  https://github.com/Gnucash/gnucash/commit/2524482b (commit)
> >>
> >> commit f29764202ec9f7ace6eb726c246799cb19ad5c1a
> >> Author: John Ralls <[hidden email]>
> >> Date:   Tue Dec 18 21:29:53 2018 -0800
> >>
> >>    Bug 796982 - Import Bills & Invoices: change in un_escape() routine...
> >>    
> >>    causes description and notes fields to be mangled.
> >>    
> >>    Simple error, but rewrote the function to be more idiomatic, resisting
> >>    temptation to abuse the ternary operator.
> >>
> >> diff --git a/gnucash/import-export/bi-import/dialog-bi-import.c
> >> b/gnucash/import-export/bi-import/dialog-bi-import.c index
> >> 5b46e84..6aabcc3
> >> 100644
> >> --- a/gnucash/import-export/bi-import/dialog-bi-import.c
> >> +++ b/gnucash/import-export/bi-import/dialog-bi-import.c
> >> @@ -876,30 +876,26 @@ gnc_bi_import_create_bis (GtkListStore * store,
> >> QofBook * book, * @param char* String to be modified
> >>
> >>  * @return char* Modified string.
> >>
> >> */
> >> -static char * un_escape(char *str)
> >> +static char*
> >> +un_escape(char *str)
> >> {
> >>
> >>     gchar quote = '"';
> >>     gchar *newStr = NULL, *tmpstr = str;
> >>     int n = 0;
> >>
> >> +
> >>
> >>     newStr = g_malloc(strlen(str)+1); // This must be freed in the
> >>     calling
> >>
> >> code. while(*tmpstr != '\0')
> >>
> >>     {
> >>    
> >>         if(*tmpstr == quote)
> >>
> >> -        {
> >> -            tmpstr++;
> >> -            if(*tmpstr == quote)
> >> -            {
> >> -                newStr[n] = quote;
> >> -            }
> >> -        }
> >> +            // We always want the character after a quote.
> >> +            newStr[n] = *(++tmpstr);
> >>
> >>         else
> >>
> >> -        {
> >> -            newStr[n] = *str;
> >> -        }
> >> -            tmpstr++;
> >> -            n++;
> >> +            newStr[n] = *tmpstr;
> >> +        ++tmpstr;
> >> +        ++n;
> >>
> >>     }
> >>
> >> +
> >>
> >>     g_free (str);
> >>
> >> - newStr[n] = '\0'; //ending the character array
> >> +    newStr[n] = '\0'; //ending the character array
> >>
> >>     return newStr;
> >>
> >> }
> >
> > Won't this introduce a read-past-end-of-string in case the last character
> > of the string is a quote ?
> >
> > As far as I understand that would make
> > newStr[n] = '\0'
> > and tmpstr will be increased twice before it's reevaluated, effectively
> > moving it past the final '\0':
> > Once in
> > newStr[n] = *(++tmpstr);
> > and a second time in
> > ++tmpstr;
>
> Good point, though it’s also true of the original.
Ah, indeed, I see that too now.

> I considered a for-loop
> instead, I’ll revisit that as less ugly than checking tmpstr twice.

Ok.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel