Static analysis of Allegro code:

I have ran two static code analysis tools on Allegro and found the following bugs/suspicious code:

1) bstrlib.c:

int _al_breada (_al_bstring b, _al_bNread readPtr, void * parm) {
int i, l, n;

  if (b == NULL || b->mlen <= 0 || b->slen < 0 || b->mlen < b->slen ||
      b->mlen <= 0 || readPtr == NULL) return _AL_BSTR_ERR;

b->mlen <= 0 is checked twice, and b->data is not checked at all. Probably should be rewritten as:

int _al_breada (_al_bstring b, _al_bNread readPtr, void * parm) {
int i, l, n;

  if (b == NULL || b->mlen <= 0 || b->slen < 0 || b->mlen < b->slen ||
      b->data == NULL || readPtr == NULL) return _AL_BSTR_ERR;

The same thing happens in:
int _al_bassigngets (_al_bstring b, _al_bNgetc getcPtr, void * parm, char terminator)
int _al_bgetsa (_al_bstring b, _al_bNgetc getcPtr, void * parm, char terminator)

2) wgl_disp.c:

static bool select_pixel_format(ALLEGRO_DISPLAY_WGL *d, HDC dc)
...
ALLEGRO_EXTRA_DISPLAY_SETTINGS **eds = NULL;
...
qsort(eds, eds_count, sizeof(eds), _al_display_settings_sorter);
...

Not quite a crash-worthy bug, but sizeof(eds) is not the correct size of element here.
Since we know eds != 0, we can safely use sizeof(eds[0]). It won't crash now, but is eds
is ever changed to be something else - it could.

3) wav.c:

The same case is checked twice AND the code is different. Smells fishy, but I don't quite know what should be done here (probably the intended check was for ALLEGRO_AUDIO_DEPTH_UINT16, as the code uses .u16 instead of .s16?)

4) wav.c:

static size_t wav_stream_update(ALLEGRO_AUDIO_STREAM *stream, void *data, size_t buf_size)
...
   WAVFILE *wavfile = (WAVFILE *) stream->extra;
   bytes_per_sample = (wavfile->bits / 8) * wavfile->channels;
   ctime = wav_stream_get_position(stream);
   bytes_per_sample = (wavfile->bits / 8) * wavfile->channels;
...

bytes_per_sample is assigned the same value twice. Probably a forgotten copypaste. Nothing dangerous, but a bad style nonetheless.

5) d3d_disp.cpp:

static bool d3d_resize_helper(ALLEGRO_DISPLAY *d, int width, int height)
...
      ret = (SetWindowPos(win_display->window, HWND_TOP,
         0, 0,
         win_size.right-win_size.left,
         win_size.bottom-win_size.top,
         SWP_NOMOVE|SWP_NOZORDER)) != 0;
...

ret value is promptly ignored and later overwritten with "true". Probably a remaining part of some debug code?

6) tga.c

ALLEGRO_BITMAP *_al_load_tga_f(ALLEGRO_FILE *f, int flags)
...
buf = al_malloc(image_width * ((bpp + 1 / 8)));
...

MAJOR mistake here! I guess (bpp+1)/8 was intended, or even bpp/8 + 1. Does tga loading even work for anyone with this?

7) ogl_bitmap.c:

static ALLEGRO_LOCKED_REGION *ogl_lock_region(ALLEGRO_BITMAP *bitmap,
   int x, int y, int w, int h, int format, int flags)
...
         if (ogl_bitmap->fbo_info) {
            glGetIntegerv(GL_FRAMEBUFFER_BINDING_EXT, &current_fbo);
            
            if (ogl_bitmap)
               glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, ogl_bitmap->fbo_info->fbo);
...

Come on, either ogl_bitmap cannot be NULL, or you have to check it BEFORE using it! I suppose this check should not be here, as ogl_bitmap pointer is freely used waaaaay before this line.

8) ogl_display.c:

Now, this here is a genuine honest-to-god error. Assigning stuff to backbuffer before checking it for NULL is just plain wrong and mean.

9) upath.c:

static ALLEGRO_PATH *get_executable_name(void)
...
   fd = open(linkname, O_RDONLY);
   if (!fd == -1) {
      ioctl(fd, PIOCPSINFO, &psinfo);
      close(fd);
...

Probably if ( !(fd == -1 ) ) was meant, otherwise it makes no sense to me.

10) d3d_bmp.cpp:

static ALLEGRO_BITMAP *d3d_create_bitmap_from_surface(LPDIRECT3DSURFACE9 surface,
   int flags)
...
   bitmap = al_create_bitmap(desc.Width, desc.Height);
   extra = get_extra(bitmap);

   al_restore_state(&backup);

   if (!bitmap) {
      surface->UnlockRect();
      return NULL;
   }
...

Again, !bitmap check should come BEFORE any operations with it.

11) xrandr.c:

static bool xrandr_realign_crtc_origin(ALLEGRO_SYSTEM_XGLX *s, int xscreen, xrandr_crtc *crtc, int new_w, int new_h, int *x, int *y)
...
bool ret = false;
...
return false;
...

ret is used throught all the function, but false is ALWAYS returned. Fishy.

Addons:

12) alsa.c:

ALLEGRO_AUDIO_DEPTH_UINT8 and the company is defined thusly in allegro_audio.h:

   ALLEGRO_AUDIO_DEPTH_UINT8  = ALLEGRO_AUDIO_DEPTH_INT8 |
                                 ALLEGRO_AUDIO_DEPTH_UNSIGNED,
   ALLEGRO_AUDIO_DEPTH_UINT16 = ALLEGRO_AUDIO_DEPTH_INT16 |
                                 ALLEGRO_AUDIO_DEPTH_UNSIGNED,
   ALLEGRO_AUDIO_DEPTH_UINT24 = ALLEGRO_AUDIO_DEPTH_INT24 |
                                 ALLEGRO_AUDIO_DEPTH_UNSIGNED

Therefore, an expression voice->depth == ALLEGRO_AUDIO_DEPTH_UINT24 will be expanded to
voice->depth == ALLEGRO_AUDIO_DEPTH_INT24 | ALLEGRO_AUDIO_DEPTH_UNSIGNED, and == has higher precedence than |.
A major error, probably. Constants definitions should be enclosed in parenthesis.

13) win_dialog.c:

static void init_menu_info(MENUITEMINFO *info, ALLEGRO_MENU_ITEM *menu)
...
hbmp = CreateDIBSection(hdc, (BITMAPINFO *)&bi, DIB_RGB_COLORS, (void **)&data, NULL, 0);

lock = al_lock_bitmap(menu->icon, ALLEGRO_PIXEL_FORMAT_ARGB_8888, ALLEGRO_LOCK_READONLY);
memcpy(data, lock->data, w * h * 4);
...

Return value of CreateDIBSection is not checked (and it MAY fail), there fore data could be NULL and memcpy will crash.

14) ogv.c:

static bool generate_next_audio_fragment(VORBIS_STREAM *vstream)
...
   p = &vstream->next_fragment[
      vstream->channels * vstream->next_fragment_pos];

   if (vstream->channels == 2) {
      for (i = 0; i < samples; i++) {
         *p++ = pcm[0][i];
         *p++ = pcm[1][i];
      }
   }
...

p is not checked for NULL. I'm not sure if it CAN be NULL, but still this seems suspicious.

And that's all, folks! Analysers used: PVS-Studio 4.43 + CPPCheck 1.53 (gui version crashes on some allegro files, use command-line version).
I can submit a patch for some of errors later, but I'm not sure about some places. Could someone from main developers review it? Also, I think Allegro community can apply to PVS Studio for a free license (it provides free versions for open-source projects). It's a great tool, and should be run at least before each release (and preferably after each commit). The only downside is that it is Visual Studio only, so the license should go to someone who's main development station in Windows-based.

Nice work!

1. We don't use any of those functions, that I can tell. It might be worth checking if these are fixed in newer versions of bstrlib.

2. In fact sizeof(eds[0]) would be correct even if eds could be null. fixed

3. The second instance of ALLEGRO_AUDIO_DEPTH_INT16 is wrong (writes unsigned samples). Fortunately the correct code appears earlier in the if-then-else chain. fixed

4. Right, an oversight. fixed

5. Don't know. Someone else fix it

6. It's wrong, but just uses a lot more memory than necessary (bpp == bits per pixel). fixed

7. This was due to r15155. I don't understand why. Probably the whole condition should not exist. fixed

8. Bug. fixed

9. Ugh, it occurs in 4.4 as well. fixed (untested; the code isn't used on Linux)

10. Bug. fixed

11. The return value is never used (but probably should be). fixed

12. The constants are defined in an enum. The static analysis has a bug? not a bug

13. There are probably one zillion bugs like this, unfortunately. someone else fix it

14. p should never be NULL here, but an assert wouldn't hurt. added

Thanks!

Yes, but what if someone uses ALLEGRO_AUDIO_DEPTH_UINT16? It would not get written at all!

Uhhh, I guess so. I checked the code quickly, and thought those were defines. Apparently, analyser made the same mistake

So, do I submit a patch, or someone else take care of fixes?

The second instance is extraneous and can be deleted.

The wav file is always written as 8-bit unsigned or 16-bit signed.

The ALLEGRO_AUDIO_DEPTH_UINT16 block is fine.

Ah, good.

I edited my post above.

OK, great! I think I'll continue to run analysis on Allegro after each release or so, so if I'll find anything else, I'll be sure to report it.

I use cppcheck occasionally and others like RATS rarely. Which ones did you use in this instance?

I used trial version of PVS-Studio and CPPCheck and combined their results (there wasn't much overlap). PVS Studio has a very nice GUI, but costs way too much for an individual developer (still, you can use it in trial mode for any amount of time, and that's official company position). But as Carmack pointed out, it's better to run as many static analyzers as you can lay your hands on, since they all catch slightly different sets of errors

I also tried trial of PC-Lint, but it was so awful!