RFC: Potential invalid pointer / release after free fix in HtmlOutputDev

[Reece Dunn-2]

NOTE: I haven't built this, as I cannot build at the moment (missing
fontconfig dependency in Ubuntu that I haven't figured out).

The original code is something like:

   GooString *str, *str1 = NULL;
   for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
    if (tmp->htext){
       str=new GooString(tmp->htext);
       fprintf(f,"<text top=\"%d\" left=\"%d\"
",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
       fprintf(f,"width=\"%d\" height=\"%d\"
",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
       fprintf(f,"font=\"%d\">", tmp->fontpos);
      if (tmp->fontpos!=-1){
        str1=fonts->getCSStyle(tmp->fontpos, str);
      }
       fputs(str1->getCString(),f);
      delete str;
       delete str1;
       fputs("</text>\n",f);
     }

As can be seen, str1 is not initialised outside of the "if
(tmp->fontpos!=-1)" condition. This means that if tmp->fontpos == -1,
str1 is either NULL (if this is the first entry, resulting in a NULL
pointer dereference), or is pointing to the previous str1 object
already deleted (resulting in a delete-after-free), and before it a
NULL pointer access, or access to already released memory (with
potentially garbage data).

Thus, the code should be:

   GooString *str, *str1 = NULL;
   for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
    if (tmp->htext){
       str=new GooString(tmp->htext);
       fprintf(f,"<text top=\"%d\" left=\"%d\"
",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
       fprintf(f,"width=\"%d\" height=\"%d\"
",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
       fprintf(f,"font=\"%d\">", tmp->fontpos);
      if (tmp->fontpos!=-1){
        str1=fonts->getCSStyle(tmp->fontpos, str);
       fputs(str1->getCString(),f);
       delete str1;
      }
      delete str;
       fputs("</text>\n",f);
     }

with str1 only being used and released inside that conditional.

Now, if tmp->fontpos == -1 the output is "<text></text>" as there is
no text being written. This does not make sense, so the
(tmp->fontpos!=-1) check can be moved to the (tmp->htext) check.

Thus my patch, which as I said is untested (hence the RFC).

NOTE: The "simple" html case is not outputting surrounding <text> or
<div> tags, so does not have this issue; it is only the "complex"
cases.

- Reece

[invalid-pointer-delete-fix.diff]

---

diff --git a/utils/HtmlOutputDev.cc b/utils/HtmlOutputDev.cc
index 64f5098..38ae103 100644
--- a/utils/HtmlOutputDev.cc
+++ b/utils/HtmlOutputDev.cc
@@ -640,17 +640,15 @@ void HtmlPage::dumpAsXML(FILE* f,int page){
   
   GooString *str, *str1 = NULL;
   for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
-    if (tmp->htext){
+    if (tmp->htext && tmp->fontpos!=-1){
       str=new GooString(tmp->htext);
       fprintf(f,"<text top=\"%d\" left=\"%d\" ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
       fprintf(f,"width=\"%d\" height=\"%d\" ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
       fprintf(f,"font=\"%d\">", tmp->fontpos);
-      if (tmp->fontpos!=-1){
- str1=fonts->getCSStyle(tmp->fontpos, str);
-      }
+      str1=fonts->getCSStyle(tmp->fontpos, str);
       fputs(str1->getCString(),f);
-      delete str;
       delete str1;
+      delete str;
       fputs("</text>\n",f);
     }
   }
@@ -724,21 +722,17 @@ void HtmlPage::dumpComplex(FILE *file, int page){
   
   GooString *str, *str1 = NULL;
   for(HtmlString *tmp1=yxStrings;tmp1;tmp1=tmp1->yxNext){
-    if (tmp1->htext){
+    if (tmp1->htext && tmp1->fontpos!=-1){
       str=new GooString(tmp1->htext);
       fprintf(pageFile,
       "<DIV style=\"position:absolute;top:%d;left:%d\">",
       xoutRound(tmp1->yMin),
       xoutRound(tmp1->xMin));
       fputs("<nobr>",pageFile);
-      if (tmp1->fontpos!=-1){
- str1=fonts->getCSStyle(tmp1->fontpos, str);  
-      }
-      //printf("%s\n", str1->getCString());
+      str1=fonts->getCSStyle(tmp1->fontpos, str);  
       fputs(str1->getCString(),pageFile);
-      
-      delete str;      
       delete str1;
+      delete str;      
       fputs("</nobr></DIV>\n",pageFile);
     }
   }

---

_______________________________________________
poppler mailing list
poppler@...
http://lists.freedesktop.org/mailman/listinfo/poppler

[Albert Astals Cid-2]

A Dimecres, 4 de novembre de 2009, Reece Dunn va escriure:
> NOTE: I haven't built this, as I cannot build at the moment (missing
> fontconfig dependency in Ubuntu that I haven't figured out).

sudo apt-get install libfontconfig1-dev
?

>
> The original code is something like:
>
>    GooString *str, *str1 = NULL;
>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>     if (tmp->htext){
>        str=new GooString(tmp->htext);
>        fprintf(f,"<text top=\"%d\" left=\"%d\"
> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>        fprintf(f,"width=\"%d\" height=\"%d\"
> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>       if (tmp->fontpos!=-1){
> str1=fonts->getCSStyle(tmp->fontpos, str);
>       }
>        fputs(str1->getCString(),f);
>       delete str;
>        delete str1;
>        fputs("</text>\n",f);
>      }
>
> As can be seen, str1 is not initialised outside of the "if
> (tmp->fontpos!=-1)" condition. This means that if tmp->fontpos == -1,
> str1 is either NULL (if this is the first entry, resulting in a NULL
> pointer dereference), or is pointing to the previous str1 object
> already deleted (resulting in a delete-after-free), and before it a
> NULL pointer access, or access to already released memory (with
> potentially garbage data).
>
> Thus, the code should be:
>
>    GooString *str, *str1 = NULL;
>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>     if (tmp->htext){
>        str=new GooString(tmp->htext);
>        fprintf(f,"<text top=\"%d\" left=\"%d\"
> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>        fprintf(f,"width=\"%d\" height=\"%d\"
> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>       if (tmp->fontpos!=-1){
> str1=fonts->getCSStyle(tmp->fontpos, str);
>        fputs(str1->getCString(),f);
>        delete str1;
>       }
>       delete str;
>        fputs("</text>\n",f);
>      }
>
> with str1 only being used and released inside that conditional.
>
> Now, if tmp->fontpos == -1 the output is "<text></text>" as there is
> no text being written. This does not make sense, so the
> (tmp->fontpos!=-1) check can be moved to the (tmp->htext) check.
>
> Thus my patch, which as I said is untested (hence the RFC).

Yeah seems logical, but the more important, do you have a case where this
happens?

Albert

>
> NOTE: The "simple" html case is not outputting surrounding <text> or
> <div> tags, so does not have this issue; it is only the "complex"
> cases.
>
> - Reece
>

_______________________________________________
poppler mailing list
poppler@...
http://lists.freedesktop.org/mailman/listinfo/poppler

[Reece Dunn-2]

2009/11/4 Albert Astals Cid <aacid@...>:
> A Dimecres, 4 de novembre de 2009, Reece Dunn va escriure:
>> NOTE: I haven't built this, as I cannot build at the moment (missing
>> fontconfig dependency in Ubuntu that I haven't figured out).
>
> sudo apt-get install libfontconfig1-dev
> ?

That works, thanks.

>> The original code is something like:
>>
>>    GooString *str, *str1 = NULL;
>>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>>     if (tmp->htext){
>>        str=new GooString(tmp->htext);
>>        fprintf(f,"<text top=\"%d\" left=\"%d\"
>> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>>        fprintf(f,"width=\"%d\" height=\"%d\"
>> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>>       if (tmp->fontpos!=-1){
>>       str1=fonts->getCSStyle(tmp->fontpos, str);
>>       }
>>        fputs(str1->getCString(),f);
>>       delete str;
>>        delete str1;
>>        fputs("</text>\n",f);
>>      }
>>
>> As can be seen, str1 is not initialised outside of the "if
>> (tmp->fontpos!=-1)" condition. This means that if tmp->fontpos == -1,
>> str1 is either NULL (if this is the first entry, resulting in a NULL
>> pointer dereference), or is pointing to the previous str1 object
>> already deleted (resulting in a delete-after-free), and before it a
>> NULL pointer access, or access to already released memory (with
>> potentially garbage data).
>>
>> Thus, the code should be:
>>
>>    GooString *str, *str1 = NULL;
>>    for(HtmlString *tmp=yxStrings;tmp;tmp=tmp->yxNext){
>>     if (tmp->htext){
>>        str=new GooString(tmp->htext);
>>        fprintf(f,"<text top=\"%d\" left=\"%d\"
>> ",xoutRound(tmp->yMin),xoutRound(tmp->xMin));
>>        fprintf(f,"width=\"%d\" height=\"%d\"
>> ",xoutRound(tmp->xMax-tmp->xMin),xoutRound(tmp->yMax-tmp->yMin));
>>        fprintf(f,"font=\"%d\">", tmp->fontpos);
>>       if (tmp->fontpos!=-1){
>>       str1=fonts->getCSStyle(tmp->fontpos, str);
>>        fputs(str1->getCString(),f);
>>        delete str1;
>>       }
>>       delete str;
>>        fputs("</text>\n",f);
>>      }
>>
>> with str1 only being used and released inside that conditional.
>>
>> Now, if tmp->fontpos == -1 the output is "<text></text>" as there is
>> no text being written. This does not make sense, so the
>> (tmp->fontpos!=-1) check can be moved to the (tmp->htext) check.
>>
>> Thus my patch, which as I said is untested (hence the RFC).
>
> Yeah seems logical, but the more important, do you have a case where this
> happens?

No, I don't have a case where this happens. It is something I saw when
looking at the code.

That is, I don't know when tmp->fontpos == -1 (for any tmp in
xyStrings) without further analysis.

- Reece
_______________________________________________
poppler mailing list
poppler@...
http://lists.freedesktop.org/mailman/listinfo/poppler