After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 729333 - orc: Add some checks on the number of variables per type
orc: Add some checks on the number of variables per type
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: orc
git master
Other Linux
: Normal normal
: NONE
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-01 14:57 UTC by Vincent Penquerc'h
Modified: 2014-06-06 11:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add some checks on the number of variables per type (11.46 KB, patch)
2014-05-01 14:57 UTC, Vincent Penquerc'h
needs-work Details | Review
add some checks on the number of variables per type (11.44 KB, patch)
2014-05-01 15:45 UTC, Vincent Penquerc'h
reviewed Details | Review
add some checks on the number of variables per type (11.63 KB, patch)
2014-06-06 11:06 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2014-05-01 14:57:41 UTC
Created attachment 275547 [details] [review]
Add some checks on the number of variables per type

Up for comments to see if anyone has opinions on the way these checks are done.
In particular, adding a new ORC_COMPILE_RESULT_LIMITS in the 0x200 series would seem better, but this would need adding a field to OrcProgram, and there are comments about ABI in there that makes me wary of adding one (there are plenty of unused fields though, which I could union...).
Comment 1 Sebastian Dröge (slomo) 2014-05-01 15:32:19 UTC
Review of attachment 275547 [details] [review]:

::: orc/orccompiler.c
@@ +194,3 @@
   ORC_INFO("initializing compiler for program \"%s\"", program->name);
+  error_msg = orc_program_get_error (program);
+  if (error_msg && strcmp (error_msg, "")) {

It should never be "", right?

::: orc/orcprogram.c
@@ +1049,3 @@
+  if (program->error_msg) {
+    free (program->error_msg);
+    program->error_msg = NULL;

Not sure this makes sense, you ideally always want to see the first error... not the last
Comment 2 Vincent Penquerc'h 2014-05-01 15:36:54 UTC
orc_program_get_error returns "" when no error.

Fair point about first error, I'll change that.
Comment 3 Vincent Penquerc'h 2014-05-01 15:45:44 UTC
Created attachment 275552 [details] [review]
add some checks on the number of variables per type
Comment 4 Sebastian Dröge (slomo) 2014-05-01 15:59:13 UTC
Review of attachment 275552 [details] [review]:

Shouldn't it return NULL instead of ""?

::: orc/orcprogram.c
@@ +1049,3 @@
+  if (!program->error_msg && error) {
+    program->error_msg = strdup (error);
+  }

Or maybe we can ensure that we never ever get multiple errors? Or that we accumulate and output them all?
Comment 5 Vincent Penquerc'h 2014-05-01 16:06:43 UTC
It could return NULL. It did seem odd, but I guess it was done for easy printf use.

Ensuring no multiple errors seems very hard given some of the code. A lot of it logs an error but does not return (just sets an error and goes on). 

Accumulating... could make sense in theory, but I'm not sure it's worth it in practice ?
Comment 6 Vincent Penquerc'h 2014-06-06 11:06:41 UTC
Created attachment 278012 [details] [review]
add some checks on the number of variables per type

Make error messages more explicit.
Comment 7 Vincent Penquerc'h 2014-06-06 11:40:06 UTC
commit 988914384acdf30eab22226d79de3541e7e6f02d
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Thu May 1 15:47:41 2014 +0100

    Add some checks on the number of variables per type
    
    We want to ensure no more than, say, 8 constants are added
    to a program. Adding more will violate pervasive assumptions
    in the code, and may lead to various buffer overflows. By
    trapping these add creation time, we prevent these issues
    without cluttering the code with range checks. The user is
    assumed non malicious here.
    
    Add a test to check we can add up to and including the limit
    for a type, but no more.