[poppler] Patch to Get/Set PDF ID in the trailer dictionary

[Pino Toscano]

Hi,

Alle mercoledì 16 giugno 2010, srinivas adicherla ha scritto:
> Index: poppler/poppler-poppler-document.html
> Index: poppler/poppler-poppler.html
> [...]
>            Generated by GTK-Doc V1.11</div>

The HTML files are autogenerated, so pretty unuseful. I'm not familiar 
with gtk-doc, so I cannot tell you what to do (Carlos?).

> +  // Return the PDF ID contains in the trailer dictionary 
> +  GBool getID(GooString *permanent_id,GooString *update_id);
> +  // Set the PDF ID in the trailer dictionary
> +  GBool   setID();

A bit more of clean indentation: add a space after a comma, and just 
once space instead of few. (This applies also for other parts.)

> +GBool PDFDoc::getID(GooString *permanent_id,GooString *update_id) {
> +
> +       Object *trailer = xref->getTrailerDict();

We don't use tabs in the poppler core, so please use the same 
indentation used in this file.

> +       char *Id1       = val1.getString()->getCString();
> +       char *Id2       = val2.getString()->getCString();
> +
> +       char **pdfids = (char**)malloc(2*sizeof(char*));
> +       pdfids[0] = (char*)malloc(32*sizeof(char));
> +       pdfids[1] = (char*)malloc(32*sizeof(char));
> +
> +       get_id(Id1,pdfids[0]);
> +       get_id(Id2,pdfids[1]);
> +
> +       permanent_id->append(pdfids[0],32);
> +       update_id->append(pdfids[1],32);
> +       
> +       free(Id1);
> +       free(Id2);

Still malloc() abuse...
  char tmpid[33];
  get_id(val1.getString()->getCString(), tmpid);
  permanent_id->append(tmpid, 32);
  get_id(val2.getString()->getCString(), tmpid);
  update_id->append(tmpid, 32);

Plus, you must free() the two Objects read.

> +GBool PDFDoc::setID() {
> +
> +       GooString permanent_id,update_id;
> +       
> +       if(getID(&permanent_id,&update_id)) { return false; }
> +       else {
> +               char *filename = getFileName()->getCString(); 

Still there is no handling of non-files documents, where this line with 
crash with a NULL pointer usage.

> +               // gets filename from the path 
> +               char *file = strrchr(filename,'/');
> +               if(!file) strrchr(filename,'\\');

Is this supposed to handle either Unix or Windows paths?
If so, see the SLASH define in utils/HtmlOutputDev.h for a way to just 
use the proper character with no need to lookup twice.

> +               // file name
> +               if(filename)
> +                       message.append(file+1);
> +               else
> +                       message.append("emptyfilename.pdf");

What is this supposed to do?
Also note strrchr() will return NULL is cannot find the character, 
meaning opening a document like "somedocument.pdf" (ie in the current 
directory) will make this code crash.
Also, I think writing a new file, with a fixed name and in the same 
directory is a really bad idea, as
a) you cannot be sure the current directory is writeable
b) that could be a security issue
It could be much better to just take a OutStream* as parameter, and let 
the caller create the actual kind of OutStream (file, buffer, etc). This 
also mean the glib function using setID() should take a file name (ow 
other glib IO stuff, which I do not know).

> +               FILE *f;
> +               f = fopen(filename,"r");
> +               fseek(f,0,SEEK_END);
> +               fileSize = ftell(f);

You are not doing any check for all of the three calls above (especially 
fopen), not even closing the file.

> +               fflush(stdout);

uh?

> Index: poppler-0.14.0/glib/poppler-document.cc
> [...]
> +  g_object_class_install_property
> +         (G_OBJECT_CLASS (klass),
> +          PROP_PERMANENT_ID,
> +          g_param_spec_string ("permanent-id",
> +                               "Permanent Id",
> +                               "Permanent Id of the Pdf Document",
> +                               NULL,
> +                               G_PARAM_READABLE));
> +
> +  g_object_class_install_property
> +         (G_OBJECT_CLASS (klass),
> +          PROP_UPDATE_ID,
> +          g_param_spec_string ("update-id",
> +                               "Update Id",
> +                               "Update Id of the Pdf Document",
> +                               NULL,
> +                               G_PARAM_READABLE));

Still, "Pdf" -> "PDF".

> +PopplerDocumentId*
> +poppler_document_get_pdf_id (PopplerDocument *document)
> +{
> +
> +       GooString permanent_id,update_id;
> +
> +       if(document->doc->getID(&permanent_id,&update_id)) {
> +
> +               PopplerDocumentId *doc_id =
> g_new(PopplerDocumentId,1);
> +               strcpy(doc_id->permanent_id,permanent_id.getCString(
> ));
> +               strcpy(doc_id->update_id,update_id.getCString());

I suppose there are better glib functions instead of strcpy().

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20100616/2c49a231/attachment.pgp>