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.

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?

Ah, good.

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 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!