[Python-checkins] r59948 - python/trunk/Objects/structseq.c

Mon Jan 14 06:30:01 CET 2008

Christian,
Can you add some comments to the code? I'll give more detailed code
review below.
On Jan 13, 2008 7:35 PM, christian.heimes <python-checkins at python.org> wrote:
> Author: christian.heimes
> Date: Mon Jan 14 04:35:38 2008
> New Revision: 59948
>> Modified:
> python/trunk/Objects/structseq.c
> Log:
> I missed the most important file
>> Modified: python/trunk/Objects/structseq.c
> ==============================================================================
> --- python/trunk/Objects/structseq.c (original)
> +++ python/trunk/Objects/structseq.c Mon Jan 14 04:35:38 2008
> @@ -30,7 +30,7 @@
> PyStructSequence_New(PyTypeObject *type)
> {
> PyStructSequence *obj;
> -
> +
> obj = PyObject_New(PyStructSequence, type);
> Py_SIZE(obj) = VISIBLE_SIZE_TP(type);
>> @@ -230,11 +230,64 @@
> static PyObject *
> structseq_repr(PyStructSequence *obj)
> {
> - PyObject *tup, *str;
> - tup = make_tuple(obj);
> - str = PyObject_Repr(tup);
> + PyObject *tup, *val, *repr;
> + PyTypeObject *typ = Py_TYPE(obj);
> + int i, len;
> + char buf[250+5]; /* "...)0円" */
> + char *cname, *crepr;

It would be nice to limit scope for cname, crepr and any others that
are only used inside the loop.
> + char *pbuf = buf;
> + char *endbuf = &buf[250];

Is 250 enough? How did you calculate? This would be good to doc.
Also, it seems like it would be better to use either MACRO_SIZE or
sizeof() calculations so these can't get out of sync.
> + strncpy(pbuf, typ->tp_name, 50);
> + pbuf += strlen(typ->tp_name) > 50 ? 50 : strlen(typ->tp_name);

50 is mentioned 3 times. Perhaps a TYPE_NAME_SIZE? Also it would
probably be better to calculate the size you want to copy first and
then pass that to strncpy. It seems less likely to break if only one
line is changed or moved. The dependency would be clearer to me.
> + *pbuf++ = '(';
> +
> + if ((tup = make_tuple(obj)) == NULL) {
> + return NULL;
> + }
> + for (i=0; i < VISIBLE_SIZE(obj); i++) {
> + cname = typ->tp_members[i].name;
> + val = PyTuple_GetItem(tup, i);
> + if (cname == NULL || val == NULL) {
> + return NULL;
> + }
> + repr = PyObject_Repr(val);
> + if (repr == NULL) {
> + Py_DECREF(tup);
> + return NULL;
> + }
> + crepr = PyString_AsString(repr);
> + if (crepr == NULL) {
> + Py_DECREF(tup);
> + Py_DECREF(repr);
> + return NULL;
> + }
> + len = strlen(cname) + strlen(crepr) + 3;

This is a potential buffer overflow. The addition should be checked.
Also note that strlen() returns size_t which is unsigned and could be
larger than an int, eg on 64-bit platforms.
> + if ((pbuf+len) < endbuf) {

If this is the last element the condition above could be false, even
though its possible for the data to fit.
> + strcpy(pbuf, cname);
> + pbuf += strlen(cname);
> + *pbuf++ = '=';
> + strcpy(pbuf, crepr);
> + pbuf += strlen(crepr);
> + *pbuf++ = ',';
> + *pbuf++ = ' ';
> + Py_DECREF(repr);
> + }
> + else {
> + strcpy(pbuf, "...");
> + pbuf += 5;

It took me a long time to figure out why 5. I don't like that this is
dependent on the 5 added to the buffer above and that you are copying
3 bytes here. I understood once I saw the -= 2 below. But this
should be documented and perhaps refactored to try to prevent breakage
if one part of the code is modified, but not the rest.
> + Py_DECREF(repr);
> + break;
> + }
> + }
> Py_DECREF(tup);
> - return str;
> +
> + pbuf-=2;

If the tuple is empty, this will print TypeNam) rather than
TypeName(). While this is an unusual use of structseq, it should
still work.
> + *pbuf++ = ')';
> + *pbuf = '0円';
> +
> + repr = PyString_FromString(buf);
> + return repr;
> }
>> static PyObject *
> _______________________________________________
> Python-checkins mailing list
> Python-checkins at python.org
> http://mail.python.org/mailman/listinfo/python-checkins
>